Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling of extension loading (through a CMake flag) and allow selectively disabling specific file systems #8152

Merged
merged 8 commits into from
Jul 6, 2023

Conversation

Mytherin
Copy link
Collaborator

@Mytherin Mytherin commented Jul 5, 2023

This PR adds support for more granular permission control:

  • Support for extension loading and extension installation can be disabled entirely using the -DDISABLE_EXTENSION_LOAD=1 option. When compiled with this option attempting to load or install an external extension (from either remote or from local storage) results in a permission exception.
  • Support for specific file systems can be selectively disabled using the disabled_filesystems setting. With this setting we can for example disable the local file system, while keeping support for reading from the remote file system.
    For example:
-- works
SELECT * FROM read_csv_auto('test.csv')
-- disable local file system
SET disabled_filesystems='LocalFileSystem';
-- PermissionException: "LocalFileSystem" has been disabled
SELECT * FROM read_csv_auto('test.csv')
-- works
SELECT * FROM read_csv_auto('https://github.com/duckdb/duckdb/raw/master/data/csv/customer.csv')


-- disable http file system as well
SET disabled_filesystems='LocalFileSystem,HTTPFileSystem';
-- PermissionException: "HTTPFileSystem" has been disabled
SELECT * FROM read_csv_auto('https://github.com/duckdb/duckdb/raw/master/data/csv/customer.csv')

Note that any file system that has previously been disabled cannot be re-enabled without restarting the server (i.e. we can only add to the list of disabled file systems).

@Mytherin Mytherin requested a review from samansmink July 5, 2023 13:30
Copy link
Contributor

@samansmink samansmink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! No comments on the changes. One thing I noted however is that we have another dlopen for adbc in src/common/adbc/driver_manager.cpp. Not sure if relevant, but should that be stripped too?

@github-actions github-actions bot marked this pull request as draft July 5, 2023 18:11
@Mytherin Mytherin marked this pull request as ready for review July 5, 2023 18:11
@Mytherin
Copy link
Collaborator Author

Mytherin commented Jul 5, 2023

Looks good! No comments on the changes. One thing I noted however is that we have another dlopen for adbc in src/common/adbc/driver_manager.cpp. Not sure if relevant, but should that be stripped too?

The ADBC code is not accessible through SQL and is only relevant when linking in an ADBC application into the library directly, so that should be fine to leave

@Mytherin Mytherin merged commit a6f0e61 into duckdb:master Jul 6, 2023
53 of 54 checks passed
@Mytherin Mytherin deleted the granularpermission branch December 4, 2023 11:44
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.

None yet

2 participants