diff --git a/src/include/duckdb/main/secret/secret_storage.hpp b/src/include/duckdb/main/secret/secret_storage.hpp index 1f67a7f7fb91..220ba05891e3 100644 --- a/src/include/duckdb/main/secret/secret_storage.hpp +++ b/src/include/duckdb/main/secret/secret_storage.hpp @@ -21,6 +21,8 @@ struct SecretEntry; //! Base class for SecretStorage API class SecretStorage { + friend class SecretManager; + public: SecretStorage(const string &name) : storage_name(name), persistent(false) {}; virtual ~SecretStorage() = default; diff --git a/src/main/secret/secret_manager.cpp b/src/main/secret/secret_manager.cpp index 861c27007cf1..47348d4d1a47 100644 --- a/src/main/secret/secret_manager.cpp +++ b/src/main/secret/secret_manager.cpp @@ -158,26 +158,29 @@ optional_ptr SecretManager::RegisterSecretInternal(CatalogTransacti resolved_storage = storage; } - if (resolved_storage != TEMPORARY_STORAGE_NAME && persist_type == SecretPersistType::TEMPORARY) { - throw InvalidInputException("Can not set secret storage for temporary secrets!"); - } - //! Lookup which backend to store the secret in auto backend = GetSecretStorage(resolved_storage); - if (backend) { - return backend->StoreSecret(transaction, std::move(secret), on_conflict); + if (!backend) { + throw InvalidInputException("Secret storage '%s' not found!", resolved_storage); } - if (resolved_storage == LOCAL_FILE_STORAGE_NAME) { - if (!config.allow_persistent_secrets) { - throw InvalidInputException("Persistent secrets are currently disabled. To enable them, restart duckdb and " - "run 'SET allow_persistent_secrets=true'"); - } else { - throw InternalException("The default local file storage for secrets was not found."); + // Validation on both allow_persistent_secrets and storage backend's own persist type + if (persist_type == SecretPersistType::PERSISTENT) { + if (backend->persistent) { + if (!config.allow_persistent_secrets) { + throw InvalidInputException( + "Persistent secrets are currently disabled. To enable them, restart duckdb and " + "run 'SET allow_persistent_secrets=true'"); + } + } else { // backend is temp + throw InvalidInputException("Cannot create persistent secrets in a temporary secret storage!"); + } + } else { // SecretPersistType::TEMPORARY + if (backend->persistent) { + throw InvalidInputException("Cannot create temporary secrets in a persistent secret storage!"); } } - - throw InvalidInputException("Secret storage '%s' not found!", resolved_storage); + return backend->StoreSecret(transaction, std::move(secret), on_conflict); } optional_ptr SecretManager::LookupFunctionInternal(CatalogTransaction transaction, @@ -493,10 +496,8 @@ void SecretManager::InitializeSecrets(CatalogTransaction transaction) { LoadSecretStorageInternal(make_uniq(TEMPORARY_STORAGE_NAME, *transaction.db)); // load the persistent storage if enabled - if (config.allow_persistent_secrets) { - LoadSecretStorageInternal( - make_uniq(*this, *transaction.db, LOCAL_FILE_STORAGE_NAME, config.secret_path)); - } + LoadSecretStorageInternal( + make_uniq(*this, *transaction.db, LOCAL_FILE_STORAGE_NAME, config.secret_path)); initialized = true; } diff --git a/test/secrets/test_custom_secret_storage.cpp b/test/secrets/test_custom_secret_storage.cpp index dbf4dfb67a66..2655e4139e8a 100644 --- a/test/secrets/test_custom_secret_storage.cpp +++ b/test/secrets/test_custom_secret_storage.cpp @@ -21,7 +21,7 @@ class TestSecretStorage : public CatalogSetSecretStorage { TestSecretStorage(const string &name_p, DatabaseInstance &db, TestSecretLog &logger_p, int64_t tie_break_offset_p) : CatalogSetSecretStorage(name_p), tie_break_offset(tie_break_offset_p), logger(logger_p) { secrets = make_uniq(Catalog::GetSystemCatalog(db)); - persistent = false; + persistent = true; include_in_lookups = true; } bool IncludeInLookups() override { @@ -65,7 +65,7 @@ TEST_CASE("Test adding a custom secret storage", "[secret][.]") { auto &storage_ref = *storage_ptr; secret_manager.LoadSecretStorage(std::move(storage_ptr)); - REQUIRE_NO_FAIL(con.Query("set allow_persistent_secrets=false;")); + REQUIRE_NO_FAIL(con.Query("set allow_persistent_secrets=true;")); REQUIRE_NO_FAIL(con.Query("CREATE SECRET s1 IN TEST_STORAGE (TYPE S3, SCOPE 's3://foo')")); REQUIRE_NO_FAIL(con.Query("CREATE PERSISTENT SECRET s2 IN test_storage (TYPE S3, SCOPE 's3://')")); @@ -128,7 +128,7 @@ TEST_CASE("Test tie-break behaviour for custom secret storage", "[secret][.]") { TestSecretLog log1; TestSecretLog log2; - REQUIRE_NO_FAIL(con.Query("set allow_persistent_secrets=false;")); + REQUIRE_NO_FAIL(con.Query("set allow_persistent_secrets=true;")); // Register custom secret storage auto &secret_manager = duckdb::SecretManager::Get(*db.instance); diff --git a/test/sql/secrets/create_secret_storage_backends.test b/test/sql/secrets/create_secret_storage_backends.test index 62d01d88fac7..d05407025ca8 100644 --- a/test/sql/secrets/create_secret_storage_backends.test +++ b/test/sql/secrets/create_secret_storage_backends.test @@ -16,7 +16,7 @@ set allow_persistent_secrets=false; statement error CREATE TEMPORARY SECRET s1 IN LOCAL_FILE ( TYPE S3 ) ---- -Can not set secret storage for temporary secrets! +Cannot create temporary secrets in a persistent secret storage! statement error CREATE PERSISTENT SECRET s1 IN NON_EXISTENT_SECRET_STORAGE ( TYPE S3 )