Skip to content

Revamp DuckDB secret management#697

Merged
Y-- merged 11 commits into
mainfrom
yl/secrets-ddb-fdw
Apr 15, 2025
Merged

Revamp DuckDB secret management#697
Y-- merged 11 commits into
mainfrom
yl/secrets-ddb-fdw

Conversation

@Y--
Copy link
Copy Markdown
Collaborator

@Y-- Y-- commented Mar 28, 2025

In #668 we changed the configuration of MotherDuck tokens to use FOREIGN SERVER + USER MAPPING under the hood, along with some helper functions for the most common usages to get people started.

In this PR, we align our secrets management with the same mechanism. That way different PG users can use different S3 tokens to connect to the same bucket. We also introduce two helper functions create_simple_secret and create_azure_secret for the most basic usage.

We are now validating secrets using DuckDB directly and store whatever the user provides as long as it leads to a valid secret creation. This allow to support virtually any kind of secrets type and their options that DuckDB supports.

Since SERVER options are visible to any PG user, the sensitive ones such as connection_string, secret, session_token and token are prohibited to in its definition and must be stored in the USER MAPPING.

DuckDB secrets are created by concatenating SERVER and USER MAPPING options (if they exists).

Implements #683
Fixes #104

@Y-- Y-- force-pushed the yl/secrets-ddb-fdw branch 4 times, most recently from 96b42cd to 90f8916 Compare April 1, 2025 14:08
@Y-- Y-- marked this pull request as ready for review April 1, 2025 14:39
@Y-- Y-- requested review from JelteF, Copilot and wuputah April 1, 2025 14:39

This comment was marked as outdated.

Comment thread docs/secrets.md
Comment thread sql/pg_duckdb--0.3.0--0.4.0.sql Outdated
SELECT * FROM duckdb.query($$
SELECT
name,
map_from_entries(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this complex thing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It allows to have a cleaner output which makes it easier to test the expected output. Will probably simplify before merging.

Comment thread test/regression/expected/duckdb_secrets.out
Comment thread test/regression/expected/duckdb_secrets.out Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
@Y-- Y-- force-pushed the yl/secrets-ddb-fdw branch 4 times, most recently from 107bbdf to 5fa66cd Compare April 15, 2025 13:18
@Y-- Y-- requested a review from JelteF April 15, 2025 13:19
Copy link
Copy Markdown
Collaborator

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Some small nitpicks really

Comment thread include/pgduckdb/pgduckdb_fdw.hpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_options.cpp
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
Comment thread src/pgduckdb_secrets_helper.cpp Outdated
@Y-- Y-- force-pushed the yl/secrets-ddb-fdw branch 2 times, most recently from 7a4c33a to 999f808 Compare April 15, 2025 15:35
@Y-- Y-- requested a review from JelteF April 15, 2025 15:35
@Y-- Y-- force-pushed the yl/secrets-ddb-fdw branch from 999f808 to 7e98fcd Compare April 15, 2025 15:37
@Y-- Y-- force-pushed the yl/secrets-ddb-fdw branch from 7e98fcd to 423754b Compare April 15, 2025 15:50
@Y-- Y-- enabled auto-merge (squash) April 15, 2025 15:54
@Y-- Y-- merged commit b281700 into main Apr 15, 2025
6 checks passed
@Y-- Y-- deleted the yl/secrets-ddb-fdw branch April 15, 2025 15:58
Y-- added a commit that referenced this pull request Apr 23, 2025
Follow-up of #697: add support
for `ALTER SERVER` syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants