diff --git a/misc/docs/README.md b/misc/docs/README.md index 24627f5..5b145b2 100644 --- a/misc/docs/README.md +++ b/misc/docs/README.md @@ -23,6 +23,7 @@ output; don't put prose here that a build step will overwrite.) | [0009](decisions/0009-scope-and-deferrals.md) | v1 scope, deferrals, the `s3dol`/`botodol` line | proposing a feature | | [0010](decisions/0010-bucket-and-bulk-operations.md) | Bucket-existence policy, `delete_many`, cascading delete | touching bucket lifecycle or bulk ops | | [0011](decisions/0011-keyed-capability-surface.md) | **No key-taking methods; capabilities are sibling stores** | adding any capability, or wondering where `url_for` went | +| [0012](decisions/0012-credential-and-endpoint-resolution.md) | **Credential + endpoint resolution**: a pure `resolve()`, a picklable spec, two endpoint rungs | touching credentials, endpoints, presets, `anon`, or the migration warning | ## The five things most likely to bite you diff --git a/misc/docs/architecture.md b/misc/docs/architecture.md index d84338c..85ca902 100644 --- a/misc/docs/architecture.md +++ b/misc/docs/architecture.md @@ -85,28 +85,49 @@ load-bearing rather than decorative: ### Layer A — `s3dol.connection` Owns the expensive resource (the botocore client) and *all* credential/endpoint -resolution. See [decisions/0002](decisions/0002-boto3-as-engine.md) and -[decisions/0003](decisions/0003-provider-presets-and-capabilities.md). +resolution. See [decisions/0002](decisions/0002-boto3-as-engine.md), +[decisions/0003](decisions/0003-provider-presets-and-capabilities.md) and — for the full +resolution design — [decisions/0012](decisions/0012-credential-and-endpoint-resolution.md). ```python @dataclass(frozen=True) class S3Connection: - preset: str | Preset | None = None # 'aws' | 'minio' | 'r2' | ... - profile_name: str | None = None + # WHERE endpoint_url: str | None = None region_name: str | None = None - credentials: Credentials | None = None # explicit; None => resolve the chain - anon: bool | Literal["auto"] = False - client_config: dict = field(default_factory=dict) + # WHO + profile: str | None = None + credentials: CredentialProvider | None = None # normalised in __post_init__ + anon: bool = False # 'auto' deferred to v1.x — ADR-0012 §D5 + # HOW ("None" means do not override; each is a ladder — ADR-0012 §D3) + signature_version: str = "s3v4" + addressing_style: Literal["path", "virtual"] | None = None + checksum: Literal["when_supported", "when_required"] | None = None + payload_signing_enabled: bool | None = None + verify: bool | str | None = None + client_kwargs: tuple[tuple[str, Any], ...] = () + # POLICY + deny_means_absent: bool = False # ADR-0004 §3 ``` -Three properties matter and are each tested: - -- **Lazy.** The client is a `cached_property`; constructing a connection performs no I/O and - never raises for missing credentials. -- **Picklable.** The connection carries a *spec*, not a client. `__getstate__` drops the - cached client so stores survive `ProcessPoolExecutor` and Dask. -- **Redacting.** No secret appears in `repr`, `str`, or any surviving local. +`preset` is an argument to the factory, not a field: it resolves to concrete values in +`resolve()`, so the spec records *what was resolved and from where*, not *what to look up later*. + +Four properties matter and are each tested: + +- **Lazy, and built under a lock.** Constructing a connection performs no I/O and never raises + for missing credentials. The client is **not** a bare `cached_property` — measured, 8 + concurrent threads produce 8 clients, which for a callable credential is 8 token fetches + ([ADR-0012](decisions/0012-credential-and-endpoint-resolution.md) §D1). +- **Picklable, always.** The connection carries a *spec*, never a live object — there is no + `connection=` escape hatch. `__getstate__`/`__setstate__` are mandatory for + *every* connection, because the boto3 client is unpicklable regardless of `anon`. +- **Redacting.** No secret appears in `repr`, `str`, or any surviving local. Note `repr()` of a + botocore provider chain dumps the entire `os.environ`, and a `str` subclass is not sufficient + redaction — it leaks through `json`, concatenation and `join`. +- **Purely resolvable.** `resolve(spec, environ, aws_config) -> Resolution` does no I/O and + carries per-field provenance, so the whole ladder is exhaustively testable with a fake environ + dict and `s3dol.diagnose()` can print where every value came from. ### Layer B — `s3dol.base` diff --git a/misc/docs/decisions/0003-provider-presets-and-capabilities.md b/misc/docs/decisions/0003-provider-presets-and-capabilities.md index f6af985..0409e5e 100644 --- a/misc/docs/decisions/0003-provider-presets-and-capabilities.md +++ b/misc/docs/decisions/0003-provider-presets-and-capabilities.md @@ -150,6 +150,16 @@ Two caveats we must not forget: ### 4. `signature_version` is never left to botocore — this is the single most load-bearing default in the package +> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md).** The mandate is right; +> its stated *scope and cause* are wrong. Measured across 16 regions on botocore 1.43.68: the +> presign downgrade fires in exactly the regions `endpoints.json` still lists as v2-capable — +> **12 region strings**, not 8, counting `aws-global` and three FIPS regions. **A custom endpoint +> is irrelevant**: `us-east-1` + MinIO → SigV2, `eu-south-2` + MinIO → SigV4. *Region* is the +> discriminator. The practical impact stands only because `us-east-1` is the conventional MinIO +> default. Pin this as a conformance test **against `endpoints.json`**, not against a literal +> region list — and note a user's own AWS-config profile setting `s3.signature_version = s3v4` +> already suppresses the downgrade, so `diagnose()` must report the source. + botocore silently **downgrades presigning to SigV2** whenever the user has not *explicitly* set `signature_version` (`botocore/client.py::_set_s3_presign_signature_version`). Measured: @@ -177,6 +187,16 @@ This is a live bug in v0, not merely a design gap: `s3dol` today presigns with S ### 5. Anonymous access +> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D5, in two places.** +> (1) **`url_for` returns `None` when anonymous — it does not raise.** +> `dol.SupportsUrlFor.url_for` is typed `-> Optional[str]`, and raising turns `lacing`'s +> documented streaming fallback into a 500 on a *public* bucket, the one case `anon` exists for. +> ADR-0011 D3b's unreachable-wrapper guard still raises; the two refusal conditions must not be +> merged. (2) **The pickle rationale is incomplete.** Keeping `UNSIGNED` out of the dataclass is +> necessary but not sufficient — the boto3 client is unpicklable full stop, so +> `__getstate__`/`__setstate__` are mandatory for *every* connection, not only anonymous ones. +> Also: `anon='auto'` is **deferred to v1.x**; v1 ships `anon: bool`. + `anon` is `bool | 'auto'` on the connection **and surfaced directly on `s3_store(...)`** — reading a public bucket must not require learning Layer A. It is translated to `botocore.UNSIGNED` *inside* the `cached_property` that builds the client: the singleton is diff --git a/misc/docs/decisions/0004-error-taxonomy.md b/misc/docs/decisions/0004-error-taxonomy.md index cf50613..bdeb6e2 100644 --- a/misc/docs/decisions/0004-error-taxonomy.md +++ b/misc/docs/decisions/0004-error-taxonomy.md @@ -77,6 +77,17 @@ produce**. ### 3. The taxonomy +> **Extended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D1** with the +> configuration/spec-time branch, which resolves before any request is made and therefore has no +> `(operation, code, status)` row: `ConfigurationError(S3Error, ValueError)` with +> `PresetConflict`, `PresetHostMismatch`, `MissingEndpoint`, `MissingPresetParam`, +> `UnknownPresetParam`, `InvalidEndpoint`. Plus a warning tree, `S3DolWarning(UserWarning)` with +> `S3DolResolutionChanged`, `AmbiguousResolution`, `AnonymousFallback` — +> `filterwarnings('error', category=S3DolWarning)` is the strict mode, so there is no `strict=` +> kwarg. Two bans belong here too: `anon=True` with credentials or a profile, and `anon=True` +> with `deny_means_absent=True` (measured under moto, anonymous HEAD gives 403 on a present key +> and 404 on an absent one, so every key reads as absent — a silently empty store). + | Condition | Raises | Rationale | |---|---|---| | object absent | `ObjectNotFound(KeyError)` | Mapping contract | diff --git a/misc/docs/decisions/0007-naming-and-compatibility.md b/misc/docs/decisions/0007-naming-and-compatibility.md index b462b5c..f9230f3 100644 --- a/misc/docs/decisions/0007-naming-and-compatibility.md +++ b/misc/docs/decisions/0007-naming-and-compatibility.md @@ -71,6 +71,11 @@ what v0 lacked was a statement of which were *public*.) ### 3. `s3dol.store.S3Store` becomes a deprecated shim, removed in v2 +> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D7:** the shim signature +> below **omits `is_supabase_endpoint=`, which v0 accepts** (`s3dol/store.py:26`). As written it +> is a `TypeError` on upgrade for anyone passing it. Add it, map it to the supabase preset, and +> preserve `False` as an opt-out. + It keeps its current signature exactly — `bucket_name` accepted **both positionally and by keyword**, `path=` (not renamed), plus `aws_access_key_id`, `aws_secret_access_key`, `aws_session_token`, `endpoint_url`, `region_name`, `profile_name`, `make_bucket`, @@ -99,6 +104,18 @@ hard-coded endpoint will silently redirect on upgrade. That is why §5 has a ste ### 4. Behaviour changes that the shim deliberately does NOT preserve +> **Corrected by [ADR-0012](0012-credential-and-endpoint-resolution.md).** The ladder below is +> kept, but the sentence "v0's effective precedence is `AWS_ENDPOINT_URL_S3 > … > explicit +> kwarg`, and the corrected ladder **inverts the top of it**" describes a **deletion, not a +> ranking**. Measured: botocore already ranks `explicit > AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL +> > config file > resolver`. v0 behaves otherwise because it *drops* the argument +> (`_find_default_credentials` never forwards `endpoint_url`). **v1 re-ranks nothing; it stops +> discarding an argument** — which deletes the "vendor a precedence ladder" work item entirely. +> Two further v0 behaviour changes are missing from the table below: v0 silently **nulls an +> explicitly-passed `aws_session_token`** and silently **changes an explicitly-passed +> `region_name`**. And `http_cosmo_prep` moves on **three axes at once** (endpoint honoured, +> credential identity, checksum policy) — one row, one release-note bullet, not three. + These are bug fixes, and preserving them would mean preserving data-misrouting: | v0 behaviour | v1 | shim keeps v0? | diff --git a/misc/docs/decisions/0008-testing-architecture.md b/misc/docs/decisions/0008-testing-architecture.md index ef44b1d..cd7524d 100644 --- a/misc/docs/decisions/0008-testing-architecture.md +++ b/misc/docs/decisions/0008-testing-architecture.md @@ -54,6 +54,17 @@ own stores. ### What the conformance suite must assert +> **Extended by [ADR-0012](0012-credential-and-endpoint-resolution.md).** The pickle conformance +> test must run **after** touching `.client` (the cache is what breaks it) and must **also assert +> `hash(conn)`** — measured, a container-valued field pickles happily but raises `TypeError` on +> `hash`, so the pickle test alone misses it. Add to the moto-fidelity list: **moto lets an +> anonymous client `list_objects_v2` and `put_object` against a private bucket**, so no tier +> below 4 can validate anonymous semantics. Any test of credential fallback must scrub CI's +> `AWS_*` variables. And presign assertions must check `'X-Amz-Algorithm'` in the URL across +> **both** a v2-legacy region and a v4-only region — `client.meta.config.signature_version` +> reports `'s3v4'` while emitting SigV2, and a `us-east-1`-only test cannot distinguish "we +> forced s3v4" from "this region defaults to v4 anyway". + Beyond the obvious Mapping laws, these are the ones that would have caught real bugs: 1. **Prefix scoping round-trip.** Fixture must include sibling, non-matching, marker and diff --git a/misc/docs/decisions/0011-keyed-capability-surface.md b/misc/docs/decisions/0011-keyed-capability-surface.md index 56f90c9..813ab4c 100644 --- a/misc/docs/decisions/0011-keyed-capability-surface.md +++ b/misc/docs/decisions/0011-keyed-capability-surface.md @@ -247,6 +247,11 @@ argument, where a method could not be called on a store lacking it. Free-functio ### D5 — A conformance test, not a registry +> **Amended by [ADR-0012](0012-credential-and-endpoint-resolution.md) §D5:** the allowlisted +> `url_for` returns `str | None`, where `None` means "no presign capability" (an anonymous +> connection). Its *other* refusal — the unreachable-wrapper guard in D3b — still **raises**. +> Two distinct conditions; do not merge them. + No `_key_methods` registry. A **reflective conformance test** enumerates the public methods of every Layer B class and fails on any that takes a key — allowlist: `url_for` (D3b), and nothing else. diff --git a/misc/docs/decisions/0012-credential-and-endpoint-resolution.md b/misc/docs/decisions/0012-credential-and-endpoint-resolution.md new file mode 100644 index 0000000..4cc8c5b --- /dev/null +++ b/misc/docs/decisions/0012-credential-and-endpoint-resolution.md @@ -0,0 +1,347 @@ +# ADR-0012: Credential and endpoint resolution + +- **Status:** Accepted +- **Date:** 2026-08-11 +- **Discussion:** [#15](https://github.com/i2mint/s3dol/discussions/15) +- **Amends:** [ADR-0003](0003-provider-presets-and-capabilities.md) §1/§4/§5/§6, + [ADR-0004](0004-error-taxonomy.md) §3, [ADR-0007](0007-naming-and-compatibility.md) §3/§4/§5, + [ADR-0008](0008-testing-architecture.md), [ADR-0011](0011-keyed-capability-surface.md) §D5 +- **Fixes:** findings 1, 2, 5 of [state-of-play](../state-of-play.md) §4; issue + [#10](https://github.com/i2mint/s3dol/issues/10) + +Everything here is either measured (botocore **1.43.68**, offline, network-blocked) or traceable +to an Accepted ADR. Measurements were produced by four independent probes and then re-verified +by an adversarial pass that overturned several of them; where a claim in #15 or an earlier ADR +turned out to be wrong, this ADR says so rather than quietly correcting it. + +## The two sentences the rest follows from + +1. **A preset is the caller's explicit endpoint, spelled by name.** It sits exactly where + `endpoint_url=` sits — above every environment and config-file channel. s3dol owns **two** + rungs of the endpoint ladder, not six. +2. **A `Config` value s3dol does not set is a channel it leaves open to the user.** So every + field is a *ladder with a documented bottom*, and "the row makes no claim" is spelled by + **omitting the key**, never by passing `None`. + +## Context — three premises in #15 that did not survive measurement + +**(a) "The obvious fix inverts v0's precedence."** #15 (and [ADR-0007](0007-naming-and-compatibility.md) §4) +say v0's effective order is `AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL > explicit kwarg`, so +"explicit wins" inverts the top of it. Measured: **botocore already ranks +`explicit > AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL > config file > resolver`.** v0 behaves +otherwise because it *drops the argument* — `_find_default_credentials(endpoint_url=…)` calls +`get_client("environment variables", **session_kwargs)` and never forwards it. **v1 re-ranks +nothing; it stops discarding an argument.** That deletes the "vendor a precedence ladder" work +item entirely. + +**(b) "SigV2 for `us-east-1` and every custom endpoint."** Measured across 16 regions: the +downgrade fires in exactly the regions `endpoints.json` still lists as v2-capable — **12 region +strings**, not 8, once `aws-global` and three FIPS regions are counted. A custom endpoint is +irrelevant: `us-east-1` + MinIO → SigV2, `eu-south-2` + MinIO → SigV4. **Region is the +discriminator.** The practical impact stands only because `us-east-1` is the conventional MinIO +default. `client.meta.config.signature_version` reports `'s3v4'` throughout — it lies. + +**(c) "`anon='auto'` needs a cheap credential probe."** Measured: a `None` credential result is +**not cached**, so probe-then-build-signed runs the whole chain twice (~4.2 s where IMDS traffic +is dropped rather than refused). An UNSIGNED client short-circuits the chain entirely — zero +credential I/O. + +## Decision + +### D1 — `S3Connection` is a frozen, picklable spec. No live objects. + +```python +@dataclass(frozen=True) +class S3Connection: + # WHERE + endpoint_url: str | None = None + region_name: str | None = None + # WHO + profile: str | None = None + credentials: CredentialProvider | None = None # normalised in __post_init__ + anon: bool = False # see D5 + # HOW (None == "do not override" — see D3's ladders) + signature_version: str = 's3v4' # ADR-0003 §4: never left to botocore + addressing_style: Literal['path', 'virtual'] | None = None + checksum: Literal['when_supported', 'when_required'] | None = None + payload_signing_enabled: bool | None = None + verify: bool | str | None = None # self-signed MinIO / corporate CA + client_kwargs: tuple[tuple[str, Any], ...] = () + # POLICY + deny_means_absent: bool = False # ADR-0004 §3 +``` + +**There is no `_live` escape hatch and no `connection=`.** Checked: no dependent +passes one — `lacing` uses `S3Store(bucket, path=prefix, **s3_kwargs)`, `reelee` reaches s3dol +*through* `lacing`. A bespoke session is expressed as +`credentials=` + `profile=` + `client_kwargs=`. + +That buys a flat, unconditional guarantee: **every s3dol store pickles**, which +[ADR-0008](0008-testing-architecture.md) §6 can assert without a special case. The alternative +was two permanent classes of connection — one picklable, one raising `TypeError` and unable to +cross a `ProcessPoolExecutor`, Dask, or a `dol` disk cache. It also deletes a measured hazard: +a live client held in a `field(compare=False)` makes two connections to **different accounts** +compare and hash equal, so an `lru_cache` hands back the wrong account's client. + +Four properties that are not optional: + +1. **`__post_init__` is the only door.** Every normalisation and contradiction check happens on + the public dataclass, not in a `from_anything` classmethod — otherwise + `S3Connection(credentials=('AK', 'sec'))` dies later inside botocore with + `AttributeError: 'tuple' object has no attribute 'METHOD'`. +2. **The client is built under a per-instance lock, not a bare `cached_property`.** Measured: + 8 barrier-synchronised threads → **8 body executions and 8 distinct surviving clients**, every + trial. CPython 3.12 removed `cached_property`'s lock deliberately. For a callable credential + that is 8 token fetches; for `credential_process`, 8 subprocess spawns. +3. **`__getstate__`/`__setstate__` are mandatory for every connection**, not only anonymous ones + — the boto3 client is unpicklable, full stop. Enumerate `dataclasses.fields` (drops all caches + by construction) and re-create the lock in `__setstate__`. *This corrects + [ADR-0003](0003-provider-presets-and-capabilities.md) §5, which attributes the pickle problem + to `UNSIGNED` alone.* +4. **Secrets never in `repr`.** Measured hazard: `repr()` of a botocore provider chain dumps the + entire `os.environ`. A `str` subclass is **not** sufficient redaction — it holds for display + and leaks through `json`, concatenation and `join`. + +One error tree and one warning tree, rooted in ADR-0004's `S3Error`: + +``` +S3Error S3DolWarning(UserWarning) +└── ConfigurationError(.., ValueError) ├── S3DolResolutionChanged (D7; dies with the shim) + ├── PresetConflict ├── AmbiguousResolution + ├── PresetHostMismatch └── AnonymousFallback + ├── MissingEndpoint / MissingPresetParam / UnknownPresetParam + └── InvalidEndpoint +``` + +**There is no `strict=` kwarg.** `filterwarnings('error', category=s3dol.errors.S3DolWarning)` +is strict mode with zero API surface, escalates per-subcategory and per-module, and works in +pytest's ini. + +### D2 — `resolve(spec, environ, aws_config) -> Resolution` is pure + +No `os.environ`, no disk, no network. `aws_config` is a pre-loaded, **credential-scrubbed** +profile dict from a separate, named impure loader passed as a **thunk** — `botocore.session.Session()` +costs 16 ms and must never sit on the common path. + +`Resolution` carries a `Sourced(value, source)` per field, the `environ` it was computed +against, the chosen `preset` and how it was chosen, `capabilities`, and a credential +**provenance label** — never a credential value. Tier-1 tests cover the whole ladder with a fake +environ dict and zero I/O. + +### D3 — The endpoint ladder, and the field ladders below it + +``` +1 endpoint_url= kwarg +2 the bound preset's endpoint [no rung if a required param is unbound — C1] + ---- s3dol hands botocore ONE value (or None) and stops ---- + AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL + > aws-config [services X] s3.endpoint_url > aws-config [profile P] endpoint_url + > the SDK's own resolver +``` + +**Never pass `Config(ignore_configured_endpoint_urls=True)`.** Measured: with an explicit +endpoint it changes nothing; with no explicit endpoint it retargets an `AWS_ENDPOINT_URL_S3` +user to `https://s3.amazonaws.com`. **It is a no-op where it is safe and harmful where it is +not.** Two independent drafts of this design passed it unconditionally; that would have been a +silent data-target move of exactly the kind this ADR exists to prevent. + +s3dol *reads* the lower rungs — purely, via +`botocore.configprovider.ConfiguredEndpointProvider(full_config=…, scoped_config=…, client_name='s3', environ=…)`, +verified constructible from plain dicts with zero network — for `diagnose()` and the guards +**only**, never to override. + +Two guards: + +- **C1** — a row whose endpoint template has an unbound *required* param contributes no rung. If + nothing below supplies an endpoint and the row is `requires_endpoint`, raise `MissingEndpoint` + naming both the missing params and `AWS_ENDPOINT_URL_S3`. `preset='minio'` silently becoming + `https://s3.amazonaws.com` is structurally impossible. +- **C2** — a *named* row with `host_patterns` whose endpoint arrived from below s3dol's two rungs + and matches no pattern: **warn**; if it matches a *different* row's patterns, **raise** + `PresetHostMismatch`. Matching nothing is genuinely ambiguous (a proxy, a vanity CNAME and a + self-hosted gateway all look like this) and hard-failing punishes a legitimate deployment; + matching another provider is unambiguous. Anyone wanting hard-fail everywhere gets it from + `filterwarnings`. The documented escape is `endpoint_url=`, which is rung 1 and never triggers + C2. + +**Per-field ladders**, each bottoming out in *omit the key*: + +| field | ladder | +|---|---| +| `region_name` | kwarg (**binds** the row's `{region_name}` template) > row literal (`PresetConflict` iff pinned) > region captured from the resolved host > `AWS_DEFAULT_REGION` > aws-config profile > `'us-east-1'` | +| `addressing_style` | kwarg > row > profile `s3.addressing_style` > **omit** | +| `checksum` | kwarg > row > `AWS_{REQUEST,RESPONSE}_CHECKSUM_*` > `'when_supported'` | +| `payload_signing_enabled` | kwarg > row > **omit** (no env channel exists) | +| `signature_version` | kwarg > `'s3v4'` (no env channel; never from a row) | + +Three measurements force these shapes: + +- **Omitting is not the same as `None`.** `Config(s3={'addressing_style': None})` *overrides* a + profile's value with `None`; omitting the key preserves it. Config's `s3` dict merges per-key. +- **The checksum must ladder, not be forced.** `Config(request_checksum_calculation=…)` beats env + in both directions, **and omitting the key lets env through**. So "always set `when_supported` + unless a preset says otherwise" would silently break the user who sets + `AWS_REQUEST_CHECKSUM_CALCULATION=when_required` to work around Supabase — the exact + population ADR-0003 §3 exists for. +- **botocore does not read `AWS_REGION`**, only `AWS_DEFAULT_REGION`. v1 must not "helpfully" add + it. And `addressing_style` has **no env var at all**; its default is endpoint-dependent (path + for any configured endpoint, virtual for the SDK-resolved one). + +### D4 — Presets are provider facts, resolved in two passes + +**Pass 1** resolves the endpoint. **Pass 2** picks the row: *named* > *detected* by +`host_patterns` against the resolved endpoint (ambiguity raises) > `generic-s3` if any endpoint +resolved > `aws` if none did. Not circular: with no preset named there is no preset rung; with +one named, pass 2 is already determined. + +**Detection supplies the full row** — addressing style, checksum, payload-signing and +capabilities — so a bare R2/Backblaze/Supabase URL works with no new argument, and ADR-0003 §3's +checksum fix reaches people who have never heard of presets. The cost is one announced +path→virtual flip for an endpoint-passing caller whose URL matches a virtual-style row. All +three known endpoint-passing dependents resolve to `generic-s3`, which is byte-identical to +today; ADR-0007 §5 step 0 exists to confirm that before shipping. + +`Preset` gains `params`, `host_patterns`, `requires_endpoint`, `soft`, `config_kwargs`, and a +**derived** `pinned`; `signature_version` and `client_kwargs` **leave** `Preset` for +`S3Connection`. `addressing_style` has **no default** so every row must state it, and `'auto'` is +illegal. + +**No credential slot, and the exclusion is structural** — not a check. `proxies`, +`proxies_config` and `client_context_params` are off the `config_kwargs` allowlist and values are +scalars-only, which closes `config_kwargs['proxies'] = {'https': 'http://user:pw@…'}`. The +residual carrier is a URL field, checked at registration and on the resolved endpoint: **no `@` +in netloc, no query, no fragment**. Measured: a query-string endpoint is *accepted* by botocore +and retained verbatim in `client.meta.endpoint_url`, so it would leak into `diagnose()` and +tracebacks. + +**`pinned` is derived and gated on `verified`.** Every builtin row is doc-sourced today, so +`PresetConflict` is dormant in v1.0 and pinning becomes a ratchet that turns on with evidence. + +### D5 — `credentials=`, `profile=`, and `anon: bool` + +**Accepted:** `None` (botocore's chain) · a bare `str` (**always** a profile name) · +`(key, secret[, token])` · a mapping of `aws_*` keys · a `CredentialProvider` instance · a +**picklable** zero-arg callable. +**Rejected, each with its own message:** a `CredentialProvider` *class*; a resolved +`Credentials`/`RefreshableCredentials` (a snapshot cannot refresh and cannot pickle); a live +boto3 object; an unknown mapping key; an unpicklable callable. + +**Mechanism:** normalise to a picklable frozen-dataclass `CredentialProvider` and inject by +**replacing** the component — +`bs.register_component('credential_provider', CredentialResolver([provider]))`. Never +`insert_before('env', …)` (raises `UnknownCredentialError` whenever a profile is set) and never +prepend (silent chain fallthrough). + +Measured facts that force this: + +- **Nothing live pickles** — not `boto3.Session`, not `botocore.session.Session`, not clients, + not refreshable credentials, not the real chain's providers. #15's "a `botocore.Session`" is + therefore impossible. A frozen dataclass holding **only a profile name** round-trips and + rebuilds live, still-refreshable credentials. +- **`profile_name` plus *any* explicit credential kwarg silently discards the profile's + credentials** — no error, no warning; a token-only kwarg leaves `Credentials(None, None, 'TOK')`. + So `profile=` and `credentials=` cannot be naively independent parameters. +- **Reading `.access_key` on a refreshable credential triggers a network refresh.** `diagnose()` + must never touch it. +- **Env credentials are not always static**: with `AWS_CREDENTIAL_EXPIRATION` set, `EnvProvider` + returns `RefreshableCredentials`. +- Refresh thresholds are advisory **900 s** / mandatory **600 s**, so a snapshot dies ~10 minutes + before its nominal expiry. + +**`profile=` stays its own parameter** because it selects *config*, not only credentials — and +because `addressing_style` defaults to `None` and `ignore_configured_endpoint_urls` is never +passed, four of five profile-supplied settings survive. + +**`anon` is `bool` in v1. `'auto'` is deferred to v1.x.** Widening `bool → bool | 'auto'` later +is fully compatible, and nothing in the ecosystem uses it today. The reason to defer is the shape +of the evidence: across three review rounds the guard count went **4 → 6 → 7**, each round +finding another silent-credential-downgrade path — the signal a feature whose *purpose* is to +guess emits. `'auto'` is the only path on which s3dol can silently read the **public view** of +data the caller believed was authenticated; every other credential path either resolves or fails +loudly. The honest use case is served by `anon=bool(os.getenv(...))` today, and the decision can +be re-taken on real `diagnose()` telemetry. + +`anon=True` still ships, and: + +- `anon=True` + credentials/profile **raises** at construction. Measured: `UNSIGNED` sets + `_request_signer._credentials` to `None`, so they would be silently discarded. +- `anon=True` + `deny_means_absent=True` **raises**. Measured under moto: an anonymous + `head_object` gives 403 on an existing key and 404 on a missing one, so **every key reads as + absent** — a silently empty store, banned by ADR-0001 goal 4. +- **`url_for` returns `None` when anonymous** — *amends ADR-0003 §5, which says it raises*. + `dol.SupportsUrlFor.url_for` is typed `-> Optional[str]`, and raising turns `lacing`'s + documented streaming fallback into a 500 on a **public** bucket, the one case `anon` exists + for. Two refusal conditions coexist and must not be merged: ADR-0011 D3b's unreachable-wrapper + guard still **raises** (wrong-key risk); anonymity returns `None` (no capability). The guard + reads `client.meta.config.signature_version is UNSIGNED`, never `spec.anon`. +- Anonymous **writes** refuse locally in `__setitem__`/`__delitem__`/`create_bucket` — botocore + offers nothing here, and moto even accepts an unsigned PUT. +- `UNSIGNED` never enters the dataclass, and — measured — a bare `Config` holding it is + unpicklable too, so it is constructed only at client-build time. + +### D6 — `diagnose()` ships first, and never raises + +Non-negotiable: it **never raises** (a failed resolution is a *row*; it exists to report exactly +the misconfigurations that raise); it holds **no credential value**, only a provenance label; it +never reads `.access_key`; credential *identity* is compared by access-key prefix only. + +It prints both resolution passes, every resolved field with its source, the botocore version, the +`anon` state and `can_presign`, and the **v0-vs-v1 divergence table** — which is what +ADR-0007 §5 step 0 is for. + +### D7 — The migration warning is a differential, not a frozen simulation + +Compute it as **one v0 argument-mangler run through v1's own resolver twice**: +`v0_mangle(args, environ) -> (branch, args')`, then `compare(resolve(args'), resolve(args))`. +Both sides go through the same resolver, so imprecision cancels and the whole class of +value-gap false positives disappears. `v0_mangle` is ~20 frozen lines mapping 1:1 onto v0, and it +dies in the same commit as the shim. This replaces a frozen ~60-line copy of v0's resolution, +which an earlier draft proposed and which could rot undetected. + +Facts that shape it: + +- **The gate is the `is_supabase_endpoint` parameter, not a `'.supabase.'` substring** — and + ADR-0007 §3's shim signature **omits `is_supabase_endpoint=` although v0 accepts it** + (`s3dol/store.py:26`), which is a `TypeError` on upgrade for anyone passing it. +- **Two v0 branches are not functions of their arguments** — the supabase and skip paths call + module-level `boto3.client(...)`, resolving through the process-global cached `DEFAULT_SESSION`. + Excluded from the golden table, with the reason in the file, and given its own release-note + line: *a long-lived process that builds one Supabase store pins its credentials for the life of + the interpreter.* +- **Credentials are compared by a boolean predicate, never by snapshot.** v0 also silently nulls + an explicitly-passed `aws_session_token` and silently changes an explicitly-passed + `region_name` — two behaviour changes neither #15 nor ADR-0007 records. +- **Cost:** two `os.environ` lookups (~2 µs) in the common case. Never call + `session.get_credentials()` in the check. + +**Who moves:** `http_cosmo_prep` (not ours — one row, three axes: endpoint honoured, credential +identity moves, checksum may change) · `lacing` (verbatim `**s3_kwargs` forwarding) · `reelee` +**transitively through `lacing`** — the pair moves together · `py2store` unaffected. + +## Consequences + +**Buys.** The endpoint fix is a deletion, not a re-ranking, so there is no vendored ladder and no +conformance test guarding it. Every store pickles, unconditionally. Resolution is a pure function +with per-field provenance, exhaustively testable with a fake environ and no I/O. The one path +that could silently read a public view is deferred out of v1. + +**Costs.** + +- Users with a bespoke `boto3.Session` must re-express it as a `CredentialProvider`. Nobody known + does this, but `BaseS3BucketReader.client` is public in v0, so an unmeasured population may. +- Detection supplying the full row means one announced path→virtual flip is possible for an + endpoint-passing caller. Gated on step-0 telemetry. +- A preset author must now state `addressing_style` explicitly and think about `pinned`/`soft`. +- The `S3DolWarning` category must not cry wolf: if it fires spuriously, users filter the whole + category and silence the migration warning with it. + +**What NOT to do.** + +1. Do not pass `Config(ignore_configured_endpoint_urls=True)`. Re-read D3. +2. Do not add a `strict=` kwarg. Use `filterwarnings`. +3. Do not store a `Credentials` snapshot, a `Session`, or a live client anywhere in the spec. +4. Do not read `.access_key` in `diagnose()` — it performs network I/O. +5. Do not spell "no claim" as `None` in a `Config` sub-dict; omit the key. +6. Do not re-add `AWS_REGION` support. botocore does not read it. +7. Do not assert on `client.meta.config.signature_version` — it reports `'s3v4'` while emitting + SigV2. Assert on `'X-Amz-Algorithm'` in the URL. diff --git a/misc/docs/state-of-play.md b/misc/docs/state-of-play.md index bf0cbd6..5f6993b 100644 --- a/misc/docs/state-of-play.md +++ b/misc/docs/state-of-play.md @@ -64,12 +64,12 @@ consumer's requirements. | Live bug found | **#10** — `url_for` emits SigV2 | | Answered | **#5** (multipart / value types), **discussion #6** (extendable KeyError) | | Upstream blockers | **i2mint/dol#82** (prefix corruption), **i2mint/dol#83** (delegation with unmapped key), plus `inner_most_key` export + `content_url` resolution (ADR-0006 §3) | -| Resolved design question | **§7** / discussion **#14** → [ADR-0011](decisions/0011-keyed-capability-surface.md) | -| Open design question | **§8** / discussion **#15** — credential and endpoint resolution | +| Resolved design questions | **§7** / **#14** → [ADR-0011](decisions/0011-keyed-capability-surface.md) · **§8** / **#15** → [ADR-0012](decisions/0012-credential-and-endpoint-resolution.md) | +| Open design questions | none — implementation is unblocked; see §11 | **Nothing has been implemented.** The repo still ships v0.1.9 unchanged. -## 3. The eleven ADRs, in one paragraph each +## 3. The twelve ADRs, in one paragraph each **[0001 — Four-layer architecture](decisions/0001-layered-architecture.md).** Three layers (`connection` → `base` → `recipes`), mirroring `azuredol` so one adapter in the family reads @@ -388,7 +388,11 @@ now a blocking upstream item ([ADR-0006](decisions/0006-key-scoping-and-dol-fixe (dol#18 Approach C, committed for 0.4/1.0) — which #14's option list did not contain, and which is why the selection criterion became *correct on 0.3.x AND redundant under is-a*. -## 8. Open question — credential and endpoint resolution +## 8. ~~Open question~~ RESOLVED — credential and endpoint resolution + +> **Resolved 2026-08-11 in [ADR-0012](decisions/0012-credential-and-endpoint-resolution.md)** +> (discussion #15). The framing below stands as the problem statement, with **three premises +> corrected** — see §8-resolution at the end of the section. ### The problem @@ -462,6 +466,35 @@ requires learning Layer A. And `'auto'` needs a precise meaning — proposed: "t credentials resolve **at all**", explicitly *not* "retry unsigned after `AccessDenied`", because an expired token would then silently downgrade to a different, public view of the data. +### §8-resolution — what was decided, and the three corrected premises + +Full record: [ADR-0012](decisions/0012-credential-and-endpoint-resolution.md). + +**Decided.** `S3Connection` is a frozen, picklable spec with **no live-object escape hatch**. +`resolve(spec, environ, aws_config)` is pure. s3dol owns **two** endpoint rungs (explicit kwarg, +then the bound preset) and hands botocore one value; every other field is a ladder bottoming out +in *omit the key*. Presets resolve in two passes and detection supplies the full row. `anon` is +`bool` in v1 — **`'auto'` is deferred to v1.x**. The migration warning is a differential through +v1's own resolver, not a frozen copy of v0. No `strict=` kwarg. + +**Correction 1 — "the obvious fix inverts v0's precedence" is wrong.** botocore already ranks +`explicit > AWS_ENDPOINT_URL_S3 > AWS_ENDPOINT_URL > config file > resolver`. v0 behaves +otherwise because it *drops* the argument. **v1 re-ranks nothing**, which deletes the +"vendor a precedence ladder" work item. + +**Correction 2 — the SigV2 scope in #10 and ADR-0003 §4 is wrong.** It is region-gated (12 +v2-capable region strings), not universal, and **not** caused by a custom endpoint. + +**Correction 3 — `Config(ignore_configured_endpoint_urls=True)` must never be passed.** Measured: +a no-op when an explicit endpoint is set, and it retargets an `AWS_ENDPOINT_URL_S3` user to AWS +when one is not. Two independent design drafts passed it unconditionally. + +Also newly measured and load-bearing: **nothing live pickles** (so #15's "a `botocore.Session`" +is impossible); **`profile_name` + any credential kwarg silently discards the profile's +credentials**; **reading `.access_key` triggers a network refresh**, so `diagnose()` must never +touch it; **`repr()` of a botocore provider chain dumps the whole `os.environ`**; and **botocore +does not read `AWS_REGION`**. + ## 9. Re-verification snippets The raw research (13 agents, ~1.4M tokens) was local-only and is **not** recoverable. Every diff --git a/pyproject.toml b/pyproject.toml index 7411ddb..28f3fa5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,7 +32,13 @@ classifiers = [ dependencies = [ "dol", "boto3", - "botocore", + # ADR-0012: the endpoint env vars (AWS_ENDPOINT_URL_S3 / AWS_ENDPOINT_URL) do not exist + # before botocore 1.31, and ADR-0003 §3's checksum knobs + # (request_checksum_calculation / response_checksum_validation) arrive in 1.36. s3dol sets + # them on every client, so an older botocore silently ignores them. + "botocore>=1.36", + # ADR-0003 §3: the aws-chunked framing fix on the transfer path. + "s3transfer>=0.11.2", ] [project.urls]