Implement CEL for StructuredAuthenticationConfig #121078

aramase · 2023-10-09T16:13:33Z

Add additional fields to JWTAuthenticator in AuthenticationConfiguration v1alpha1
Add support for CEL expressions and integrate with OIDC authenticator

fixes #119235

/kind feature
/sig auth
/triage accepted
/milestone v1.29
/priority important-soon

Adds CEL expressions to v1alpha1 AuthenticationConfiguration.

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3331-structured-authentication-configuration

k8s-ci-robot · 2023-10-09T16:13:35Z

@aramase: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Milestone Maintainers Team and have them propose you as an additional delegate for this responsibility.

In response to this:

TODO

fixes #119235

/kind feature
/sig auth
/triage accepted
/milestone v1.29
/priority important-soon
TODO
[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3331-structured-authentication-configuration

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-10-11T18:33:42Z

@aramase: The label(s) kind/api-review cannot be applied, because the repository doesn't have them.

In response to this:

/kind api-review

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aramase · 2023-10-11T18:34:37Z

/label api-review

enj

Initial API review on 10-12-23

enj · 2023-10-12T16:30:36Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go

+	// These allow invariants to be applied to incoming identities such as preventing the
+	// use of the system: prefix that is commonly used by Kubernetes components.
+	// +optional
+	UserInfoValidationRules []UserInfoValidationRule `json:"userInfoValidationRules,omitempty"`


The CEL variable that contains the final user info should be called user

Suggested change

UserInfoValidationRules []UserInfoValidationRule `json:"userInfoValidationRules,omitempty"`

UserValidationRules []UserValidationRule `json:"userValidationRules,omitempty"`

Specifically that all validation rules are logically ANDed

Internally, we may want to combine all the CEL expressions by wrapping them in parentheses and using logical ANDs (after we benchmark to see if it worth it). On a failure, we would run the individual expressions so that we could provide the correct error message.

staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

staging/src/k8s.io/apiserver/pkg/authentication/cel/interface.go

cici37

Just did a quick pass, will save time for another pass on imp details and tests. Thank you!

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

staging/src/k8s.io/apiserver/pkg/authentication/cel/compile.go

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

MaryamTavakkoli · 2023-10-27T08:50:21Z

Hello!
Bug triage for the 1.29 release cycle is here! I want to check the status and remind you that the code freeze is starting 01:00 UTC Wednesday 1st November 2023 / 18:00 PDT Tuesday 31st October 2023 (next week), as we want to ensure that each PR has a chance to be merged.
Are we still targeting the 1.29 release?

aramase · 2023-10-27T17:36:26Z

Hello! Bug triage for the 1.29 release cycle is here! I want to check the status and remind you that the code freeze is starting 01:00 UTC Wednesday 1st November 2023 / 18:00 PDT Tuesday 31st October 2023 (next week), as we want to ensure that each PR has a chance to be merged. Are we still targeting the 1.29 release?

Yes, this PR is still targeted for v1.29.

staging/src/k8s.io/apiserver/pkg/apis/apiserver/types.go

staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go

liggitt · 2023-10-31T02:36:53Z

staging/src/k8s.io/apiserver/pkg/authentication/cel/mapper.go

+
+// CELMapper is a struct that holds the compiled expressions for
+// username, groups, uid, extra, claimValidation and userValidation
+type CELMapper struct {


non-blocking since this doesn't touch the config API layer, but ClaimsMapper is a really confusing interface... each instance held in CELMapper is only valid to call one of the methods. It's weird to have to put the documentation about which uses of this interface should call which methods on the interface doc.

interaces like these would be clearer (even if the names get a little silly by the end):

Username StringMapper Groups StringArrayMapper UID StringMapper Extra MapStringStringArrayMapper ClaimValidationRules BoolMapper UserValidationRules BoolMapper

type BoolMapper interface { MapBool(context.Context, *unstructured.Unstructured) (bool, error) } type StringMapper interface { MapString(context.Context, *unstructured.Unstructured) (string, error) } type StringArrayMapper interface { MapStringArray(context.Context, *unstructured.Unstructured) ([]string, error) } type MapStringStringArrayMapper interface { MapMapStringStringArray(context.Context, *unstructured.Unstructured) (map[string][]string, error) }

specific implementations of those could wrap an interface like

EvalSingle(context.Context, *unstructured.Unstructured) (EvaluationResult, error)

or

EvalMultiple(context.Context, *unstructured.Unstructured) ([]EvaluationResult, error)

and type-check / accumulate

I think this could be cleaned up in 1.30 to be a lot more composable

added this to the follow-up items list for v1.30. I'll get this cleaned up.

liggitt · 2023-10-31T02:42:05Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

-		return nil, err
+	authenticator := &Authenticator{}
+
+	celMapper, fieldErr := apiservervalidation.ValidateJWTAuthenticator(opts.JWTAuthenticator)


nit: if we're doing more than validation, rename to CompileAndValidateJWTAuthenticator?

will celMapper be nil if there are no CEL rules? will fieldErr be non-nil if the structuredauthn feature is off and JWTAuthenticator contains cel rules?

will celMapper be nil if there are no CEL rules? will fieldErr be non-nil if the structuredauthn feature is off and JWTAuthenticator contains cel rules?

celMapper is not nil when there are no CEL rules. The subfields will be empty if there are no expressions defined for the field.

fieldErr will be non-nil if feature is off and any of the fields in JWTAuthenticator contain expressions. We have validation in all the fields that support an expression.

liggitt · 2023-10-31T02:46:17Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

+	compiler := authenticationcel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
+	mapper := &authenticationcel.CELMapper{}


if the feature's not enabled, probably don't need to construct these, right?

hmm.. I can't tell if celMapper is conditional on the feature gate (will be nil if the feature's off) or on whether CEL expressions were used for various things (subfields will be nil if there's no CEL group expression, etc) or both.

if the feature's not enabled, probably don't need to construct these, right?

right now I'm passing the struct to different validate funcs that set the subfields instead of returning subfields, so we're initializing this.

hmm.. I can't tell if celMapper is conditional on the feature gate (will be nil if the feature's off) or on whether CEL expressions were used for various things (subfields will be nil if there's no CEL group expression, etc) or both.

In the current implementation, if the feature gate is not set, the subfields that contain an expression will be nil but celMapper is not nil.

We can return nil from validateJWTAuthenticator if feature gate is not set.

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

liggitt · 2023-10-31T17:41:16Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

+		}
+	}
+
+	compilationResults, err := validatePrefixClaimOrExpression(compiler, m.Username, fldPath.Child("username"), true, structuredAuthnFeatureEnabled)


