-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CEL auth program evaluation during JWT login #869
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Geoff Wilson <[email protected]>
Signed-off-by: Geoff Wilson <[email protected]>
Signed-off-by: Geoff Wilson <[email protected]>
Signed-off-by: Geoff Wilson <[email protected]>
Signed-off-by: Geoff Wilson <[email protected]>
and "remove_policies" Signed-off-by: Geoff Wilson <[email protected]>
Signed-off-by: Geoff Wilson <[email protected]>
Signed-off-by: Geoff Wilson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some comments inline, I think we might've crossed wires on the RFC, reading it more closely...
@@ -194,11 +205,99 @@ func (b *jwtAuthBackend) pathLogin(ctx context.Context, req *logical.Request, d | |||
return nil, err | |||
} | |||
|
|||
if err := b.applyCelRoles(ctx, req, allClaims, auth); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting, I think this is different than I'd expect based on discussion in #493 (comment) -- though, reading the RFC more carefully, I think there's a mismatch between RFC sections.
In "User-facing Description":
Administrators can define CEL extensions under a new endpoint,
auth/{oidc|jwt}/cel/roles/:name
, to be executed during authentication. CEL roles evaluate incoming claims and generatelogical.Auth{}
instances having appropriate token policies.
Namely, the "executed during authentication" makes me think it supplements the existing role-based system, as is the case in this code. However, I thought we had decided on replacing the role system entirely, which seems to match what is in the "Technical Description" section:
CEL roles integrate with Vault's Auth engine (OIDC and JWT) to dynamically evaluate claims. Instead of using static role-to-policy assignment with
bound_claim
, CEL programs validate claims and return alogical.Auth
instance containing dynamically assigned token policies.
(from the "Instead of using static role-to-policy..." portion of that quote).
My 2c., but I think I'd prefer the latter to the former as it is simpler to reason about and has fewer moving parts.
In particular, though not articulated in the RFC, I think I'd have expected a CEL/regular role name to be globally unique (across both types). Login (with a role name) would then lookup the single unique role (whether regular or CEL) and evaluate just that one role on the request.
Or, perhaps easier, rather than globally unique, have a cel/login/:role
path, which logs in with the specified CEL role.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do -- I was thinking a layering/additive approach might make it easier to compose a bunch of uncomplicated CEL programs -- but your idea makes more sense with the way regular roles work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the existing --role
param can include a cel prefix when it's desired, eg --role cel/roles/db_user
. That way, the names don't have to be unique across role and cel/role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think there's a lot of complexity here if we want to completely replace role -- everything that roles
do, the cel/roles
will need to do. The approach in my PR was more supplemental (to just post-process the token_policies, essentially) based on some role (eg, default) already being applied. I think it's worth considering this strategy before discarding since it's much simpler -- the "cel_auth_program" could even become an reference attribute on the role?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought -- what if cel_auth_program
is just an optional field belonging to jwtRole
. We apply if it exists for the active role, otherwise it's just a normal role. This avoids issue of name uniquesness across different role categories; and allows a CelRole
to behave like existing for other fields such as OIDCScopes
, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I've also been contemplating this. In particular, with pure CEL roles, role authors would have to write rules for cases that could be handled by simply enabling or disabling a flag in a regular role. , for instance:
Flag: allow_localhost = true
CEL Equivalent: request.common_name == 'localhost' || request.common_name == '127.0.0.1'
This level of detail might become overly cumbersome. For instance, what if the author’s intent was simply to ensure that two specific domains (e.g., example.com and example.org) were included in the request in addition to a few other regular role constraints.
A possible solution might be to allow role authors to optionally combine CEL roles with regular roles. I haven’t worked out a concrete design yet, but perhaps CEL roles could take an additional parameter referencing the path to a regular role. When validation requests are made, both the CEL rules and the traditional role's validation rules could be applied.
That said, I share the same concerns as @cipherboy’s first point. Semantically invalid combination of roles could be easily created. For example, a CEL role might specify a rule like request.common_name == "example.com"
, which is valid on its own. But if combined with a regular role that lacks cn_validations: ["hostname"]
, the rule becomes obsolete and could fail in unexpected ways.
This idea seems like the inverse of @suprjinx's proposal:
Another thought -- what if cel_auth_program is just an optional field belonging to jwtRole. We apply if it exists for the active role, otherwise it's just a normal role.
Initially, I thought CEL roles could optionally inherit the same fields as regular roles, enforcing those rules in addition to CEL rules. However, I think this is too complex and has similar problems as the previous suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea that just came to mind is introducing the CEL rule equivalents of regular role parameters as globally accessible supplemental variables for CEL role authors. That way, they can simply specify the variable in their cel expressions. For example, rather than requiring authors to rewrite something like:
request.common_name == 'localhost' || request.common_name == '127.0.0.1'
They could simply reference a predefined variable like allow_localhost
in their CEL expressions.
This would reduce the burden on CEL role authors by allowing them to leverage the existing regular role logic without manually replicating it.
However, I'm uncertain about the complexity and potential cost this could add to the CEL program itself, particularly around managing and validating these supplemental variables. For instance:
- Would these variables need to be defined globally, even when the corresponding feature isn’t enabled in the system? For example, if the system defines a global variable
allow_localhost
as part of the CEL environment, should it always exist (perhaps defaulting to false) even if no rule explicitly enablesallow_localhost
? - How should conflicting logic between predefined variables and custom CEL rules be handled? For example, what happens if a predefined variable suggests one outcome, but the custom CEL rule implies another?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @fatima2003! You raise good questions here. My reaction is that the CEL rule should be supplemental to the role, so the CEL author doesn't have to specify all of role attributes. As a "post-processing program" it would be pretty clear that the CEL rule has final say on an attribute (in this PR, only token_policies
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cipherboy looks like I found the wrong meeting on the wiki for the community call today, sorry I missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suprjinx No worries! You might've been on the TSC call. I've published slides and recordings here: https://lf-edge.atlassian.net/wiki/spaces/OP/pages/15211863/OpenBao+Meetings (and also added a link to the correct bridge, hopefully).
We discussed this on the call, but rather than using variables, I think we can inject helper functions into the environment that will behave similarly to the roles. We could even go all the way to allowing full role evaluation (using the same logic as the Go side) using either a named role or using a stubbed role configuration. That way, CEL authors would have a very easy way of either using pieces of roles policy or using a full policy along side their additional CEL policy, if they'd like to.
I'm also soliciting feedback from the Dev WG (Jan, Dan mostly unless Nathan wishes to comment) -- but I think that with Fatima's approach, we can have a global policy/suggested way of using CEL as being an independent evaluation language (free of requiring a role evaluation), but with the option to call back into the role system if desired. This would also work later for Sentinel-like policies or a replacement to the ACL policy system, where the CEL policy could be evaluated and may optionally invoke additional ACL policy evaluations on the request, if desired.
return nil | ||
} | ||
for _, key := range celRoleKeys { | ||
celRole, err := b.getCelRole(ctx, req.Storage, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In particular, I don't quite like this behavior of evaluating all CEL policies against all tokens. It seems like it could lead to high complexity/latency if lots of CEL roles exist, which each validate a small portion of a token, rather than evaluating a single larger role.
This is a POC PR to rough out an approach for adding CEL auth_programs to the JWT auth method. CEL entries are posted to path
cel/roles
for the auth method, and chiefly contain a string attributeauth_program
. Allcel/roles
for the auth method are executed at the end of login and can modify thelogical.Auth
before it is returned.A CEL auth pogram that returns
false
or{"authorized": false}
will reject the login.A CEL auth program that returns
{"add_policies": ["some_policy","some_other"]}
will add the policies to thelogical.Auth
returned frompath_login
.A CEL auth program that returns
{"remove_policies": ["some_policy"]}
will remove the policy from thelogical.Auth
.The
cel/roles
CRUD pieces are borrowed from @fatima2003's PR #753Resolves #493
Co-authored-by @fatima2003