Skip to content

Disallow attaching non-DuckDB databases if enable_external_access is disabled#16015

Merged
Mytherin merged 2 commits intoduckdb:v1.2-histrionicusfrom
Mytherin:attachexternalaccess
Feb 1, 2025
Merged

Disallow attaching non-DuckDB databases if enable_external_access is disabled#16015
Mytherin merged 2 commits intoduckdb:v1.2-histrionicusfrom
Mytherin:attachexternalaccess

Conversation

@Mytherin
Copy link
Collaborator

This partially reverts the changes made in #14568

We used to fully disallow ATTACH when this setting was disabled. We now allow attaching, and fail later based on the allowed_directories/allowed_paths . This allows us to still attach files if they are within the allowed set of paths.

Storage extensions do not necessarily abide by these rules, however. For now we just move back to disallowing ATTACH of non-DuckDB files entirely if the setting is set to false.

@Mytherin Mytherin merged commit a93569e into duckdb:v1.2-histrionicus Feb 1, 2025
48 checks passed
@carlopi
Copy link
Contributor

carlopi commented Feb 1, 2025

I am not sure, I think it might not be the right place, since a sqlite file will still pass the check (say ATTACH 'sakila.db') and, less serious, but ATTACH 'duckdb:myfile.db' will not pass the check.

@Mytherin
Copy link
Collaborator Author

Mytherin commented Feb 1, 2025

That's a good point - but the SQLite file will only pass the check if it is able to be opened using the DuckDB filesystem which means it is in the allowed_directories, so that's not specifically a security concern at least. We can think about it a bit more going forward, because in general we should be able to allow SQLite to support the allowed_paths instead of being blanket disallowed.

Mytherin added a commit that referenced this pull request Feb 2, 2025
…gain - it is up to the storage extensions themselves to limit this (#16025)

Reverts #16015
@Mytherin Mytherin deleted the attachexternalaccess branch April 2, 2025 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants