Skip to content

Commit 18361e5

Browse files
rustyconoverclaude
andcommitted
feat(catalog): secret flag on AttachOptionSpec for credential attach options
Part A of the attach-credentials proposal. AttachOption/AttachOptionSpec gain `secret: bool = False`, carried on the wire as a nullable bool column named "secret" appended after "required". Readers look columns up by name, so a spec without the column reads as secret=False and older peers ignore it. `secret` combines with `required`, and is allowed with a default (documented as discouraged). Credential options (API keys, tokens, passwords) must be declared secret=True so clients and the DuckDB extension can mask them, keep them out of cache keys and logs, and supply them from a `vgi_attach` secret. - the attach-options fixture's required api_key is now secret - worker page and landing page flag required/secret options and never show a secret option's default - docs: "Declaring Attach Options" in docs/catalog-interface.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 4906f11 commit 18361e5

9 files changed

Lines changed: 241 additions & 17 deletions

File tree

‎CLAUDE.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,8 @@ Set `SENTRY_DSN` (and install `vgi[sentry]`) to forward unhandled exceptions to
392392

393393
**Attach options redaction:** by default *no* options are logged in the `catalog.attach` / `catalog.create` breadcrumbs because options routinely carry credentials. Implementers opt in via `CatalogInterface.loggable_attach_options(options) -> Mapping`, which returns a redacted, safe-to-log subset (host, region, bucket — never password/token/secret). When the override returns an empty mapping (the default), the `options` field is omitted from the breadcrumb entirely. See `docs/catalog-interface.md` for details.
394394

395+
**Secret attach options:** credential options (API keys, tokens, passwords) MUST be declared `AttachOption(..., secret=True)`. `AttachOptionSpec.secret` rides as a nullable `bool` column named `secret` appended after `required` (an absent column reads as `False`); clients and the DuckDB extension mask such options, keep them out of cache keys and logs, and can supply them from a `vgi_attach` DuckDB secret (`CREATE SECRET (TYPE vgi_attach, SCOPE '<worker url>', api_key '…')`). Secret + required is fine; secret + default is allowed but discouraged.
396+
395397
**Releases.** `vgi-serve` populates `release` from `SENTRY_RELEASE` if set, otherwise from `importlib.metadata.version("vgi-python")`. Production deploys should set `SENTRY_RELEASE=$(git rev-parse HEAD)` (or a tagged release identifier) so Sentry's commit-tracking and regression-detection features can correlate events to specific commits. Publishing GitHub releases makes the release-comparison UI more useful.
396398

397399
The same enrichment applies to OTel spans when `VGI_OTEL_ENABLED=1` — both backends read from the same `VgiTracer.set_current_span_attributes()` call sites in `vgi/otel.py`. Either, neither, or both can be active in a process.

‎docs/catalog-interface.md‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,42 @@ class MyCatalog(CatalogInterface):
246246

247247
---
248248

249+
## Declaring Attach Options
250+
251+
A worker declares the options its catalogs accept at `ATTACH` time in an `AttachOptions` inner class. Each option is advertised to clients at discovery (`CatalogInfo.attach_option_specs`), so DuckDB can cast and validate it, and frontends can render an options form.
252+
253+
```python test="skip"
254+
from typing import Annotated
255+
from vgi import Worker
256+
from vgi.catalog.attach_option import AttachOption
257+
258+
class MyWorker(Worker):
259+
class AttachOptions:
260+
region: Annotated[str, AttachOption(desc="AWS region")] = "us-east-1"
261+
# A credential: required, secret, and with no default.
262+
api_key: Annotated[str, AttachOption(desc="API key", required=True, secret=True)]
263+
```
264+
265+
| Flag | Meaning |
266+
|------|---------|
267+
| `required=True` | The caller must supply the option. Declare it with no class-level default; `required` plus a default is rejected. |
268+
| `secret=True` | The option carries a credential. |
269+
270+
**Credential options (API keys, tokens, passwords) MUST be declared `secret=True`.** Clients and the DuckDB extension then mask the value, keep it out of result-cache keys and logs, and can supply it from a `vgi_attach` DuckDB secret, so the `ATTACH` statement never carries it:
271+
272+
```sql
273+
CREATE SECRET (TYPE vgi_attach, SCOPE 'https://worker.example.com', api_key '...');
274+
ATTACH 'mydb' AS mydb (TYPE vgi, LOCATION 'https://worker.example.com');
275+
```
276+
277+
`secret` combines with `required`. It is allowed together with a default, but a secret option should normally have none: the default is advertised in plain text to every client at discovery.
278+
279+
On the wire, `required` and `secret` are nullable boolean columns appended after the four shared spec columns (`name`, `description`, `type`, `default_value`), in that order. Readers look columns up by name, so a spec from a peer that predates either column reads it as `false`, and older peers ignore columns they don't know.
280+
281+
`secret` does not change what the worker itself logs: attach options are never logged unless the catalog opts in through `loggable_attach_options()` (below), which must never return a secret option.
282+
283+
---
284+
249285
## Logging Attach Options Safely
250286

251287
The worker emits structured `_logger.info` records and Sentry breadcrumbs for catalog lifecycle events (`catalog.attach`, `catalog.detach`, `catalog.create`, `catalog.transaction.begin`, `catalog.transaction.commit`, `catalog.transaction.rollback`). `attach_opaque_data` and `transaction_opaque_data` are short-hashed (12-char SHA-256 prefixes) before they reach the log record, the breadcrumb data, or the Sentry scope tags — the raw values never appear in observability output, since they may carry secrets. An operator correlates the short hash back to the catalog via these breadcrumbs.

‎tests/catalog/test_attach_option.py‎

Lines changed: 121 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Copyright 2025, 2026 Query Farm LLC - https://query.farm
22

3-
"""Tests for the AttachOption descriptor, its ``required`` flag, and validation."""
3+
"""Tests for the AttachOption descriptor, its ``required`` and ``secret`` flags, and validation."""
44

55
from typing import Annotated
66

@@ -25,7 +25,11 @@ class _Options:
2525

2626
region: Annotated[str, AttachOption(desc="AWS region")] = "us-east-1"
2727
# No class-level assignment: nothing to fall back to, so the caller must supply it.
28-
api_key: Annotated[str, AttachOption(desc="API key", required=True)]
28+
api_key: Annotated[str, AttachOption(desc="API key", required=True, secret=True)]
29+
# A credential that may be omitted, with no default (the recommended shape).
30+
token: Annotated[str | None, AttachOption(desc="Optional token", arrow_type=pa.string(), secret=True)] = None
31+
# Allowed, though discouraged: a secret option with a default.
32+
password: Annotated[str, AttachOption(desc="Password", secret=True)] = "changeme"
2933

3034

3135
def _roundtrip(spec: AttachOptionSpec) -> AttachOptionSpec:
@@ -83,6 +87,7 @@ def test_setting_wire_format_unchanged(self) -> None:
8387
"type",
8488
"default_value",
8589
"required",
90+
"secret",
8691
]
8792

8893
def test_reads_batch_without_required_column(self) -> None:
@@ -124,6 +129,120 @@ def test_null_required_reads_as_false(self) -> None:
124129
assert AttachOptionSpec.deserialize(restored).required is False
125130

126131

