Repository: duckdb/duckdb (language: python)

Issue:
[SecretManager] Fix deserialization of Value types in KeyValueSecret::Deserialize
This PR fixes #14207

When deserializing, we now lookup the `CreateSecretFunction` which holds the `named_parameter_type_map_t named_parameters` that contains all the name+type pairs that are possible for the given secret.

From the VARCHAR we serialized, we can then DefaultCastAs to the type specified by the `named_parameters`


Discussion / hints:
Thanks!

Fix patch:
diff --git a/.github/workflows/NightlyTests.yml b/.github/workflows/NightlyTests.yml
index 24eb8372a241..2222b8b67fb9 100644
--- a/.github/workflows/NightlyTests.yml
+++ b/.github/workflows/NightlyTests.yml
@@ -978,6 +978,7 @@ jobs:
       DUCKDB_S3_ENDPOINT: duckdb-minio.com:9000
       DUCKDB_S3_USE_SSL: false
       HTTP_PROXY_PUBLIC: localhost:3128
+      TEST_PERSISTENT_SECRETS_AVAILABLE: true
 
       GEN: ninja
 
@@ -994,6 +995,9 @@ jobs:
       shell: bash
       run: sudo apt-get update -y -qq && sudo apt-get install -y -qq ninja-build
 
+    - name: Fix permissions of test secrets
+      shell: bash
+      run: chmod -R 700 data/secrets
 
     # TODO: fix the authenticated proxy here
     - name: Install and run http proxy squid
diff --git a/data/secrets/README.md b/data/secrets/README.md
new file mode 100755
index 000000000000..dccb3ed1911e
--- /dev/null
+++ b/data/secrets/README.md
@@ -0,0 +1,10 @@
+# Test secrets
+DuckDB only allows persistent secrets with the x00 permission (e.g. 600 or 700). Therefore to use these 
+secrets, the permissions need to be set before running any tests that uses them.
+
+The recommended way to add tests that touch these persistent secret files is to put them behind a
+```shell
+require-env TEST_PERSISTENT_SECRETS_AVAILABLE
+```
+statement, which ensures the tests only run in CI jobs where the permissions are set correctly.
+
diff --git a/data/secrets/http/http_v_1_1_0.duckdb_secret b/data/secrets/http/http_v_1_1_0.duckdb_secret
new file mode 100644
index 000000000000..438d15243f52
Binary files /dev/null and b/data/secrets/http/http_v_1_1_0.duckdb_secret differ
diff --git a/data/secrets/http/http_v_1_1_2.duckdb_secret b/data/secrets/http/http_v_1_1_2.duckdb_secret
new file mode 100755
index 000000000000..827b4c4582c5
Binary files /dev/null and b/data/secrets/http/http_v_1_1_2.duckdb_secret differ
diff --git a/data/secrets/http/http_v_1_1_3.duckdb_secret b/data/secrets/http/http_v_1_1_3.duckdb_secret
new file mode 100755
index 000000000000..4f9cf962fd29
Binary files /dev/null and b/data/secrets/http/http_v_1_1_3.duckdb_secret differ
diff --git a/data/secrets/httpfs/s3_config_secret_v1_1_2.duckdb_secret b/data/secrets/httpfs/s3_config_secret_v1_1_2.duckdb_secret
new file mode 100755
index 000000000000..07998250b523
Binary files /dev/null and b/data/secrets/httpfs/s3_config_secret_v1_1_2.duckdb_secret differ
diff --git a/data/secrets/httpfs/s3_config_secret_v1_1_3.duckdb_secret b/data/secrets/httpfs/s3_config_secret_v1_1_3.duckdb_secret
new file mode 100755
index 000000000000..a3e161bc17bd
Binary files /dev/null and b/data/secrets/httpfs/s3_config_secret_v1_1_3.duckdb_secret differ
diff --git a/data/secrets/httpfs/s3_config_secret_v_1_0_0.duckdb_secret b/data/secrets/httpfs/s3_config_secret_v_1_0_0.duckdb_secret
new file mode 100755
index 000000000000..7ff366232729
Binary files /dev/null and b/data/secrets/httpfs/s3_config_secret_v_1_0_0.duckdb_secret differ
diff --git a/data/secrets/httpfs/s3_secret_chain_v_1_0_0.duckdb_secret b/data/secrets/httpfs/s3_secret_chain_v_1_0_0.duckdb_secret
new file mode 100755
index 000000000000..212fb180563b
Binary files /dev/null and b/data/secrets/httpfs/s3_secret_chain_v_1_0_0.duckdb_secret differ
diff --git a/data/secrets/httpfs/s3_secret_chain_v_1_1_2.duckdb_secret b/data/secrets/httpfs/s3_secret_chain_v_1_1_2.duckdb_secret
new file mode 100755
index 000000000000..9cc6ef5ad869
Binary files /dev/null and b/data/secrets/httpfs/s3_secret_chain_v_1_1_2.duckdb_secret differ
diff --git a/data/secrets/httpfs/s3_secret_chain_v_1_1_3.duckdb_secret b/data/secrets/httpfs/s3_secret_chain_v_1_1_3.duckdb_secret
new file mode 100755
index 000000000000..b13faa6a48dd
Binary files /dev/null and b/data/secrets/httpfs/s3_secret_chain_v_1_1_3.duckdb_secret differ
diff --git a/src/common/enum_util.cpp b/src/common/enum_util.cpp
index 5938a7e8bd72..9a0db08f82be 100644
--- a/src/common/enum_util.cpp
+++ b/src/common/enum_util.cpp
@@ -6467,6 +6467,29 @@ SecretPersistType EnumUtil::FromString<SecretPersistType>(const char *value) {
 	throw NotImplementedException(StringUtil::Format("Enum value: '%s' not implemented in FromString<SecretPersistType>", value));
 }
 
