Repository navigation
Identity: Move actor JWT/cert minting into the main API - #1315
Conversation
571be1b to
f501545
Compare
|
Whoops, this isn't ready to review yet. |
943838f to
10a0120
Compare
|
Ready for review now |
10a0120 to
461bc37
Compare
461bc37 to
deab8c3
Compare
| ObjectRef tag = 1; | ||
| } | ||
|
|
||
| message MintActorJWTRequest { |
There was a problem hiding this comment.
[nit] missing doc comment here and a few places below (looks like tests are complaining about this)
There was a problem hiding this comment.
Done.
| CredentialBundlePath: s.workerCredentialBundlePath, | ||
| TrustBundlePath: s.podIdentityTrustBundlePath, | ||
| ExpectedActorUID: actorUID, | ||
| ActorUID: actorUID, |
There was a problem hiding this comment.
IIUC we also need to set actor atespace and name here, otherwise the check in https://github.com/agent-substrate/substrate/pull/1315/changes#diff-7888c14ca7ad7f062e76770497045d59a0a9dd4c4a8c967c95e403c1fcab0954R74 fails
There was a problem hiding this comment.
You're right, fixed. (Also in ateom-microvm)
| TrustBundlePath: s.podIdentityTrustBundlePath, | ||
| ExpectedActorUID: actorUID, | ||
|
|
||
| ActorUID: actorUID, |
There was a problem hiding this comment.
Also need to set actor atespace and name here, otherwise the check in https://github.com/agent-substrate/substrate/pull/1315/changes#diff-7888c14ca7ad7f062e76770497045d59a0a9dd4c4a8c967c95e403c1fcab0954R74 fails
There was a problem hiding this comment.
Done.
| if err != nil { | ||
| t.Fatalf("CreateActor failed: %v", err) | ||
| } | ||
| _, err = tc.client.MintActorJWT(t.Context(), &ateapipb.MintActorJWTRequest{ |
There was a problem hiding this comment.
Should we check the error here?
There was a problem hiding this comment.
Done.
| return errs | ||
| } | ||
|
|
||
| func (s *RPCService) MintActorJWT(ctx context.Context, req *ateapipb.MintActorJWTRequest) (*ateapipb.MintActorJWTResponse, error) { |
There was a problem hiding this comment.
The old RPC handlers called these auto-generated validation functions like so:
func validateMintJWTRequest(ctx context.Context, req *ateapipb.MintJWTRequest) field.ErrorList {
// Call the generated validation.
op := operation.Operation{Type: operation.Create}
return controlapi.Validate_MintJWTRequest(ctx, op, nil, req, nil)
}
func validateMintCertRequest(ctx context.Context, req *ateapipb.MintCertRequest) field.ErrorList {
// Call the generated validation.
op := operation.Operation{Type: operation.Create}
return controlapi.Validate_MintCertRequest(ctx, op, nil, req, nil)
}
I believe we need to copy that over to enforce the tag-based validation?
There was a problem hiding this comment.
Good catch. Done.
| } | ||
|
|
||
| // We only issue tokens with audience bindings. | ||
| if len(req.GetAudience()) == 0 { |
There was a problem hiding this comment.
The old handler also validated the issuer (caller.Issuer != s.actorIdentityJWTIssuer), should we do that in the new handler as well?
There was a problem hiding this comment.
That didn't make any sense -- you shouldn't have to authenticate with an Actor JWT in order to mint an Actor JWT. I would think typically, the thing calling this RPC would be the egress gateway, and it would be authenticating with a k8s client certificate.
|
|
||
| // Verify that this actor exists in the store. It doesn't need to be | ||
| // running, since we may need to issue JWTs during actor boot / resume. | ||
| dbActor, err := s.impl.GetActor(ctx, resources.ActorRefFromObjectRef(req.GetActor())) |
There was a problem hiding this comment.
Hmm, should we check that the actor is in a RUNNING state (or in some set of valid states) before minting tokens or certs for it?
There was a problem hiding this comment.
I am not sure. I think it's likely that components will need to issue credentials even if the actor is actively running (think about a frontend harness service that might need to access files, etc with actor identity, even if the LLM harness bits inside the actor proper are not currently running).
I think "not deleted" is the safest for now.
| return nil, fmt.Errorf("while retrieving actor: %w", err) | ||
| } | ||
| if dbActor.GetMetadata().GetUid() != req.GetActorUid() { | ||
| return nil, status.Error(codes.Aborted, "conflict; actor has been deleted and recreated") |
There was a problem hiding this comment.
Looks like egress.renew() only stops retrying on FailedPrecondition or PermissionDenied:
substrate/internal/atunnel/egress.go
Line 148 in 2fcfa64
Should we add Aborted to the list of codes checked there, or adjust the code returned here, to stop it from retrying?
There was a problem hiding this comment.
FailedPrecondition is like, the one single code you are not supposed to retry on. It means you need to fix something about your request or the state of the system.
I'll change that to Aborted.
There was a problem hiding this comment.
Whoops, I read that backwards. As you point out, those are the codes it's not retrying on. I've added Aborted to that code list.
f45aa7e to
28a47df
Compare
233cc21 to
2781b13
Compare
This was originally a separate service to make it easy to apply separate authentication and authorization interceptors. It now seems clear that they should be in controlapi, with the same authn/z handlers chosen there. * Remove checks that will be handled in the authorizer framework.
2781b13 to
12169d8
Compare
…der (agent-substrate#1834) Part of agent-substrate#1756. - Adds `--actor-jwt-issuer`. Unset, it defaults to `https://idp.<namespace>.svc`, the Service ate-idp-server will serve discovery from. Before this, `iss` was hardcoded to `https://api.ate-system.svc`, which points at ateapi's gRPC port. - Adds `oidcdiscovery.ParseIssuer`, which requires a canonical https URL with no query, fragment, or user info and strips trailing slashes. ate-idp-server will use it too, so both emit the same bytes. - Actor JWT headers now include `typ: JWT`. - Drops the `actorIdentityJWTIssuer` plumbing into `RPCService`, unused since agent-substrate#1315. The default `iss` changes, but `MintActorJWT` has no production caller yet. Testing: new unit tests for the parser, the default, the header, and flag resolution. `TestMintActorJWT_Success` now checks `typ`, `iss`, and `sub`. It needs Docker, so it hasn't run locally. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR (docs land later in the agent-substrate#1756 series)
…entity docs (agent-substrate#2597) Part of agent-substrate#1756. **Why:** every install still has to set `actorIdentityJWTProvider`, and `docs/authentication.md` says it decides who may mint actor JWTs. It no longer decides anything: - agent-substrate#757 added it when minting lived in a separate `ActorIdentity` service. `MintJWT` refused any caller whose JWT came from a different provider's issuer, which in practice limited minting to atelet's Kubernetes service account tokens. - agent-substrate#1315 moved minting into `ateapi.Control` and dropped that check, leaving caller limits to the authz framework. - agent-substrate#1834 removed the last code that read the provider's issuer. So ate-api-server requires a field and fails to start if it names a missing provider, yet nothing uses it, and the docs say it decides who may mint, which authorization now decides. Section 7 of `docs/api-guide.md` still describes the old service and its atelet-only rules. **What changes:** - `actorIdentityJWTProvider` is gone from ate-api-server's authentication config, and ate-setup stops writing it. - `docs/authentication.md` drops it and keeps the pointer to `authorization.md` for what a caller may do. - Section 7 of `docs/api-guide.md` now describes `MintActorJwt` and `MintActorCertificate` on `ateapi.Control`, who may call them, their fields, and the JWT's claims, instead of the old service, its caller checks, and a wrong SPIFFE path. The ADK paragraph, which pointed at the same service, now describes the egress gateway attaching actor JWTs. For reviewers: the config is parsed strictly and ate-setup only creates `ate-api-authentication` when it's missing, so an existing install must delete the `actorIdentityJWTProvider` line from that ConfigMap before ate-api-server restarts on this version. Testing: unit tests for the config loader and ate-setup's rendered config. I didn't run e2e; CI's e2e installs a fresh kind cluster, so it doesn't exercise the upgrade case. - [ ] Tests pass - [x] Appropriate changes to documentation are included in the PR
This was originally a separate service to make it easy to apply separate authentication and authorization interceptors.
It now seems clear that our authn/z framework will be strong enough to support atelet and external callers in one system (based on OpenFGA).
This change moves the MintJWT and MintCert RPCs into the control API, and removes some inline authz checks that will be handled by our unified authorizer framework.