Merge pull request duckdb#10612 from samansmink/delay-secret-storage-…

…directory-initialization

delay secret storage initialization

src/main/secret/secret_manager.cpp

@@ -47,9 +47,6 @@ void SecretManager::Initialize(DatabaseInstance &db) {
 	vector<string> path_components = {".duckdb", "stored_secrets"};
 	for (auto &path_ele : path_components) {
 		config.default_secret_path = fs.JoinPath(config.default_secret_path, path_ele);
-		if (!fs.DirectoryExists(config.default_secret_path)) {
-			fs.CreateDirectory(config.default_secret_path);
-		}
 	}
 	config.secret_path = config.default_secret_path;
 

src/main/secret/secret_storage.cpp

@@ -134,13 +134,9 @@ LocalFileSecretStorage::LocalFileSecretStorage(SecretManager &manager, DatabaseI
     : CatalogSetSecretStorage(db_p, name_p), secret_path(secret_path) {
 	persistent = true;
 
+	// Check existence of persistent secret dir
 	LocalFileSystem fs;
-
-	if (!fs.DirectoryExists(secret_path)) {
-		fs.CreateDirectory(secret_path);
-	}
-
-	if (persistent_secrets.empty()) {
+	if (fs.DirectoryExists(secret_path)) {
 		fs.ListFiles(secret_path, [&](const string &fname, bool is_dir) {
 			string full_path = fs.JoinPath(secret_path, fname);
 
@@ -172,6 +168,35 @@ CatalogTransaction CatalogSetSecretStorage::GetTransactionOrDefault(optional_ptr
 
 void LocalFileSecretStorage::WriteSecret(const BaseSecret &secret, OnCreateConflict on_conflict) {
 	LocalFileSystem fs;
+
+	// We may need to create the secret dir here if the directory was not present during LocalFileSecretStorage
+	// construction
+	if (!fs.DirectoryExists(secret_path)) {
+		// TODO: recursive directory creation should probably live in filesystem
+		auto sep = fs.PathSeparator(secret_path);
+		auto splits = StringUtil::Split(secret_path, sep);
+		D_ASSERT(!splits.empty());
+		string extension_directory_prefix;
+		if (StringUtil::StartsWith(secret_path, sep)) {
+			extension_directory_prefix = sep; // this is swallowed by Split otherwise
+		}
+		try {
+			for (auto &split : splits) {
+				extension_directory_prefix = extension_directory_prefix + split + sep;
+				if (!fs.DirectoryExists(extension_directory_prefix)) {
+					fs.CreateDirectory(extension_directory_prefix);
+				}
+			}
+		} catch (std::exception &ex) {
+			ErrorData error(ex);
+			if (error.Type() == ExceptionType::IO) {
+				throw IOException("Failed to initialize persistent storage directory. (original error: '%s')",
+				                  error.RawMessage());
+			}
+			throw;
+		}
+	}
+
 	auto file_path = fs.JoinPath(secret_path, secret.GetName() + ".duckdb_secret");
 
 	if (fs.FileExists(file_path)) {
@@ -201,6 +226,7 @@ void LocalFileSecretStorage::RemoveSecret(const string &secret, OnEntryNotFound
 			                  "instance. (original error: '%s')",
 			                  file, error.RawMessage());
 		}
+		throw;
 	}
 }
 

test/sql/secrets/create_secret_non_writable_persistent_dir.test

@@ -0,0 +1,45 @@
+# name: test/sql/secrets/create_secret_non_writable_persistent_dir.test
+# description: Test persistent secrets when the secret dir is non-writable
+# group: [secrets]
+
+statement ok
+PRAGMA enable_verification;
+
+load __TEST_DIR__/create_secret_non_writable_persistent_dir.db
+
+require httpfs
+
+# First we create any file
+statement ok
+COPY (SELECT 1 as a) to '__TEST_DIR__/file_to_prevent_the_secret_dir_from_being_created.csv'
+
+# Then we set the secret dir to this.
+statement ok
+set secret_directory='__TEST_DIR__/file_to_prevent_the_secret_dir_from_being_created.csv'
+
+# Now on creation of a tmp secret, the secret manager is initialized, but the persistent secret directory creation is impossible
+statement ok
+CREATE SECRET my_tmp_secret (
+	TYPE S3,
+    SCOPE 's3://bucket1'
+)
+
+# This now fails with the message that we could not create the persistent secret directory
+statement error
+CREATE PERSISTENT SECRET my_tmp_secret (
+	TYPE S3,
+    SCOPE 's3://bucket2'
+)
+----
+
+restart
+
+# Try with a correct, deeply nested path: AOK?
+statement ok
+set secret_directory='__TEST_DIR__/create_secret_non_writable_persistent_dir/a/deeply/nested/folder/will/be/created'
+
+statement maybe
+CREATE PERSISTENT SECRET my_tmp_secret (
+	TYPE S3,
+    SCOPE 's3://bucket2'
+)

test/sql/secrets/create_secret_persistence_error_handling.test

@@ -23,16 +23,4 @@ COPY (select 1 as a ) to '__TEST_DIR__/create_secret_persistence_error_handling/
 statement error
 FROM duckdb_secrets();
 ----
-Serialization Error: Failed to deserialize the persistent secret file:
-
-
-restart
-
-statement ok
-set secret_directory='s3://very_malicious_place_you_wouldnt_want_your_secrets_to_end_up/dir'
-
-# Error should be thrown indicating that this is not a valid dir and directory creation failed
-statement error
-FROM duckdb_secrets();
-----
-create directory
+Serialization Error: Failed to deserialize the persistent secret file: