Repository navigation
Add pkg/objectstore/actorauthz so out-of-tree object-store providers can run the actor checks - #3358
Conversation
a88e650 to
3691d0b
Compare
3691d0b to
d153f6f
Compare
d153f6f to
83f8736
Compare
|
Anish Ramasekar (@aramase) is this meant to be a draft still? |
|
Benjamin Elder (@BenTheElder) yes, until #3447 merges. It removes |
The #3447 was not approved for RC2 yet. We did not discuss it during grooming meeting yesterday. |
Dmitry Berkovich (@dberkov) I just broke that change into a separate PR instead of increasing the size of this one. Happy to just fold it all into this PR. |
|
let's keep them split given the sizes |
|
Dmitry Berkovich (@dberkov) could #3447 go on the next grooming agenda for RC2? Max Thompson (@thompsonmax) approved it at 5feb97d, and ILLUM1N0X's description comments are addressed. If you'd rather review it as part of this PR, I can still fold it in. |
|
Anish Ramasekar (@aramase) at this point I think the plan is to cut tag early in the week unless there's a serious oustanding blocker, these PRs are already aspirationally in the milestone, but AFAICT this can land as soon as possible but the semantic version tag doesn't really matter here, since this only impacts building plugins. We have to land breaking changes. I left a review on #3447. |
|
Hey Anish, would you please prioritize this PR today? We would like to get it merged and bulk cherrypick changes into the release branch. Thanks!! |
…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]>
83f8736 to
36f4e36
Compare
… actor JWT issuer Public CAs cannot issue a certificate for a .svc name, so the system roots can only add CAs that should never vouch for the in-cluster issuer, such as a TLS-inspecting proxy's. StartVerifier now verifies a .svc issuer against the CA file alone, and any other issuer against the system roots as well. The host is lowercased and its trailing dot trimmed before the check. Signed-off-by: Anish Ramasekar <[email protected]>
…viders A provider outside this repository cannot import the exchange's actor checks under internal/. The new package wraps them. Authorize returns the actor UID, the grants with each prefix a URI ending in "/", and the token's expiry. It refuses a jwt over MaxJWTSize before decoding it, and returns Internal for a grant access it has no public value for. Close releases the issuer and ate-api-server connections. package-deps.sh bans the same imports here as in the in-tree actorauthz, and client-go. Signed-off-by: Anish Ramasekar <[email protected]>
…ernal/ Go allows a pkg/ package to import internal/ and an outside module to import that package. The script builds such packages from an outside module and checks the same module cannot import internal/ directly. Signed-off-by: Anish Ramasekar <[email protected]>
ed6b82a to
76414b6
Compare
| return nil, nil, errors.New("--actor-jwt-issuer-ca-file is required to serve the exchange on GCS") | ||
| } | ||
| verifier, err := newActorVerifier(ctx, cfg.issuer, cfg.issuerCAFile) | ||
| verifier, _, err := actorauthz.StartVerifier(ctx, cfg.issuer, cfg.issuerCAFile) |
There was a problem hiding this comment.
Non-blocking: I saw you mentioned this as an open point in the PR desc, but yes we should close the idle connections held by the issuer client in the verifier. I think we can just call CloseIdleConnections in the cleanup function newExchangeService returns?
|
Hi all, Can I get a status on this? We need to cut RC3 in O(hours) |
All the comments are addressed and I'm just waiting for approvals at this point. Max Thompson (@thompsonmax) has already approved. |
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
None of these are blocking, some of them would be good follow-ups in 1.0.x (e.g. the internal/ wrapper docs don't really belong in godoc IMO).
| // Expiry is the token's exp. A credential issued from a Decision must not | ||
| // outlive it. |
There was a problem hiding this comment.
non-blocking: I don't think we can promise this as written. atelet reuses a cached JWT until only 5m is left, since it "only has to be valid when the node plugin exchanges it, not for the duration of a transfer" (cmd/atelet/worker_actor_jwt.go:35), and there's no mid-transfer refresh (internal/objectstoreplugin/credential/credential.go:28), so a provider that follows this can hand out a ~5m credential for a multi-GiB upload. The GCS exchange doesn't follow it either, Expiry never reaches the issuer and gcsissuer issues up to 1h. (Expiry can also already be in the past, oidcjwt allows 5m of skew after exp.)
Can we drop the "must not outlive it" line and just say what it is, or make atelet and the GCS exchange honor it in a follow-up?
| if err != nil { | ||
| return nil, "", fmt.Errorf("invalid actor JWT issuer: %w", err) | ||
| } | ||
| if host := strings.ToLower(strings.TrimSuffix(u.Hostname(), ".")); strings.HasSuffix(host, ".svc") { |
There was a problem hiding this comment.
non-blocking: .svc.cluster.local is just as internal, but this (and TestIssuerRoots) keeps the system roots for it, intended? Config.IssuerCAFile documents the suffix rule in pkg/ now, so it'd be good to settle before it ships, either cover .svc.cluster.local too or say why it differs.
| // - Canceled, DeadlineExceeded: ctx ended during the key fetch, or the | ||
| // actor lookup was canceled or timed out. | ||
| func (a *Authorizer) Authorize(ctx context.Context, jwt string) (*Decision, error) { | ||
| if len(jwt) > MaxJWTSize { |
There was a problem hiding this comment.
non-blocking: can this move into the internal Authorize next to the empty check? Then the in-tree exchange gets the same cap and "runs the checks Substrate's GCS credential exchange runs" stays true.
| // This package imports packages under internal/, so that a provider runs the | ||
| // same code as the in-tree exchange and only this package's API is public. Go | ||
| // allows it: the internal rule checks the importer's path, and every package | ||
| // under go.ate.dev/substrate may import go.ate.dev/substrate/internal/..., | ||
| // while code outside the module may import this package. |
There was a problem hiding this comment.
non-blocking: I think this is the first pkg/ package that wraps internal/? That feels like a pattern to document in the pkg/ vs internal/ section of docs/dev/code-layout.md rather than here, provider authors reading the godoc don't need it.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| # Builds every pkg/ package that imports internal/ from a module outside |
There was a problem hiding this comment.
non-blocking: I'm really not excited about this, I can OK it for the GA but I think it's important that we have cheap gates and "compile lots of packages during a verify check in a somewhat dense bash script" isn't it
|
Eitan Yarmush (@EItanya) verbally approved including this in 1.0, and we have 2 code reviews, so there's N=3 "this should ship in 1.0" and two full reviews in that N=3 as well. |
|
Tim Hockin (@thockin) Benjamin Elder (@BenTheElder) this needs to be re-added to merge queue, got removed because of I had this in one other PR and opened a possible fix for this: #3374 |
…can run the actor checks (#3358) Step 2 of #3095. A provider in its own repository cannot import the actor checks under `internal/`. - `New(ctx, Config)` builds the verifier through `internal/objectstoreplugin/actorauthz.StartVerifier`, as the in-tree exchange does. - A `.svc` issuer is trusted only through `IssuerCAFile`, in-tree too. An install whose `.svc` issuer certificate verified only through the system roots now fails at sidecar start. - `Authorize(ctx, jwt)` returns `ActorUID`, `Grants` and `Expiry`, or a gRPC status error. A grant access with no public `Access` value gets `Internal`. - `Close` closes the ate-api-server connection and idle issuer connections. - `ateapiauth.ClientConfig` loses `K8sClient`. Its four users add the `k8sresolver` option themselves, so the package does not link client-go. - `hack/verify/package-deps.sh` bans cloud SDKs and client-go here. `hack/verify/pkg-external-import.sh` builds the package from an outside module. Tested: `hack/verify-all.sh` passes. `go test -race ./...` fails only in `internal/atunnel/egress` and `internal/netns`, as at the merge base. No e2e. - [ ] Tests pass - [x] Docs updated --------- Signed-off-by: Anish Ramasekar <[email protected]> (cherry picked from commit 2289ade) Signed-off-by: dberkov <[email protected]>
ateapiauth.ClientConfig lost K8sClient upstream (agent-substrate#3358), so the router's token-caller connection adds the k8sresolver option itself, as router.go does. The files this branch adds carry "The Agent Substrate Authors" copyright (agent-substrate#3644). Co-Authored-By: Claude Opus 5.5 <[email protected]>
ateapiauth.ClientConfig lost K8sClient upstream (agent-substrate#3358), so the router's token-caller connection adds the k8sresolver option itself, as router.go does. The files this branch adds carry "The Agent Substrate Authors" copyright (agent-substrate#3644).
Step 2 of #3095. A provider in its own repository cannot import the actor checks under
internal/.New(ctx, Config)builds the verifier throughinternal/objectstoreplugin/actorauthz.StartVerifier, as the in-tree exchange does..svcissuer is trusted only throughIssuerCAFile, in-tree too. An install whose.svcissuer certificate verified only through the system roots now fails at sidecar start.Authorize(ctx, jwt)returnsActorUID,GrantsandExpiry, or a gRPC status error. A grant access with no publicAccessvalue getsInternal.Closecloses the ate-api-server connection and idle issuer connections.ateapiauth.ClientConfiglosesK8sClient. Its four users add thek8sresolveroption themselves, so the package does not link client-go.hack/verify/package-deps.shbans cloud SDKs and client-go here.hack/verify/pkg-external-import.shbuilds the package from an outside module.Tested:
hack/verify-all.shpasses.go test -race ./...fails only ininternal/atunnel/egressandinternal/netns, as at the merge base. No e2e.