Require email_verified to be used when email is set as username via CEL #123737
Conversation
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go
Show resolved
Hide resolved
| // Same as the --oidc-username-claim and --oidc-username-prefix flags. | ||
| // If username.expression is set, the expression must produce a string value. | ||
| // If username.expression uses 'claims.email', then 'claims.email_verified' must be used in | ||
| // username.expression or extra[*].valueExpression or claimValidationRules[*].expression. |
There was a problem hiding this comment.
Could you add integrations tests using claims.email_verified in
- username.expression
- extra[*].valueExpression
- claimValidationRules[*].expression
There was a problem hiding this comment.
I can though we can only cover the positive cases since the API server won't start on the negative cases.
There was a problem hiding this comment.
I don't feel strongly about unit or integration tests as long as the unit tests actually exercise the functional evaluator
There was a problem hiding this comment.
Opened #123825, will handle this in a follow-up due to limited time left in the release.
| return false | ||
| } | ||
|
|
||
| func usesEmailVerifiedClaim(ast *celgo.Ast) bool { |
There was a problem hiding this comment.
I'll trust the unit tests and @jpbetz's review of the traversal logic / comparison with the CEL visitor for correctness.
My only comment all the way down the chain is that it's not obvious which of the fields can be nil, and where the visitor currently works but would panic if it got an AST that was valid but unexpected.
Two ways to resolve that:
- know for sure what can / can't be nil (now or in the future with any possible future AST changes)
- check everything that can be nil for nil explicitly
There was a problem hiding this comment.
I added nil checks for everything.
| // Same as the --oidc-username-claim and --oidc-username-prefix flags. | ||
| // If username.expression is set, the expression must produce a string value. | ||
| // If username.expression uses 'claims.email', then 'claims.email_verified' must be used in | ||
| // username.expression or extra[*].valueExpression or claimValidationRules[*].expression. |
There was a problem hiding this comment.
I don't feel strongly about unit or integration tests as long as the unit tests actually exercise the functional evaluator
Signed-off-by: Monis Khan <mok@microsoft.com>
f6a102f to
121607e
Compare
|
LGTM label has been added. Git tree hash: 5d10fb759700fcf75c98372c30ff0ea165a3c44d
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/kind feature
/kind api-change
/assign liggitt aramase
/triage accepted
/milestone v1.30
/sig auth
Fixes #123675
xref: #130875