Identity: Move actor JWT/cert minting into the main API - #1315
Identity: Move actor JWT/cert minting into the main API#1315Taahir Ahmed (ahmedtd) wants to merge 1 commit into
Conversation
571be1b to
f501545
Compare
|
Whoops, this isn't ready to review yet. |
Taahir Ahmed (ahmedtd)
changed the title
943838f to
10a0120
Compare
|
Ready for review now |
10a0120 to
461bc37
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.
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)
| 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
| 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
| 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?
| 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?
| } | ||
|
|
||
| // 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?
|
|
||
| // 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?
| 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?
2 participants
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.