132+
class TestSecretExtraction:
133+
"""Extraction of the ``secret`` flag from Annotated declarations."""
134+
135+
def test_defaults_to_false(self) -> None:
136+
"""An option that isn't declared secret isn't secret."""
137+
specs = {s.name: s for s in extract_attach_option_specs(_Options)}
138+
assert specs["region"].secret is False
139+
140+
def test_secret_with_required(self) -> None:
141+
"""``secret`` and ``required`` combine: a credential the caller must supply."""
142+
specs = {s.name: s for s in extract_attach_option_specs(_Options)}
143+
assert specs["api_key"].secret is True
144+
assert specs["api_key"].required is True
145+
assert specs["api_key"].default is None
146+
147+
def test_secret_without_required(self) -> None:
148+
"""A secret option need not be required."""
149+
specs = {s.name: s for s in extract_attach_option_specs(_Options)}
150+
assert specs["token"].secret is True
151+
assert specs["token"].required is False
152+
153+
def test_secret_with_default_allowed(self) -> None:
154+
"""A secret option may declare a default (discouraged, but not an error)."""
155+
specs = {s.name: s for s in extract_attach_option_specs(_Options)}
156+
assert specs["password"].secret is True
157+
assert specs["password"].default == "changeme"
158+
159+
def test_secret_required_and_default_still_rejected(self) -> None:
160+
"""``secret`` doesn't relax the required + default contradiction."""
161+
with pytest.raises(ValueError, match="required but also declares a default"):
162+
AttachOptionSpec(name="k", desc="", type=pa.string(), default="x", required=True, secret=True)
163+
164+
165+
class TestSecretWireFormat:
166+
"""Serialization of the added ``secret`` column."""
167+
168+
@pytest.mark.parametrize("secret", [True, False], ids=["secret", "plain"])
169+
def test_roundtrip_preserves_secret(self, secret: bool) -> None:
170+
"""``secret`` survives an Arrow IPC round trip in both states."""
171+
spec = AttachOptionSpec(name="api_key", desc="API key", type=pa.string(), default=None, secret=secret)
172+
restored = _roundtrip(spec)
173+
assert restored.secret is secret
174+
assert restored.required is False
175+
176+
@pytest.mark.parametrize(
177+
("required", "secret"),
178+
[(True, True), (True, False), (False, True), (False, False)],
179+
)
180+
def test_roundtrip_required_and_secret_independent(self, required: bool, secret: bool) -> None:
181+
"""The two flags occupy separate columns and never bleed into each other."""
182+
spec = AttachOptionSpec(
183+
name="api_key", desc="API key", type=pa.string(), default=None, required=required, secret=secret
184+
)
185+
restored = _roundtrip(spec)
186+
assert (restored.required, restored.secret) == (required, secret)
187+
188+
def test_secret_column_follows_required(self) -> None:
189+
"""``secret`` is a nullable bool appended after ``required``."""
190+
schema = AttachOptionSpec.ARROW_SCHEMA
191+
assert schema.get_field_index("secret") == schema.get_field_index("required") + 1
192+
assert schema.get_field_index("secret") == len(schema) - 1
193+
field = schema.field("secret")
194+
assert field.type == pa.bool_()
195+
assert field.nullable
196+
197+
def test_reads_batch_without_secret_column(self) -> None:
198+
"""A peer that has ``required`` but predates ``secret`` yields ``secret=False``."""
199+
type_bytes = pa.schema([pa.field("value", pa.string())]).serialize().to_pybytes()
200+
older_schema = pa.schema([*_SpecBase.ARROW_SCHEMA, pa.field("required", pa.bool_(), nullable=True)])
201+
older = pa.RecordBatch.from_pylist(
202+
[{"name": "api_key", "description": "", "type": type_bytes, "default_value": None, "required": True}],
203+
schema=older_schema,
204+
)
205+
restored_batch, _ = deserialize_record_batch(serialize_record_batch_bytes(older))
206+
restored = AttachOptionSpec.deserialize(restored_batch)
207+
assert restored.secret is False
208+
assert restored.required is True
209+
210+
def test_reads_four_column_batch_as_not_secret(self) -> None:
211+
"""A peer that predates both columns reads as neither required nor secret."""
212+
type_bytes = pa.schema([pa.field("value", pa.string())]).serialize().to_pybytes()
213+
legacy = pa.RecordBatch.from_pylist(
214+
[{"name": "region", "description": "", "type": type_bytes, "default_value": None}],
215+
schema=_SpecBase.ARROW_SCHEMA,
216+
)
217+
restored = AttachOptionSpec.deserialize(legacy)
218+
assert (restored.required, restored.secret) == (False, False)
219+
220+
def test_null_secret_reads_as_false(self) -> None:
221+
"""An explicit null in the nullable column means "not secret"."""
222+
type_bytes = pa.schema([pa.field("value", pa.string())]).serialize().to_pybytes()
223+
batch = pa.RecordBatch.from_pylist(
224+
[
225+
{
226+
"name": "region",
227+
"description": "",
228+
"type": type_bytes,
229+
"default_value": None,
230+
"required": None,
231+
"secret": None,
232+
}
233+
],
234+
schema=AttachOptionSpec.ARROW_SCHEMA,
235+
)
236+
restored, _ = deserialize_record_batch(serialize_record_batch_bytes(batch))
237+
assert AttachOptionSpec.deserialize(restored).secret is False
238+
239+
def test_older_reader_ignores_secret_column(self) -> None:
240+
"""A four-column reader still reads a six-column batch (forward compatible)."""
241+
spec = AttachOptionSpec(name="api_key", desc="", type=pa.string(), default=None, required=True, secret=True)
242+
batch, _ = deserialize_record_batch(spec.serialize())
243+
assert SettingSpec.deserialize(batch).name == "api_key"
244+
245+
127246
class TestValidateRequiredAttachOptions:
128247
"""The shared validation helper."""
129248

