Refactor so we validate before auth on the vault#22835
Conversation
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
russell-stern
force-pushed
the
refactor/validate_before_auth
branch
from
d48922d to
d937b6d
Compare
|
product-security-plaid-production
Bot
requested review from
Atrax1,
bolekk and
kalverra
There was a problem hiding this comment.
General comment.
I think that both before and after the refactor, we have a lot of code around validation fragmented in different places.
Very hard to reason or understand which component is responsible for what, where does the namespace get inserted if empty, where do we validate inner owner matches authorized one, etc.
Perhaps in this, or later PR, we should consolidate and make the code simpler?
Also @cedric-cordenier
PS: My upcoming PR for owner canonicalization also struggles with these challenges. We ideally should mutate the request only at 1 place inside capability. And remove mutations from all other places.
| var Methods = append(append([]string(nil), UserSecretsMethods...), MethodPublicKeyGet) | ||
|
|
||
| // IsUserSecretsMethod reports whether method is a user-facing secrets management JSON-RPC method. | ||
| func IsUserSecretsMethod(method string) bool { |
There was a problem hiding this comment.
nit: rename to IsUserFacingMethod?
| // Every vault JSON-RPC method must have an explicit case here (binding logic or an intentional skip). | ||
| // Callers must validate request shape before authorization so malformed params surface as | ||
| // InvalidParamsError instead of authorization failures. | ||
| func bindVaultOwners(req jsonrpc.Request[json.RawMessage], workflowOwner string) error { |
There was a problem hiding this comment.
nit: rename to validateBoundVaultOwners
There was a problem hiding this comment.
just trying to add a hint in the method name that it is read-only for requests.
| if err := stripPrefixedRequestIDFromParams(&authReq, originalRequestID); err != nil { | ||
| h.lggr.Errorw("failed to normalize gateway request for authorization", "method", req.Method, "requestID", originalRequestID, "error", err) | ||
| if err := h.requestValidator.PrepareUserJSONRPCRequest(ctx, req, UserJSONRPCValidationOptions{ | ||
| SkipLabelValidation: true, |
There was a problem hiding this comment.
SkipLabelValidation should be false on here, since it is inside the Vault Node, where masterpublickey is always available
| // Prefix request id with authorizedOwner, to ensure uniqueness across different owners | ||
| // We do this ourselves to ensure the ID is unique and can't be tampered with by the user. | ||
| req.ID = authorizedOwner + vaulttypes.RequestIDSeparator + req.ID | ||
| if err := h.FinalizeAuthorizedJSONRPCRequest(&req, req.ID); err != nil { |
There was a problem hiding this comment.
This will mutate the request by adding an empty namespace if it wasn't there right?
I see this mutation was also existing in old code.
I'm a bit lost, how were we mutating the request earlier, without breaking the requestDigest on the gw_handler side.
Doesn't it mean the mutation would change the request digest, and thus the vault node would never match the allowlist digest?
There was a problem hiding this comment.
This file should likely be inside this package: core/capabilities/vault/vaultutils
2 participants
No description provided.