Repository navigation
Verify actor JWTs in the credential exchange through OIDC discovery - #3447
Tim Hockin (thockin) merged 7 commits into
Conversation
|
cc ILLUM1N0X |
ILLUM1N0X
left a comment
There was a problem hiding this comment.
admin.go:266 and cmd/kubectl-ate/README.md:335 still describe a verifier reading jwks. Could those change in this PR?
3d87410 to
f1640f9
Compare
|
ILLUM1N0X on admin.go:266 and the kubectl-ate README: fixed in f1640f9. Since nothing reads |
|
ILLUM1N0X Max Thompson (@thompsonmax) the fixes for your comments are in. Could you take another look? Benjamin Elder (@BenTheElder) Dmitry Berkovich (@dberkov) #3358 is on M3 and waits on this one, so could this get M3 too? |
|
Looks good. PR desc needs updating:
|
Max Thompson (thompsonmax)
left a comment
There was a problem hiding this comment.
Left a couple non-blocking 🤖 comments, otherwise LGTM.
f1640f9 to
5feb97d
Compare
| defer cancel() | ||
| err := verifier.Refresh(ctx, time.Now()) | ||
| if certErr := (*tls.CertificateVerificationError)(nil); errors.As(err, &certErr) { | ||
| return fmt.Errorf("actor JWT issuer serves a certificate that does not verify against --actor-jwt-issuer-ca-file, or the system roots when it is empty: %w", err) |
There was a problem hiding this comment.
Has this path run on GCS at all? On kind (S3) the exchange answers UNIMPLEMENTED before we get here. With this change, if ate-idp-server's cert doesn't verify against the servicedns trust bundle, ate-api-server won't start on any GCS install. Can we run snapshotcredentials on GCS once before merging?
I assume you don't have access, so ILLUM1N0X can you check this?
There was a problem hiding this comment.
No, it has not run on GCS. I don't have access, and on kind the exchange answers UNIMPLEMENTED before this path. The start check exits the sidecar only on a certificate that does not verify and warns on any other error, and after b258896 the default issuer is checked against the system roots plus the servicedns trust bundle. Thanks for pinging ILLUM1N0X to run snapshotcredentials on GCS.
There was a problem hiding this comment.
I ran the e2e suite on our GKE cluster with this PR's build. ate-api-server started normally, the sidecar fetched the discovery document and keys from ate-idp-server with no certificate errors, and snapshotcredentials passed.
|
Max Thompson (@thompsonmax) ILLUM1N0X could you take another look at b258896? Since your approvals, the unknown-kid fix landed in 6d4b9eb and the CA trust change (system roots plus the servicedns bundle) in b258896, so the earlier approval no longer counts. Benjamin Elder (@BenTheElder) I can't run snapshotcredentials on GCS myself. If one of you can run it once before the tag, that covers your last ask. |
|
Hey Anish, could you please prioritize merging this PR as well, thanks! |
…rifier.Refresh Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
Signed-off-by: Anish Ramasekar <[email protected]>
…e actor JWT pool Secret Signed-off-by: Anish Ramasekar <[email protected]>
…lable A fetch for an unknown key ID starts a one-minute backoff even when the caller cancels it. Within that backoff, Verify returned a plain "unknown key ID", which the exchange maps to UNAUTHENTICATED, so a valid token with a newly rotated key ID failed without a retry. Until a fetch completes, wrap ErrSigningKeysUnavailable instead. Signed-off-by: Anish Ramasekar <[email protected]>
b258896 to
039f726
Compare
|
Max Thompson (@thompsonmax) ILLUM1N0X I rebased this onto main to clear the conflict in |
|
Anish Ramasekar (@aramase) for some reason DCO isn't responding here |
039f726 to
a012364
Compare
…le for the issuer The sidecar verified the actor JWT issuer with a CA file that ate-setup picked by an exact match on the default issuer, so an equivalent spelling such as https://idp.<ns>.svc:443 fell back to the system roots and stopped the sidecar. Verify the issuer against the system roots plus --actor-jwt-issuer-ca-file, which defaults to the service DNS trust bundle, and drop the ATE_API_ACTOR_JWT_ISSUER_CA_FILE ConfigMap key. Signed-off-by: Anish Ramasekar <[email protected]>
a012364 to
6822b56
Compare
Benjamin Elder (@BenTheElder) Looks like the DCO app missed the event for my last force-push; I re-pushed the same tree with a re-signed top commit to retrigger it. |
Max Thompson (thompsonmax)
left a comment
There was a problem hiding this comment.
LGTM, just left a few non-blocking comments. Recommend addressing those in a follow-up.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return newHTTPClient(transport), nil |
There was a problem hiding this comment.
🤖 Non-blocking, security hardening: this client only fetches from ate-idp-server, which serves both documents directly, so it never needs to follow a redirect. Today it uses Go's default policy, which follows up to 10 redirects to any host, including from https:// to http://, after which the key set arrives without TLS. A custom issuer's Ingress or proxy could send one. Could this client refuse redirects?
client := newHTTPClient(transport)
client.CheckRedirect = func(*http.Request, []*http.Request) error {
return http.ErrUseLastResponse
}
return client, nilfetchJSON then rejects the 3xx as non-200, so a redirect fails closed with the status code in the error. Doing it here leaves apiauthn's providers on NewHTTPClient unchanged.
Separately, discoverKeysForIssuer fetches jwks_uri without checking its scheme, so an http:// value has the same effect. Requiring https there is safe for every caller. Both are fine as a follow-up.
| ctx, cancel := context.WithTimeout(ctx, issuerCheckTimeout) | ||
| defer cancel() | ||
| err := verifier.Refresh(ctx, time.Now()) | ||
| if certErr := (*tls.CertificateVerificationError)(nil); errors.As(err, &certErr) { |
There was a problem hiding this comment.
🤖 Non-blocking nit, reliability improvement: a discovery document that names another issuer won't fix itself, since the issuer keeps serving the same document. But it only warns here, and then every exchange returns UNAVAILABLE and retries once a minute while the pod looks healthy. Like the certificate case above, could it fail the start? A sentinel from discoverKeysForIssuer would let this check tell it apart:
// ErrIssuerMismatch is wrapped when the discovery document names another issuer.
var ErrIssuerMismatch = errors.New("discovered issuer does not match")
// in discoverKeysForIssuer:
return nil, fmt.Errorf("%w: discovered %q, expected %q", ErrIssuerMismatch, oidcConfig.Issuer, issuer)and here, before the warning:
if errors.Is(err, oidcjwt.ErrIssuerMismatch) {
return fmt.Errorf("actor JWT issuer %s serves discovery for another issuer; it must match ate-idp-server's --issuer: %w", issuer, err)
}ate-setup writes all three issuer flags from one value, so this only catches a hand-edited config, such as :443 on one side only. Recommend as a follow-up.
| // issuer's keys already fetched by checkIssuerCertificate. The verifier | ||
| // trusts the system roots and caFile, re-read when it changes. | ||
| func newActorVerifier(ctx context.Context, issuer, caFile string) (*oidcjwt.Verifier, error) { | ||
| client, err := oidcjwt.NewHTTPClientWithRoots(credbundle.SystemPoolLoader(caFile)) |
There was a problem hiding this comment.
🤖 Non-blocking, hardening: for a .svc issuer, could this trust only the service DNS bundle?
Only our service DNS signer can legitimately issue a certificate for idp.<ns>.svc, since public CAs can't issue internal names. The system roots add only CAs that should never vouch for it, including any root an operator adds, such as a TLS-inspecting proxy's CA (this transport honors HTTPS_PROXY). A certificate from any of them, plus a redirected connection, lets someone serve their own key set, forge worker-for-actor JWTs, and get storage credentials for any actor. Trusting only the bundle makes that fail closed. It breaks nothing: before b258896 the default issuer already trusted only the bundle, and no public certificate can exist for a .svc name.
Choosing by the parsed host keeps the :443 fix:
u, err := url.Parse(issuer)
if err != nil {
return nil, fmt.Errorf("invalid --actor-jwt-issuer %q: %w", issuer, err)
}
loadRoots := credbundle.SystemPoolLoader(caFile)
if host := strings.ToLower(strings.TrimSuffix(u.Hostname(), ".")); strings.HasSuffix(host, ".svc") {
loadRoots = credbundle.PoolLoader(caFile)
}|
I am unqualified to review the security aspect of it but I approve M3 merge based on other reviewers |
…gent-substrate#3447) Part of agent-substrate#3095. The objectstore-plugin exchange read actor JWT keys from a file, a stopgap from before ate-idp-server. agent-substrate#3358 needs an issuer instead, so its `pkg/` API takes no key file ([thread](agent-substrate#3358 (comment))). - The exchange fetches keys through `<issuer>/.well-known/openid-configuration`. `--actor-jwt-jwks-file`, `oidcjwt.FileKeySource` and the `actor-id-jwt-jwks` volume are removed. - The sidecar verifies the issuer's certificate against the system roots plus `--actor-jwt-issuer-ca-file`, which is new and defaults to the service DNS trust bundle. The default ate-idp-server issuer verifies with or without an explicit `:443`, and a custom issuer can use a public certificate. ate-setup writes no CA setting for it. The sidecar re-reads the bundle when it changes and keeps the last good pool if a re-read fails. ate-api-server's `JWTProvider` CA files use the same reloading client and still replace the system roots there. - Nothing reads the pool Secret's `jwks` key now, so its writers are removed: the `admin` commands of kubectl-ate, ate-setup, `PublicJWKS` and `ConcretePool.JWKS`. Existing Secrets keep the stale key. - The sidecar fetches keys once at start. It exits on a certificate error and only warns on other errors, so an unreachable issuer fails exchanges with `UNAVAILABLE` until a fetch succeeds. Cached keys refresh after 5m, a failed refresh retries after 1m, and a token with an unknown key ID triggers at most one fetch per 1m. Until a fetch completes, including one the caller canceled, such a token gets `UNAVAILABLE`, so a newly rotated key ID is retried. Upgrade: drop `--actor-jwt-jwks-file` from custom sidecar configs. Set `--actor-jwt-issuer-ca-file` only if the issuer uses a private CA other than the service DNS one. Tests pass for `oidcjwt`, `credbundle`, `objectstoreplugin`, `cmd/objectstore-plugin`, `cmd/ate-setup` and `apiauthn`. The snapshotcredentials e2e needed no change. It compiles with `-tags=e2e` but has not run, as it needs GCS. - [x] Tests pass - [x] Docs updated --------- Signed-off-by: Anish Ramasekar <[email protected]> (cherry picked from commit 92040fe) Signed-off-by: Eitan Yarmush <[email protected]>
Bring `release-1.0` up to `main` at 5a5b192 with the 19 changes merged since the previous batch in #3536. The final tree matches that commit exactly. **Please use Rebase and merge to preserve the individual commits and author attribution.** All 19 applied cleanly in upstream order. Original authors, author dates, co-author and sign-off trailers, and `-x` source references are retained. Upgrade notes: - #3533: old gVisor MEMORY snapshots, including goldens, no longer restore. Recreate them after upgrading. MicroVM MEMORY snapshots still resume with their old mounts until recreated. `/tmp` is now always tmpfs; ROOTFS snapshots no longer preserve its contents, and image-provided files there are hidden. MicroVM `/tmp` counts against actor memory and is capped at half the guest's memory. gVisor `/dev/shm` is now a 64 MiB tmpfs. Mount a `durableDir` at either path when persistence or more space is needed. `/run` now comes from the image rootfs. - #3447: remove `--actor-jwt-jwks-file` from custom credential-exchange sidecar configs. Set `--actor-jwt-issuer-ca-file` only when the issuer uses a private CA other than the service DNS CA. - #3450: do not reuse a deleted atespace name. See the updated atespace name-reuse guidance. Includes #3505, which overlaps the open backport in #3547. GKE provisioning and teardown now live in [substrate-gke](https://github.com/ai-on-gke/substrate-gke) rather than this repository. <details> <summary>Included PRs, in cherry-pick order</summary> 1. #3500: demos/{sandbox,jupyter,multi-template}: fix tutorial prerequisites, walkthroughs, and counter volume 2. #3498: demos/counter: inline gVisor walkthrough and fix setup commands 3. #3511: e2e tests: Extend the existing tests to cover tag actor. 4. #2775: Generate API reference documentation and publish it on Substrate website 5. #3119: Document how people, pods and CI jobs should authenticate to ate-api 6. #3505: tools: remove setup-gcp and hack/teardown.sh 7. #3147: e2e: resume fidelity after SandboxConfig version edits 8. #3370: ate-setup: reconcile ate-api-server ConfigMaps and roll out on config changes 9. #1607: hack: reject breaking proto changes across each proto package 10. #3415: docs: update and add security diagrams 11. #3485: Resuming a microVM actor on the same node no longer reads its rootfs upper back from disk 12. #3447: Verify actor JWTs in the credential exchange through OIDC discovery 13. #3533: ocispec: mount tmpfs at /dev/shm, and /tmp on both runtimes 14. #3450: docs: Document that atespace names must not be reused 15. #3410: Document using more than one egress credential provider 16. #3486: atelet: register the device plugin on a new socket each time 17. #3553: ate-setup: wait for the demo namespace in delete demo 18. #3560: Fix multiple issues in claude-code-multiplex demo 19. #3517: docs: remove inaccurate early-stage architecture documentation </details> Validation: every intermediate tree matches its upstream source commit, all 19 authorship records and original attribution trailers are preserved, and `git diff --check` passes. The original 18 backport commits are unchanged. AI use: AI-assisted cherry-picking, attribution checks, and review. - [ ] Tests pass - [x] Docs updated --------- Signed-off-by: Mofi Rahman <[email protected]> Signed-off-by: Eitan Yarmush <[email protected]> Signed-off-by: Nayana Bidari <[email protected]> Signed-off-by: Julian Gutierrez Oschmann <[email protected]> Signed-off-by: Max Thompson <[email protected]> Signed-off-by: Steven Shriver <[email protected]> Signed-off-by: Grant McCloskey <[email protected]> Signed-off-by: Shrutiya <[email protected]> Signed-off-by: Haiyan Meng <[email protected]> Signed-off-by: Greg Castle <[email protected]> Signed-off-by: Benjamin Elder <[email protected]> Signed-off-by: Anish Ramasekar <[email protected]> Signed-off-by: Shruti Nair <[email protected]> Signed-off-by: Haven Xia <[email protected]> Co-authored-by: Mofi Rahman <[email protected]> Co-authored-by: nybidari <[email protected]> Co-authored-by: Julian Gutierrez Oschmann <[email protected]> Co-authored-by: Max Thompson <[email protected]> Co-authored-by: Grant McCloskey <[email protected]> Co-authored-by: Steven Shriver <[email protected]> Co-authored-by: shrutiyam-glitch <[email protected]> Co-authored-by: haiyanmeng <[email protected]> Co-authored-by: Greg Castle <[email protected]> Co-authored-by: Benjamin Elder <[email protected]> Co-authored-by: Anish Ramasekar <[email protected]> Co-authored-by: Shruti Nair <[email protected]> Co-authored-by: Haven Xia <[email protected]> Co-authored-by: Adam Wathieu <[email protected]>
…3447) Part of #3095. The objectstore-plugin exchange read actor JWT keys from a file, a stopgap from before ate-idp-server. #3358 needs an issuer instead, so its `pkg/` API takes no key file ([thread](#3358 (comment))). - The exchange fetches keys through `<issuer>/.well-known/openid-configuration`. `--actor-jwt-jwks-file`, `oidcjwt.FileKeySource` and the `actor-id-jwt-jwks` volume are removed. - The sidecar verifies the issuer's certificate against the system roots plus `--actor-jwt-issuer-ca-file`, which is new and defaults to the service DNS trust bundle. The default ate-idp-server issuer verifies with or without an explicit `:443`, and a custom issuer can use a public certificate. ate-setup writes no CA setting for it. The sidecar re-reads the bundle when it changes and keeps the last good pool if a re-read fails. ate-api-server's `JWTProvider` CA files use the same reloading client and still replace the system roots there. - Nothing reads the pool Secret's `jwks` key now, so its writers are removed: the `admin` commands of kubectl-ate, ate-setup, `PublicJWKS` and `ConcretePool.JWKS`. Existing Secrets keep the stale key. - The sidecar fetches keys once at start. It exits on a certificate error and only warns on other errors, so an unreachable issuer fails exchanges with `UNAVAILABLE` until a fetch succeeds. Cached keys refresh after 5m, a failed refresh retries after 1m, and a token with an unknown key ID triggers at most one fetch per 1m. Until a fetch completes, including one the caller canceled, such a token gets `UNAVAILABLE`, so a newly rotated key ID is retried. Upgrade: drop `--actor-jwt-jwks-file` from custom sidecar configs. Set `--actor-jwt-issuer-ca-file` only if the issuer uses a private CA other than the service DNS one. Tests pass for `oidcjwt`, `credbundle`, `objectstoreplugin`, `cmd/objectstore-plugin`, `cmd/ate-setup` and `apiauthn`. The snapshotcredentials e2e needed no change. It compiles with `-tags=e2e` but has not run, as it needs GCS. - [x] Tests pass - [x] Docs updated --------- Signed-off-by: Anish Ramasekar <[email protected]> (cherry picked from commit 92040fe) Signed-off-by: Eitan Yarmush <[email protected]>
Part of #3095. The objectstore-plugin exchange read actor JWT keys from a file, a stopgap from before ate-idp-server. #3358 needs an issuer instead, so its
pkg/API takes no key file (thread).<issuer>/.well-known/openid-configuration.--actor-jwt-jwks-file,oidcjwt.FileKeySourceand theactor-id-jwt-jwksvolume are removed.--actor-jwt-issuer-ca-file, which is new and defaults to the service DNS trust bundle. The default ate-idp-server issuer verifies with or without an explicit:443, and a custom issuer can use a public certificate. ate-setup writes no CA setting for it. The sidecar re-reads the bundle when it changes and keeps the last good pool if a re-read fails. ate-api-server'sJWTProviderCA files use the same reloading client and still replace the system roots there.jwkskey now, so its writers are removed: theadmincommands of kubectl-ate, ate-setup,PublicJWKSandConcretePool.JWKS. Existing Secrets keep the stale key.UNAVAILABLEuntil a fetch succeeds. Cached keys refresh after 5m, a failed refresh retries after 1m, and a token with an unknown key ID triggers at most one fetch per 1m. Until a fetch completes, including one the caller canceled, such a token getsUNAVAILABLE, so a newly rotated key ID is retried.Upgrade: drop
--actor-jwt-jwks-filefrom custom sidecar configs. Set--actor-jwt-issuer-ca-fileonly if the issuer uses a private CA other than the service DNS one.Tests pass for
oidcjwt,credbundle,objectstoreplugin,cmd/objectstore-plugin,cmd/ate-setupandapiauthn. The snapshotcredentials e2e needed no change. It compiles with-tags=e2ebut has not run, as it needs GCS.