‎tests/test_worker_page.py‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,10 @@
2323
Table,
2424
View,
2525
)
26-
from vgi.catalog.attach_option import AttachOption, extract_attach_option_specs
26+
from vgi.catalog.attach_option import AttachOption, AttachOptionSpec, extract_attach_option_specs
2727
from vgi.http.worker_page import (
2828
WorkerPageResource,
29+
_build_attach_options_table,
2930
_display_function_type,
3031
build_worker_page,
3132
)
@@ -542,6 +543,20 @@ def test_options_render_with_types_and_defaults(self) -> None:
542543
# Defaults render via repr() and HTML-escape the quotes.
543544
assert "&#x27;hello&#x27;" in html
544545

546+
def test_required_and_secret_flags_render(self) -> None:
547+
"""Required/secret options are flagged, and a secret's default is never shown."""
548+
specs = (
549+
AttachOptionSpec(name="api_key", desc="", type=pa.string(), default=None, required=True, secret=True),
550+
AttachOptionSpec(name="password", desc="", type=pa.string(), default="hunter2", secret=True),
551+
AttachOptionSpec(name="region", desc="", type=pa.string(), default="us-east-1"),
552+
)
553+
html = _build_attach_options_table(specs)
554+
assert ">required</span>" in html
555+
assert ">secret</span>" in html
556+
assert "hunter2" not in html
557+
assert "<em>hidden</em>" in html
558+
assert "&#x27;us-east-1&#x27;" in html
559+
545560

546561
class TestVersionDisplay:
547562
"""Implementation chip + Data version input."""

‎vgi/_test_fixtures/attach_options.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ class RequiredAttachOptions:
123123

124124
# No class-level assignment: there is no default to fall back on, so the
125125
# caller has to supply it.
126-
api_key: Annotated[str, AttachOption(desc="API key", required=True)]
126+
api_key: Annotated[str, AttachOption(desc="API key", required=True, secret=True)]
127127
region: Annotated[str, AttachOption(desc="Region")] = "us-east-1"
128128

129129

‎vgi/catalog/_descriptor_spec.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
names and behaviour are unchanged.
1111
1212
A subclass may widen its wire format with additional columns — `AttachOption`
13-
adds ``required`` — via :meth:`_SpecBase._extra_row` /
13+
adds ``required`` and ``secret`` — via :meth:`_SpecBase._extra_row` /
1414
:meth:`_SpecBase._extra_kwargs` and :meth:`_DescriptorBase.extra_spec_kwargs`.
1515
Because both directions read by column name, the widening is compatible in both
1616
directions: a reader ignores columns it doesn't know, and a reader whose peer

‎vgi/catalog/attach_option.py‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,14 @@
99
The declaration mirrors ``vgi.catalog.setting.Setting`` — same Arrow IPC spec
1010
format, same Python type → Arrow mapping, same extractor shape — so both share
1111
the machinery in ``vgi.catalog._descriptor_spec``.
12+
13+
Credential options (API keys, tokens, passwords) MUST be declared
14+
``secret=True``. Clients and the DuckDB extension then mask the value, keep it
15+
out of result-cache keys and logs, and can supply it from a ``vgi_attach``
16+
DuckDB secret instead of the ``ATTACH`` text::
17+
18+
CREATE SECRET (TYPE vgi_attach, SCOPE 'https://worker.example.com', api_key '...');
19+
ATTACH 'mydb' AS mydb (TYPE vgi, LOCATION 'https://worker.example.com');
1220
"""
1321

1422
from dataclasses import dataclass
@@ -42,18 +50,31 @@ class AttachOptionSpec(_SpecBase):
4250
rather than surfacing a failure that reads like an empty catalog.
4351
Mutually exclusive with ``default`` — an option that falls back to
4452
a value is by definition satisfiable without the caller.
45-
ARROW_SCHEMA: The shared four columns plus ``required``.
53+
secret: The option carries a credential (an API key, token or
54+
password). Clients and the DuckDB extension mask its value, keep it
55+
out of result-cache keys and logs, and can supply it from a
56+
``vgi_attach`` DuckDB secret
57+
(``CREATE SECRET (TYPE vgi_attach, SCOPE '<worker url>', api_key '...')``)
58+
rather than the ``ATTACH`` text. Credential options MUST set this.
59+
Compatible with ``required``. Allowed alongside a ``default``, but a
60+
secret option should normally have none: a default credential is
61+
advertised in plain text to every client at discovery.
62+
ARROW_SCHEMA: The shared four columns plus ``required`` and ``secret``.
4663
4764
"""
4865