to strengthen in 1.30: if we're using the same validatePrefixClaimOrExpression function to construct the CEL mapper for both username and groups, how is it doing any kind of type checking that the result is a string (for username) or a string / string array / null (for groups)?

a username or groups expression of "true" should fail type checking, but I don't think it would, I think it would fail at runtime

obviously, things returning claims directly we can't typecheck until runtime since claims are dynamic, but things returning cel literals can be typechecked

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

liggitt · 2023-10-31T18:19:33Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

+
+		if err != nil {
+			allErrs = append(allErrs, err)
+		} else if structuredAuthnFeatureEnabled {


we shouldn't be able to get here if structuredAuthnFeatureEnabled is false, right?

we do compile the CEL expression even when feature gate isn’t enabled and don’t short circuit on error. This is done to aggregate all errors as part of validation.

Leaving this as-is based on the slack conversation.

liggitt · 2023-10-31T18:34:33Z

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

-	if m.Groups.Prefix != nil && len(m.Groups.Claim) == 0 {
-		allErrs = append(allErrs, field.Required(fldPath.Child("groups", "claim"), "non-empty claim name is required when prefix is set"))
+
+	if structuredAuthnFeatureEnabled && len(compilationResults) > 0 {


it shouldn't be possible to have non-zero compilation results and the feature be off, right?

It would be possible for non-zero compilation results when the feature is off as we perform validation and compiling of expression always. This was done to aggregate all errors from validation and return to user.

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

liggitt · 2023-10-31T18:58:24Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	switch val.Type().TypeName() {
+	case celgo.StringType.TypeName():


this needs cleanup in 1.30... I suspect there are more efficient ways to switch on the types natively in go, possibly like:

switch t := val.Value().(type) { case nilType: ... no-op case stringType: if len(t) > 0 { append } case []interface{}: ... iterate, safely cast, skip nil and empty string, append non-empty string, error on other types case []ref.Val{}: ... iterate, safely cast, skip nil and empty string, append non-empty string, error on other types default: ... error, unexpected type }

liggitt · 2023-10-31T19:05:59Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+
+	extra := make(map[string][]string, len(evalResult))
+	for _, result := range evalResult {
+		extraMapping, ok := result.ExpressionAccessor.(*authenticationcel.ExtraMappingCondition)


this cast is a code smell to clean up in 1.30 before beta... we're encapsulating the key information in the accessor, carrying it along to the evalresults, and then digging it back out here.

Agreed! Have a task in #121553 to try and use generics.

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

liggitt · 2023-10-31T21:29:58Z

/approve
for API bits

@enj has lgtm

enj · 2023-10-31T22:15:59Z

/lgtm
/approve

k8s-ci-robot · 2023-10-31T22:16:07Z

LGTM label has been added.

Git tree hash: b34c77509b763f829284ef76394d17d3538ccba4

k8s-ci-robot · 2023-10-31T22:16:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, 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:

~~staging/src/k8s.io/apiserver/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/pkg/authentication/OWNERS~~ [enj,liggitt]
~~staging/src/k8s.io/apiserver/plugin/pkg/authenticator/OWNERS~~ [enj,liggitt]
~~test/OWNERS~~ [enj,liggitt]
~~vendor/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aramase changed the title ~~Implement CEL for StructuredAuthenticationConfig~~ [WIP] Implement CEL for StructuredAuthenticationConfig Oct 9, 2023

k8s-ci-robot requested review from liggitt and mikedanese October 9, 2023 16:14

aramase mentioned this pull request Oct 9, 2023

Review #121078 aramase/kubernetes#2

Closed

aramase force-pushed the aramase/f/kep_3331_cel_integration branch from 569c36f to 790305f Compare October 11, 2023 16:47

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Oct 11, 2023

aramase force-pushed the aramase/f/kep_3331_cel_integration branch 3 times, most recently from c258db8 to 38283b9 Compare October 12, 2023 07:28

liggitt assigned liggitt and enj Oct 12, 2023

liggitt added this to Assigned in API Reviews Oct 12, 2023

liggitt moved this from Assigned to In progress in API Reviews Oct 12, 2023

enj reviewed Oct 12, 2023

View reviewed changes

liggitt moved this from In progress to Changes requested in API Reviews Oct 12, 2023

cici37 reviewed Oct 12, 2023

View reviewed changes

liggitt reviewed Oct 31, 2023

View reviewed changes

aramase force-pushed the aramase/f/kep_3331_cel_integration branch from e851239 to 96d94ab Compare October 31, 2023 05:25

aramase requested review from liggitt and enj October 31, 2023 06:07

aramase force-pushed the aramase/f/kep_3331_cel_integration branch 2 times, most recently from dca4357 to 905a09c Compare October 31, 2023 15:49

liggitt reviewed Oct 31, 2023

View reviewed changes

staging/src/k8s.io/apiserver/pkg/apis/apiserver/v1alpha1/types.go Outdated Show resolved Hide resolved

add new fields in v1alpha1 StructuredAuthenticationConfiguration

6b97115

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

aramase force-pushed the aramase/f/kep_3331_cel_integration branch from 905a09c to 9d1cf54 Compare October 31, 2023 17:04

liggitt reviewed Oct 31, 2023

View reviewed changes

aramase added 2 commits October 31, 2023 20:16

Implement CEL and wire it with OIDC authenticator

26e3a03

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

add integration tests

cc190e0

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

aramase force-pushed the aramase/f/kep_3331_cel_integration branch from 9d1cf54 to cc190e0 Compare October 31, 2023 20:18

aramase requested a review from liggitt October 31, 2023 20:25

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023

k8s-ci-robot merged commit fafccc0 into kubernetes:master Oct 31, 2023
16 checks passed

aramase deleted the aramase/f/kep_3331_cel_integration branch October 31, 2023 23:33

enj mentioned this pull request Mar 14, 2024

Structured Authentication Config kubernetes/enhancements#3331

Open

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CEL for StructuredAuthenticationConfig #121078

Implement CEL for StructuredAuthenticationConfig #121078

aramase commented Oct 9, 2023 •

edited

k8s-ci-robot commented Oct 9, 2023

k8s-ci-robot commented Oct 11, 2023

aramase commented Oct 11, 2023

enj left a comment

enj Oct 12, 2023

enj Oct 12, 2023

enj Oct 12, 2023

cici37 left a comment

MaryamTavakkoli commented Oct 27, 2023

aramase commented Oct 27, 2023

liggitt Oct 31, 2023

aramase Oct 31, 2023

liggitt Oct 31, 2023

aramase Oct 31, 2023 •

edited

liggitt Oct 31, 2023

liggitt Oct 31, 2023

aramase Oct 31, 2023 •

edited

liggitt Oct 31, 2023 •

edited

liggitt Oct 31, 2023

aramase Oct 31, 2023

liggitt Oct 31, 2023

aramase Oct 31, 2023

liggitt Oct 31, 2023

liggitt Oct 31, 2023

aramase Oct 31, 2023

liggitt commented Oct 31, 2023

enj commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

	UserInfoValidationRules []UserInfoValidationRule `json:"userInfoValidationRules,omitempty"`
	UserValidationRules []UserValidationRule `json:"userValidationRules,omitempty"`

		compiler := authenticationcel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
		mapper := &authenticationcel.CELMapper{}

		switch val.Type().TypeName() {
		case celgo.StringType.TypeName():

Implement CEL for StructuredAuthenticationConfig #121078

Implement CEL for StructuredAuthenticationConfig #121078

Conversation

aramase commented Oct 9, 2023 • edited

k8s-ci-robot commented Oct 9, 2023

k8s-ci-robot commented Oct 11, 2023

aramase commented Oct 11, 2023

enj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cici37 left a comment

Choose a reason for hiding this comment

MaryamTavakkoli commented Oct 27, 2023

aramase commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramase Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramase Oct 31, 2023 • edited

Choose a reason for hiding this comment

liggitt Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 31, 2023

enj commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

k8s-ci-robot commented Oct 31, 2023

aramase commented Oct 9, 2023 •

edited

aramase Oct 31, 2023 •

edited

aramase Oct 31, 2023 •

edited

liggitt Oct 31, 2023 •

edited