Skip to content

Commit 643ec62

Browse files
committed
docs: fold v3 design notes into the docs PR
Consolidates the documentation-only changes into a single PR so they can be reviewed together and thinned out in one place: adds docs/design/signedby-tag-resolution.md and docs/design/config-credentials-coupling.md, previously proposed separately. The config-credentials note is the version that describes the design as a proposal. A later revision on the v3 working branch marks it implemented, but the shared configfile package it refers to has not landed on main yet, so that wording would be inaccurate here. Signed-off-by: Terry Howe <terrylhowe@gmail.com>
1 parent 9b2d5cd commit 643ec62

2 files changed

Lines changed: 266 additions & 0 deletions

File tree

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
# Design: decoupling `credentials` from `config` without a global
2+
3+
Status: **proposal** — recommend settling before the v3 API freeze.
4+
5+
## Problem
6+
7+
`credentials.NewStore` depends on a package-level mutable variable that a
8+
*different* package populates as an import side effect.
9+
10+
`registry/remote/credentials/store.go`:
11+
12+
```go
13+
// defaultConfigLoader is set by the config package during init.
14+
var defaultConfigLoader ConfigFileLoader
15+
16+
var ErrNoConfigLoader = fmt.Errorf("no config loader registered; import the config package or use NewStoreFromConfig")
17+
18+
func SetDefaultConfigLoader(loader ConfigFileLoader) {
19+
defaultConfigLoader = loader
20+
}
21+
22+
func NewStore(configPath string, opts StoreOptions) (*DynamicStore, error) {
23+
if defaultConfigLoader == nil {
24+
return nil, ErrNoConfigLoader
25+
}
26+
// ...
27+
}
28+
```
29+
30+
`registry/remote/config/config.go`:
31+
32+
```go
33+
func init() {
34+
credentials.SetDefaultConfigLoader(func(configPath string) (credentials.ConfigFile, error) { /* ... */ })
35+
}
36+
```
37+
38+
Three consequences:
39+
40+
1. **Action at a distance.** Whether `credentials.NewStore` works depends on
41+
whether some *other* package was linked in. A caller who imports only
42+
`credentials` gets `ErrNoConfigLoader` at runtime, and the fix is to add an
43+
import they do not otherwise use — the kind of thing that gets deleted by a
44+
later "remove unused import" cleanup and fails in production.
45+
2. **Unsynchronized global.** `SetDefaultConfigLoader` writes a package variable
46+
with no mutex. Set from `init()` it is safe, but the function is *exported*,
47+
so any caller can rewrite it at any time from any goroutine. That is a data
48+
race by construction, and it is racy across the whole process — one library
49+
swapping the loader changes behaviour for every other consumer.
50+
3. **A ten-method interface exists only to break an import cycle.**
51+
`credentials.ConfigFile` mirrors `config.Config` (`GetAuthConfig`,
52+
`GetAuthConfigHierarchical`, `PutAuthConfig`, `DeleteAuthConfig`,
53+
`GetCredentialHelper`, `CredentialsStore`, `SetCredentialsStore`,
54+
`IsAuthConfigured`, `Path`, `Save`). Every change to config file handling now
55+
has to be made in two places that must stay in sync.
56+
57+
The underlying cause is just an import cycle: `config` needs `credentials` for
58+
`Credential`/`AuthConfig` types, and `credentials` needs `config` to load a
59+
config file.
60+
61+
## Proposal
62+
63+
Move the shared types into an internal package that both can import, so the
64+
cycle disappears and the global is unnecessary.
65+
66+
```
67+
registry/remote/internal/configfile/ (new)
68+
authconfig.go — AuthConfig, DecodeAuth
69+
configfile.go — the ConfigFile interface, Load
70+
```
71+
72+
- `credentials` imports `configfile` for the interface and loader.
73+
- `config` imports `configfile` for the same types and provides the concrete
74+
implementation.
75+
- Neither imports the other. `init()`, `SetDefaultConfigLoader`,
76+
`ConfigFileLoader`, and `ErrNoConfigLoader` are all deleted.
77+
78+
`credentials.NewStore` becomes:
79+
80+
```go
81+
func NewStore(configPath string, opts StoreOptions) (*DynamicStore, error) {
82+
cfg, err := configfile.Load(configPath)
83+
if err != nil {
84+
return nil, err
85+
}
86+
return NewStoreFromConfig(cfg, opts), nil
87+
}
88+
```
89+
90+
which works with no phantom import and no ordering dependency.
91+
92+
`credentials.AuthConfig` and `credentials.ConfigFile` stay as thin aliases of the
93+
internal types so the public API does not move:
94+
95+
```go
96+
type AuthConfig = configfile.AuthConfig
97+
type ConfigFile = configfile.ConfigFile
98+
```
99+
100+
## Alternatives considered
101+
102+
**Keep the global, add a mutex.** Fixes the race but not the action-at-a-distance
103+
or the duplicated interface. It also leaves an exported process-wide setter,
104+
which is the more consequential design problem.
105+
106+
**Merge `config` and `credentials` into one package.** Removes the cycle
107+
outright, but the two have genuinely different audiences — plenty of callers want
108+
credential resolution without containers-registries.d parsing — and it would be a
109+
much larger public API change.
110+
111+
**Have `config` own everything and make `credentials` config-free.** Arguably the
112+
cleanest long-term shape, but it inverts the existing dependency direction and
113+
would move `Store`, `DynamicStore`, and the native-helper handling. Too large for
114+
the current window.
115+
116+
## Migration impact
117+
118+
- `SetDefaultConfigLoader`, `ConfigFileLoader`, `ErrNoConfigLoader`: removed.
119+
These exist only in v3 pre-release, so no released API is affected.
120+
- `credentials.NewStore`: unchanged signature, no longer requires the caller to
121+
import `config`. This is strictly a relaxation — code that imported `config`
122+
keeps working.
123+
- `credentials.AuthConfig`, `credentials.ConfigFile`: unchanged as far as callers
124+
are concerned (type aliases).
125+
126+
## Recommendation
127+
128+
Do this before the v3 API freeze. Afterwards, removing an exported setter and
129+
an exported error becomes a breaking change, and the two-copy interface has to
130+
be maintained for the life of v3.
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
# Design: `signedBy` policy evaluation for tag references
2+
3+
Status: **proposal** — needs a decision before v3 GA.
4+
5+
## Problem
6+
7+
A `signedBy` policy requirement can never be satisfied when the image is
8+
referenced by tag.
9+
10+
`DefaultSignedByVerifier.Verify` needs the manifest digest, because that is what
11+
a simple-signing payload binds to (`critical.image.docker-manifest-digest`). It
12+
obtains one via `parseImageDigest`, which scans the reference for `@` and errors
13+
out if there is none:
14+
15+
```go
16+
func parseImageDigest(ref string) (digest.Digest, error) {
17+
for i := len(ref) - 1; i >= 0; i-- {
18+
if ref[i] == '@' { /* ... */ }
19+
}
20+
return "", fmt.Errorf("reference %s does not contain a digest", ref)
21+
}
22+
```
23+
24+
But policy is evaluated *before* resolution. `Repository.Resolve` and
25+
`Repository.FetchReference` call `checkPolicy(ctx, reference)` with whatever the
26+
caller passed in — usually a tag:
27+
28+
```go
29+
func (r *Repository) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) {
30+
if err := r.checkPolicy(ctx, reference); err != nil {
31+
return ocispec.Descriptor{}, err
32+
}
33+
// ...
34+
}
35+
```
36+
37+
So for `registry.example.com/app:v1` under a `signedBy` policy:
38+
`checkPolicy``IsImageAllowed``evaluateSignedBy``Verify`
39+
`parseImageDigest` fails → `Verify` returns an error → the evaluator returns
40+
`(false, err)` → the pull is denied. Every time, regardless of whether a
41+
perfectly valid signature exists.
42+
43+
This fails closed, so it is not a security hole. It is a functional gap: the
44+
feature is unusable for the most common way people name images.
45+
46+
## Why it is not a one-line fix
47+
48+
The obvious move — resolve the tag first, then evaluate policy — inverts the
49+
current ordering, and the ordering exists for a reason: policy should be able to
50+
reject a request *before* the client talks to the registry about it. Scope- and
51+
transport-level requirements (`reject`, `insecureAcceptAnything`) are meaningful
52+
pre-flight; signature requirements inherently are not.
53+
54+
There is also a re-entrancy hazard. Resolving a tag inside the verifier means
55+
calling back into `Repository.Resolve`, which calls `checkPolicy` again. The
56+
`policyCheckedKey` context marker prevents infinite recursion but only if it is
57+
threaded correctly through the verifier.
58+
59+
## How containers/image handles it
60+
61+
`containers/image` splits the decision in two. `PolicyContext.IsRunningImageAllowed`
62+
operates on an `UnparsedImage` that has already been fetched far enough to know
63+
its manifest digest, while reference-level rules are applied earlier against the
64+
parsed reference. Signature requirements only ever see a resolved image.
65+
66+
## Options
67+
68+
### 1. Two-phase policy evaluation (recommended target)
69+
70+
Split requirements by what they need:
71+
72+
- **Pre-resolve**: `reject`, `insecureAcceptAnything`, and any future
73+
reference-shaped rule. Evaluated in `checkPolicy` as today.
74+
- **Post-resolve**: `signedBy`, `sigstoreSigned`. Evaluated after the descriptor
75+
is known, against `ImageReference` carrying the resolved digest.
76+
77+
Sketch:
78+
79+
```go
80+
func (r *Repository) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) {
81+
if err := r.checkPolicyPreResolve(ctx, reference); err != nil {
82+
return ocispec.Descriptor{}, err
83+
}
84+
desc, err := r.Manifests().Resolve(withPolicyChecked(ctx), reference)
85+
if err != nil {
86+
return ocispec.Descriptor{}, err
87+
}
88+
if err := r.checkPolicyPostResolve(ctx, reference, desc); err != nil {
89+
return ocispec.Descriptor{}, err
90+
}
91+
return desc, nil
92+
}
93+
```
94+
95+
Cost: the `Evaluator` needs to partition requirements and expose two entry
96+
points, and every mutating/reading path in `repository.go` needs the second
97+
call sited correctly. `Fetch` already has a digest, so it feeds the post-resolve
98+
phase directly.
99+
100+
Risk to watch: a requirement set containing *only* post-resolve requirements
101+
must not let a pre-resolve pass be mistaken for an allow. The partition must
102+
track that at least one phase actually evaluated the requirement.
103+
104+
### 2. Verifier resolves the tag itself
105+
106+
Give `DefaultSignedByVerifier` a resolver and have it turn a tag into a digest
107+
on demand.
108+
109+
Cheaper — no change to the evaluator's shape — but it puts a network call inside
110+
a policy verifier, needs `withPolicyChecked` threaded through to avoid
111+
re-entering policy, and means the digest the signature is checked against is
112+
fetched by a different code path than the one that will actually pull the
113+
content. That last point is a TOCTOU seam: the tag could move between the
114+
verifier's resolve and the caller's.
115+
116+
### 3. Document `signedBy` as digest-only
117+
118+
Make `parseImageDigest`'s failure an explicit, documented limitation, and have
119+
`Verify` return a clearly-worded error naming the constraint.
120+
121+
Cheapest and honest, but it substantially reduces the feature's value — most
122+
users pull by tag.
123+
124+
## Recommendation
125+
126+
Target **option 1**. It matches containers/image semantics, avoids the TOCTOU
127+
seam in option 2, and is the only option under which `signedBy` is actually
128+
usable as specified.
129+
130+
Ship **option 3** as the interim state in the meantime: an explicit error and a
131+
documented limitation are much better than the current behaviour, where a
132+
correctly-signed image pulled by tag is denied with a message about digest
133+
parsing.
134+
135+
Shipping v3 GA with `signedBy` silently unusable for tag references is the
136+
outcome to avoid.

0 commit comments

Comments
 (0)