4966
required: bool = False
67+
secret: bool = False
5068

5169
ARROW_SCHEMA: ClassVar[pa.Schema] = pa.schema(
5270
[
5371
*_SpecBase.ARROW_SCHEMA,
54-
# Nullable, and appended last: a peer that predates this column
72+
# Nullable, and appended after the shared columns: a peer that predates this column
5573
# reads the batch by name and simply doesn't see it.
5674
pa.field("required", pa.bool_(), nullable=True),
75+
# Same rules as ``required``: nullable, appended after it, and an
76+
# absent column reads as False.
77+
pa.field("secret", pa.bool_(), nullable=True),
5778
]
5879
)
5980

@@ -67,12 +88,16 @@ def __post_init__(self) -> None:
6788
)
6889

6990
def _extra_row(self) -> dict[str, Any]:
70-
return {"required": self.required}
91+
return {"required": self.required, "secret": self.secret}
7192

7293
@classmethod
7394
def _extra_kwargs(cls, row: dict[str, object]) -> dict[str, Any]:
74-
# Absent column (older peer) and explicit null both mean "not required".
75-
return {"required": bool(row.get("required") or False)}
95+
# Absent column (older peer) and explicit null both mean "not required"
96+
# / "not secret".
97+
return {
98+
"required": bool(row.get("required") or False),
99+
"secret": bool(row.get("secret") or False),
100+
}
76101

77102

78103
@dataclass
@@ -90,18 +115,26 @@ class AttachOption(_DescriptorBase):
90115
91116
class AttachOptions:
92117
region: Annotated[str, AttachOption(desc="AWS region")] = "us-east-1"
93-
api_key: Annotated[str, AttachOption(desc="API key", required=True)]
118+
api_key: Annotated[str, AttachOption(desc="API key", required=True, secret=True)]
119+
120+
Credential options (API keys, tokens, passwords) MUST be declared
121+
``secret=True``; see `AttachOptionSpec.secret`.
94122
95123
Attributes:
96124
required: The caller must supply this option at ``ATTACH`` time.
125+
secret: The option carries a credential. Clients and the DuckDB
126+
extension mask it, keep it out of cache keys and logs, and can
127+
supply it from a ``vgi_attach`` DuckDB secret. Should normally have
128+
no default.
97129
98130
"""
99131

100132
required: bool = False
133+
secret: bool = False
101134

102135
def extra_spec_kwargs(self) -> dict[str, Any]:
103-
"""Carry ``required`` through to the `AttachOptionSpec`."""
104-
return {"required": self.required}
136+
"""Carry ``required`` and ``secret`` through to the `AttachOptionSpec`."""
137+
return {"required": self.required, "secret": self.secret}
105138

106139

107140
class MissingAttachOptionsError(ValueError):

‎vgi/http/landing.html‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,8 @@ <h1 id="worker-name">—</h1>
544544
name: s.name, type: typeStr(s.type),
545545
default: s.default == null ? '' : String(s.default),
546546
description: s.description || '',
547-
required: !!s.required
547+
required: !!s.required,
548+
secret: !!s.secret
548549
}));
549550
} catch (e) { /* keep the card; drop unreadable specs */ }
550551

@@ -792,7 +793,8 @@ <h1 id="worker-name">—</h1>
792793
'<table class="opts"><thead><tr><th>Option</th><th>Type</th><th>Default</th><th>Description</th></tr></thead><tbody>'+
793794
opts.map(o=>'<tr><td class="o-name">'+esc(o.name)+
794795
(o.required?'<span class="o-req" title="Must be supplied at ATTACH time">required</span>':'')+
795-
'</td><td class="o-type">'+esc(o.type)+'</td><td class="o-def">'+esc(o.default||'—')+'</td><td class="o-desc">'+esc(o.description||'')+'</td></tr>').join('')+
796+
(o.secret?'<span class="o-req" title="A credential: supply it from a vgi_attach DuckDB secret rather than the ATTACH text">secret</span>':'')+
797+
'</td><td class="o-type">'+esc(o.type)+'</td><td class="o-def">'+esc(o.secret&&o.default?'hidden':(o.default||'—'))+'</td><td class="o-desc">'+esc(o.description||'')+'</td></tr>').join('')+
796798
'</tbody></table></details>';
797799
}
798800
function renderConnect(){

0 commit comments

Comments
 (0)