vault: add generic authorizer with jwt auth support#21714
vault: add generic authorizer with jwt auth support#21714prashantkumar1982 wants to merge 15 commits intodevelopfrom
Conversation
product-security-plaid-production
bot
requested review from
archseer and
justinkaseman
…th-authorizer # Conflicts: # core/capabilities/vault/gw_handler.go # core/capabilities/vault/gw_handler_test.go # core/services/ocr2/delegate.go
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
There was a problem hiding this comment.
This file is pretty much just a rename of previous request_authorizer.go file
- Cap JWKS response body to 1 MB to prevent resource exhaustion - Make all AuthResult fields unexported with accessor methods - Add iat claim validation to JWT parser - Annotate unreachable return in allowlist retry loop - Fix misleading test name for digest verification delegation Made-with: Cursor
prashantkumar1982
force-pushed
the
codex/vault-gwt-auth-authorizer
branch
from
77cda12 to
68f66e2
Compare
…th-authorizer # Conflicts: # core/capabilities/vault/allow_list_based_auth_test.go # core/capabilities/vault/request_authorizer.go
CORA - Pending Reviewers
Legend: ✅ Approved | ❌ Changes Requested | 💬 Commented | 🚫 Dismissed | ⏳ Pending | ❓ Unknown For more details, see the full review summary. |
|
| @@ -0,0 +1,99 @@ | |||
| // Code generated by mockery v2.53.0. DO NOT EDIT. | |||
There was a problem hiding this comment.
Why not stub it with mockery? Would save quite a bit of code / maintenance
| expiresAt int64 | ||
| } | ||
|
|
||
| func NewAllowListBasedAuthResult(workflowOwner, digest string, expiresAt int64) *AuthResult { |
There was a problem hiding this comment.
nit: having a function seems a bit overkill to construct a struct (especially if we support more auth types), we could just inline this.
| if a.orgID != "" { | ||
| return a.orgID | ||
| } | ||
| return a.workflowOwner |
There was a problem hiding this comment.
Aren't we tying all secrets to the org ID? Or is this just for backwards compat and we're planning to remove it?
| } | ||
|
|
||
| // JWTBasedAuth validates Vault requests authenticated with JWTs. | ||
| type JWTBasedAuth interface { |
There was a problem hiding this comment.
Why do we need a separate interface, if it's the same as type Authorizer interface? The JWTBasedAuth could just implement the Authorizer interface?
| } | ||
|
|
||
| func (a *authorizer) authorizeRequest(ctx context.Context, req jsonrpc.Request[json.RawMessage]) (*AuthResult, error) { | ||
| if req.Auth == "" { |
There was a problem hiding this comment.
Could you add a comment on why we pick allowlist based auth here? IIIUC it's for backwards compat since we currently don't populate this field?
| }, nil | ||
| } | ||
|
|
||
| // NewDisabledJWTBasedAuth returns a JWTBasedAuth implementation that always fails closed. |
There was a problem hiding this comment.
suggestion: similar to the other comment on []grpc.Option, it's generally cleaner to expose a list of options []JwtAuthOption, allowing you to extend multiple flexible configs in the future, rather than exposing multiple constructors. The possible combinations can become excessive quickly.
| } | ||
| if !enabled { | ||
| v.lggr.Debugw("JWTBasedAuth rejected request because it is disabled", "method", req.Method, "requestID", req.ID) | ||
| return nil, errors.New("JWTBasedAuth is disabled") |
There was a problem hiding this comment.
Is this actually an on / off toggle, or a rate limit? The Limit call is confusing
| return nil, fmt.Errorf("invalid JWT auth token: %w", err) | ||
| } | ||
|
|
||
| requestDigest, err := req.Digest() |
There was a problem hiding this comment.
nit: it's a bit more efficient to call & validate this before validating the token, could prevent early JWKS calls
| return "", "", ErrMissingRequestDigest | ||
| } | ||
|
|
||
| if detail, ok := rawDetails.(map[string]interface{}); ok { |
There was a problem hiding this comment.
authorization_details can only be of array type, so this code path is redundant
| for _, d := range details { | ||
| if detail, ok := d.(map[string]interface{}); ok { | ||
| if wo, rd, err := parseAuthDetail(detail); err == nil { | ||
| return wo, rd, nil |
There was a problem hiding this comment.
The parsing is incorrect & we are returning too early. Here's a sample of how the details will look like:
"authorization_details": [
{
"type": "request_digest",
"value": "testdigestabc"
},
{
"type": "workflow_owner",
"value": "0xabcd"
}
]
You will need to look at both array elements, and drive the logic based on type.
2 participants
Summary
This adds the Vault auth abstraction needed for gateway-side JWT support while keeping current behavior unchanged because JWT auth remains disabled.
What changed
Authorizerused by both the Vault gateway handler and the Vault capability gateway handlerAllowListBasedAuthandJWTBasedAuthAuthResultcontract and a sharedRequestReplayGuardJWTBasedAuth, gated internally and failing closed when disabledBehavior