Extend declarative validation to MintJWT/MintCert requests - #1261
Extend declarative validation to MintJWT/MintCert requests#1261shrutiyam-glitch wants to merge 9 commits into
Conversation
Lior Lieberman (LiorLieberman)
left a comment
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
LGTM, one question
| "Stale": {requestUID: func(string) string { return "uid-of-a-previous-incarnation" }, wantCode: codes.FailedPrecondition}, | ||
| // The stale uid is well-formed on purpose: a malformed one is rejected | ||
| // as INVALID_ARGUMENT by request validation before the guard runs. | ||
| "Stale": {requestUID: func(string) string { return "9d1f7b06-3c58-4a2e-8b40-5f7c1e9a2d63" }, wantCode: codes.FailedPrecondition}, |
There was a problem hiding this comment.
what is this string?
There was a problem hiding this comment.
this uid should come from somewhere?
There was a problem hiding this comment.
This is a random UID that is assumed not to collide with the UID randomly picked by the store when it created the actor. I don't think we need the agent comment.
|
FYI I am working on a PR to move MintJWT / MintCert into the main API, and remove the dedicated ActorIdentity API. It seems clear from the authorization discussion that we will be able to handle any needed authorization using the overall authz framework we choose. |
| "Stale": {requestUID: func(string) string { return "uid-of-a-previous-incarnation" }, wantCode: codes.FailedPrecondition}, | ||
| // The stale uid is well-formed on purpose: a malformed one is rejected | ||
| // as INVALID_ARGUMENT by request validation before the guard runs. | ||
| "Stale": {requestUID: func(string) string { return "9d1f7b06-3c58-4a2e-8b40-5f7c1e9a2d63" }, wantCode: codes.FailedPrecondition}, |
There was a problem hiding this comment.
This is a random UID that is assumed not to collide with the UID randomly picked by the store when it created the actor. I don't think we need the agent comment.
|
|
||
| // +k8s:required | ||
| // +k8s:minimum=1 | ||
| // +k8s:maximum=1 # keep this in sync with the ActorCertificatePurpose enum |
There was a problem hiding this comment.
I am not sure we need to try to guard the enum values using k8s validation logic. Our implementation has to throw an error on an unrecognized purpose, and the error message will be better from there.
There was a problem hiding this comment.
Yeah
Actually, there is +k8s:enum in DV - https://kubernetes.io/docs/reference/using-api/declarative-validation/#tag-enum (stability level:beta though)
But, this was the kind of practice everywhere - https://github.com/agent-substrate/substrate/blob/main/pkg/proto/ateapipb/ateapi.proto#L443-L449, hence followed it for uniformity.
There was a problem hiding this comment.
sorry, nvm.
looks like +k8s:enum only works on string types. The proto enums compile to int32, so the tag can't be used on ours.
3 participants
Follow up on issue #1168 and the base PR #1215
Identity RPCs (
MintJWT/MintCert)MintJWTRequestandMintCertRequest.purpose != ATUNNELpolicy check remains in the handler).Notes
atespace/actor_namecan no longer mint malformed JWT subjects. UIDs must now be valid UUIDs.MintJWTempty-audience error is now anInvalidArgument(wasUnknown) checked before disk I/O.MintCert's atespace-on-global-ref error is now typedForbidden(wasInvalid).Testing
TestValidateMintJWTRequest/TestValidateMintCertRequest), as these requests previously lacked validation tests.controlapi,functionaltest, etc.),go vet, andgofmtpass cleanly.