+template<>
+const char* EnumUtil::ToChars<SecretSerializationType>(SecretSerializationType value) {
+	switch(value) {
+	case SecretSerializationType::CUSTOM:
+		return "CUSTOM";
+	case SecretSerializationType::KEY_VALUE_SECRET:
+		return "KEY_VALUE_SECRET";
+	default:
+		throw NotImplementedException(StringUtil::Format("Enum value: '%d' not implemented in ToChars<SecretSerializationType>", value));
+	}
+}
+
+template<>
+SecretSerializationType EnumUtil::FromString<SecretSerializationType>(const char *value) {
+	if (StringUtil::Equals(value, "CUSTOM")) {
+		return SecretSerializationType::CUSTOM;
+	}
+	if (StringUtil::Equals(value, "KEY_VALUE_SECRET")) {
+		return SecretSerializationType::KEY_VALUE_SECRET;
+	}
+	throw NotImplementedException(StringUtil::Format("Enum value: '%s' not implemented in FromString<SecretSerializationType>", value));
+}
+
 template<>
 const char* EnumUtil::ToChars<SequenceInfo>(SequenceInfo value) {
 	switch(value) {
diff --git a/src/include/duckdb/common/enum_util.hpp b/src/include/duckdb/common/enum_util.hpp
index 7cae3772ed57..3e6a235aa0c6 100644
--- a/src/include/duckdb/common/enum_util.hpp
+++ b/src/include/duckdb/common/enum_util.hpp
@@ -278,6 +278,8 @@ enum class SecretDisplayType : uint8_t;
 
 enum class SecretPersistType : uint8_t;
 
+enum class SecretSerializationType : uint8_t;
+
 enum class SequenceInfo : uint8_t;
 
 enum class SetOperationType : uint8_t;
@@ -728,6 +730,9 @@ const char* EnumUtil::ToChars<SecretDisplayType>(SecretDisplayType value);
 template<>
 const char* EnumUtil::ToChars<SecretPersistType>(SecretPersistType value);
 
+template<>
+const char* EnumUtil::ToChars<SecretSerializationType>(SecretSerializationType value);
+
 template<>
 const char* EnumUtil::ToChars<SequenceInfo>(SequenceInfo value);
 
@@ -1218,6 +1223,9 @@ SecretDisplayType EnumUtil::FromString<SecretDisplayType>(const char *value);
 template<>
 SecretPersistType EnumUtil::FromString<SecretPersistType>(const char *value);
 
+template<>
+SecretSerializationType EnumUtil::FromString<SecretSerializationType>(const char *value);
+
 template<>
 SequenceInfo EnumUtil::FromString<SequenceInfo>(const char *value);
 
diff --git a/src/include/duckdb/main/secret/secret.hpp b/src/include/duckdb/main/secret/secret.hpp
index f3257f005217..ee721d7f885b 100644
--- a/src/include/duckdb/main/secret/secret.hpp
+++ b/src/include/duckdb/main/secret/secret.hpp
@@ -37,8 +37,7 @@ struct CreateSecretInput {
 	case_insensitive_map_t<Value> options;
 };
 
-typedef unique_ptr<BaseSecret> (*secret_deserializer_t)(Deserializer &deserializer, BaseSecret base_secret,
-                                                        const named_parameter_type_map_t &options);
+typedef unique_ptr<BaseSecret> (*secret_deserializer_t)(Deserializer &deserializer, BaseSecret base_secret);
 typedef unique_ptr<BaseSecret> (*create_secret_function_t)(ClientContext &context, CreateSecretInput &input);
 
 //! A CreateSecretFunction is a function adds a provider for a secret type.
@@ -81,6 +80,13 @@ struct SecretType {
 	string default_provider;
 };
 
+enum class SecretSerializationType : uint8_t {
+	//! The secret is serialized with a custom serialization function
+	CUSTOM = 0,
+	//! The secret has been serialized as a KeyValueSecret
+	KEY_VALUE_SECRET = 1
+};
+
 //! Base class from which BaseSecret classes can be made.
 class BaseSecret {
 	friend class SecretManager;
@@ -181,30 +187,14 @@ class KeyValueSecret : public BaseSecret {
 
 	// FIXME: use serialization scripts
 	template <class TYPE>
-	static unique_ptr<BaseSecret> Deserialize(Deserializer &deserializer, BaseSecret base_secret,
-	                                          const named_parameter_type_map_t &options) {
+	static unique_ptr<BaseSecret> Deserialize(Deserializer &deserializer, BaseSecret base_secret) {
 		auto result = make_uniq<TYPE>(base_secret);
 		Value secret_map_value;
 		deserializer.ReadProperty(201, "secret_map", secret_map_value);
 
 		for (const auto &entry : ListValue::GetChildren(secret_map_value)) {
 			auto kv_struct = StructValue::GetChildren(entry);
-			auto key = kv_struct[0].ToString();
-			auto raw_value = kv_struct[1].ToString();
-
-			auto it = options.find(key);
-			if (it == options.end()) {
-				throw IOException("Failed to deserialize secret '%s', it contains an unexpected key: '%s'",
-				                  base_secret.GetName(), key);
-			}
-			auto &logical_type = it->second;
-			Value value;
-			if (logical_type.id() == LogicalTypeId::VARCHAR) {
-				value = Value(raw_value);
-			} else {
-				value = Value(raw_value).DefaultCastAs(logical_type);
-			}
-			result->secret_map[key] = value;
+			result->secret_map[kv_struct[0].ToString()] = kv_struct[1];
 		}
 
 		Value redact_set_value;
diff --git a/src/main/secret/secret.cpp b/src/main/secret/secret.cpp
index 8535ef6313ea..9a39b7176817 100644
--- a/src/main/secret/secret.cpp
+++ b/src/main/secret/secret.cpp
@@ -87,7 +87,8 @@ void KeyValueSecret::Serialize(Serializer &serializer) const {
 		map_values.push_back(Value::STRUCT(map_struct));
 	}
 
-	auto map_type = LogicalType::MAP(LogicalType::VARCHAR, LogicalType::VARCHAR);
+	// Warning: the secret map is serialized into a single MAP value with type ANY
+	auto map_type = LogicalType::MAP(LogicalType::VARCHAR, LogicalType::ANY);
 	auto map = Value::MAP(ListType::GetChildType(map_type), map_values);
 	serializer.WriteProperty(201, "secret_map", map);
 
diff --git a/src/main/secret/secret_manager.cpp b/src/main/secret/secret_manager.cpp
index f783a0c3e24a..045d1efe8088 100644
--- a/src/main/secret/secret_manager.cpp
+++ b/src/main/secret/secret_manager.cpp
@@ -97,6 +97,20 @@ unique_ptr<BaseSecret> SecretManager::DeserializeSecret(Deserializer &deserializ
 	vector<string> scope;
 	deserializer.ReadList(103, "scope",
 	                      [&](Deserializer::List &list, idx_t i) { scope.push_back(list.ReadElement<string>()); });
+	auto serialization_type =
+	    deserializer.ReadPropertyWithExplicitDefault(104, "serialization_type", SecretSerializationType::CUSTOM);
+
+	switch (serialization_type) {
+	// This allows us to skip looking up the secret type for deserialization altogether
+	case SecretSerializationType::KEY_VALUE_SECRET:
+		return KeyValueSecret::Deserialize<KeyValueSecret>(deserializer, {scope, type, provider, name});
+	// Continues below: we need to do a type lookup to find the secret deserialize method
+	case SecretSerializationType::CUSTOM:
+		break;
+	default:
+		throw IOException("Unrecognized secret serialization type found in secret '%s': %s", secret_path,
+		                  EnumUtil::ToString(serialization_type));
+	}
 
 	SecretType deserialized_type;
 	if (!TryLookupTypeInternal(type, deserialized_type)) {
@@ -108,15 +122,7 @@ unique_ptr<BaseSecret> SecretManager::DeserializeSecret(Deserializer &deserializ
 		    "Attempted to deserialize secret type '%s' which does not have a deserialization method", type);
 	}
 
-	auto function_entry = LookupFunctionInternal(type, provider);
-	if (!function_entry) {
-		throw IOException("Attempted to deserialize secret (type: '%s', provider: '%s', path: '%s') which does not "
-		                  "have any functions registered",
-		                  type, provider, secret_path);
-	}
-
-	return deserialized_type.deserializer(deserializer, {scope, type, provider, name},
-	                                      function_entry->named_parameters);
+	return deserialized_type.deserializer(deserializer, {scope, type, provider, name});
 }
 
 void SecretManager::RegisterSecretType(SecretType &type) {


Test patch:
diff --git a/test/sql/secrets/secret_compatibility_http.test b/test/sql/secrets/secret_compatibility_http.test
new file mode 100644
index 000000000000..98df0034d44c
--- /dev/null
+++ b/test/sql/secrets/secret_compatibility_http.test
@@ -0,0 +1,16 @@
+# name: test/sql/secrets/secret_compatibility_http.test
+# description: Test secret compatibility across versions
+# group: [secrets]
+
+require-env TEST_PERSISTENT_SECRETS_AVAILABLE
+
+# Ensure any currently stored secrets don't interfere with the test
+statement ok
+set secret_directory='./data/secrets/http'
+
+query IIIIIII
+from duckdb_secrets();
+----
+http_v_1_1_0	http	config	true	local_file	[]	name=http_v_1_1_0;type=http;provider=config;serializable=true;scope;extra_http_headers={Authorization=Bearer sk_test_VePHdqKTYQjKNInc7u56JBrQ}
+http_v_1_1_2	http	config	true	local_file	[]	name=http_v_1_1_2;type=http;provider=config;serializable=true;scope;extra_http_headers={Authorization=Bearer sk_test_VePHdqKTYQjKNInc7u56JBrQ}
+http_v_1_1_3	http	config	true	local_file	[]	name=http_v_1_1_3;type=http;provider=config;serializable=true;scope;extra_http_headers={Authorization=Bearer sk_test_VePHdqKTYQjKNInc7u56JBrQ}
\ No newline at end of file
diff --git a/test/sql/secrets/secret_compatibility_httpfs.test b/test/sql/secrets/secret_compatibility_httpfs.test
new file mode 100644
index 000000000000..954347397b84
--- /dev/null
+++ b/test/sql/secrets/secret_compatibility_httpfs.test
@@ -0,0 +1,23 @@
+# name: test/sql/secrets/secret_compatibility_httpfs.test
+# description: Test secret compatibility across versions
+# group: [secrets]
+
+require httpfs
+
+require-env TEST_PERSISTENT_SECRETS_AVAILABLE
+
+# Ensure any currently stored secrets don't interfere with the test
+statement ok
+set secret_directory='./data/secrets/httpfs'
+
+mode output_result
+
+query IIIIIII
+from duckdb_secrets() order by name;
+----
+s3_config_secret_v1_1_2	s3	config	true	local_file	[s3://, s3n://, s3a://]	name=s3_config_secret_v1_1_2;type=s3;provider=config;serializable=true;scope=s3://,s3n://,s3a://;region=us-east-2;use_ssl=false
+s3_config_secret_v1_1_3	s3	config	true	local_file	[s3://, s3n://, s3a://]	name=s3_config_secret_v1_1_3;type=s3;provider=config;serializable=true;scope=s3://,s3n://,s3a://;region=us-east-2;use_ssl=false
+s3_config_secret_v_1_0_0	s3	config	true	local_file	[s3://, s3n://, s3a://]	name=s3_config_secret_v_1_0_0;type=s3;provider=config;serializable=true;scope=s3://,s3n://,s3a://;endpoint=s3.amazonaws.com;key_id=;region=us-east-2;s3_url_compatibility_mode=0;secret=redacted;session_token=redacted;url_style=;use_ssl=0
+s3_secret_chain_v_1_0_0	s3	credential_chain	true	local_file	[s3://, s3n://, s3a://]	name=s3_secret_chain_v_1_0_0;type=s3;provider=credential_chain;serializable=true;scope=s3://,s3n://,s3a://;endpoint=s3.amazonaws.com;region=us-east-2;use_ssl=false
+s3_secret_chain_v_1_1_2	s3	credential_chain	true	local_file	[s3://, s3n://, s3a://]	name=s3_secret_chain_v_1_1_2;type=s3;provider=credential_chain;serializable=true;scope=s3://,s3n://,s3a://;endpoint=s3.amazonaws.com;region=us-east-2;use_ssl=false
+s3_secret_chain_v_1_1_3	s3	credential_chain	true	local_file	[s3://, s3n://, s3a://]	name=s3_secret_chain_v_1_1_3;type=s3;provider=credential_chain;serializable=true;scope=s3://,s3n://,s3a://;endpoint=s3.amazonaws.com;region=us-east-2;use_ssl=false
\ No newline at end of file