duckdb / duckdb/duckdb-httpfs

Secret refresh re-creates the secret in its origin storage, which breaks (or silently mutates) extension-registered secret storages

Open
#412 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
60
Forks
100
Avg merge
1h 50m
Merged PRs (30d)
25

Description

### What happens

`TryRefreshS3Secret` refreshes a secret by generating a `CreateSecretInput` from the existing entry and calling `SecretManager::CreateSecret`. `GenerateRefreshSecretInfo` copies the entry's persistence and storage:

```cpp
result.persist_type = secret_entry.persist_type;
...
if (result.persist_type != SecretPersistType::TRANSACTION) {
result.storage_type = Identifier(secret_entry.storage_mode);
}
```

(`src/create_secret_functions.cpp`, `GenerateRefreshSecretInfo`)

For secrets in the built-in `memory`/`local_file` storages this is fine. For secrets loaded from an **extension-registered storage** — e.g. a PostgreSQL-backed store attached with duckdb-postgres's `SECRET_STORAGE_TABLE` — re-creating into the origin storage is wrong in both possible directions:

1. **Read-only storage session** → the refresh throws (surfaced as `Exception thrown while trying to refresh secret …`, or `Cannot create temporary secrets in a persistent secret storage` depending on persist-type mismatch), so a read that could have succeeded after re-resolving credentials fails instead.
2. **Writable storage session** → the refresh **silently writes the secret back to the shared store**. A session-local credential re-resolution becomes a durable, multi-user mutation. We have observed a long-lived session *resurrecting rows that had been deleted from the store* hours earlier, because its loaded copy refreshed and re-persisted them.

Refreshed credential material is inherently session-local state (the point of `credential_chain` + `refresh` is "my resolved credentials went stale — resolve again"); persisting it to a shared backing store is never the right side effect, and failing the read because the store is read-only is also not the right outcome.

### Reproduction sketch

Deterministic without waiting for STS expiry — a wrong-key secret with `refresh 'auto'` triggers the refresh path on the first 403:

```sql
-- store attach per duckdb-postgres:
ATTACH '' AS store (TYPE postgres, SECRET meta,
SECRET_STORAGE_TABLE 'duckdb.duckdb_secrets');
-- a secret that lives in that storage, with refresh enabled
-- (created via a writable session, or seeded externally)
-- then, in a session whose store role is read-only:
FROM 's3://some-bucket/some.parquet';
-- → auth failure → TryRefreshS3Secret → CreateSecret(storage_type='postgres_store')
-- → refresh exception instead of a re-resolve
-- in a writable session: the row's contents/timestamp change in the store
```

### Suggested fix

Refresh should update the loaded secret's credential material **in place**, without any storage write: a `SecretManager` API that swaps the cached entry's contents transiently, which `TryRefreshS3Secret` calls instead of `CreateSecret`. The existing `// TODO: change SecretManager API to avoid requiring catching this exception` in `TryRefreshS3Secret` points at the same seam, and an in-place update would remove that exception-catching along the way.

Re-creating the refreshed secret into `memory` storage instead would avoid the storage write but leave the same name live in two storages at once — the by-name ambiguity state that `GetSecretByName` and `DROP SECRET` treat as an error — so in-place is the direction that fixes refresh without introducing a new inconsistency.

### Related

- #195 reports the read-only half of this on the built-in persistent storage (`Cannot create temporary secrets in a persistent secret storage` for an `sts`-chain persistent secret) — same root cause, seen from the storage that can only fail loudly. An in-place refresh would resolve it too.
- The current copy of `persist_type`/`storage_mode` into the refresh's `CreateSecretInput` appears to originate in the fix for #380 (previously the refresh hardcoded `TEMPORARY`). That fix was right for TRANSACTION-scoped secrets, but for writable non-built-in storages it makes the refresh's `CreateSecret` land as a durable write — the silent-mutation half above.

Contributor guide

No contributing guide indexed for this repository

Research direction

Read src/create_secret_functions.cpp, especially GenerateRefreshSecretInfo, and follow TryRefreshS3Secret through SecretManager::CreateSecret. Replace the refresh path's storage re-creation with the proposed transient in-place update of the loaded credential material. Done means refresh re-resolves credentials without writing to the origin storage, including read-only and writable extension-registered storage sessions.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, postgresql
Domain
databases, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.