Merge pull request #2 from aramase/aramase/f/kep_3331_review_comments_1

address API review comments for extra mappings

Author: nabokihms

keps/sig-auth/3331-structured-config-for-oidc-authentication/README.md

@@ -130,7 +130,7 @@ Items marked with (R) are required *prior to targeting to a milestone / release*
 
 This enhancement proposal covers adding structured authentication configuration to the Kubernetes API server.
 Initially, only a `jwt` configuration will be supported, which will serve as the next iteration of the existing
-OIDC authenticator.  OIDC authentication is important part of Kubernetes, yet it has limitations in its current state.
+OIDC authenticator.  OIDC authentication is an important part of Kubernetes, yet it has limitations in its current state.
 Below we will discuss that limitation and propose solutions.
 
 # Motivation
@@ -223,10 +223,8 @@ jwt:
     uid:
       claim: 'sub'
     extra:
-    - key:
-        constant: 'client_name'  # TODO: decide if we really need this flexibility or can we just have constant keys
-      value:
-        claim: 'aud'
+    - key: 'client_name'
+      valueExpression: 'claims.some_claim'
   # TODO(enj): drop this and figure out to get from CEL
   # claimFilters:
   # - username
@@ -260,8 +258,6 @@ is `sub` required or is the requirement to just have some username field?
 Payloads with nested data are supported as well (it will be possible
 to use the `foo` value as a claim mapping):
 
-TODO(aramase): validate if CEL can work with multiple level of nesting
-
 ```json
 {
   "custom": {
@@ -290,7 +286,7 @@ type AuthenticationConfiguration struct {
 
     // jwt is a list of OIDC providers to authenticate Kubernetes users.
     // For an incoming token, each JWT authenticator will be attempted in
-    // the order in which it is specifcied in this list.  Note however that
+    // the order in which it is specified in this list.  Note however that
     // other authenticators may run before or after the JWT authenticators.
     // The specific position of JWT authenticators in relation to other
     // authenticators is neither defined nor stable across releases.  Since
@@ -325,7 +321,7 @@ type JWTAuthenticator struct {
     // ClaimsFilter []string `json:"claimFilters,omitempty"`
 
     // userInfoValidationRules are rules that are applied to final userInfo before completing authentication.
-    // These allow invariants to be applied to incoming identites such as preventing the
+    // 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"`
@@ -343,7 +339,7 @@ type JWTAuthenticator struct {
         // Required to be unique.
         URL string `json:"url,omitempty"`
 
-        // If specified, overrides the URL used to fetch discovery information.
+        // discoveryURL if specified, overrides the URL used to fetch discovery information.
         // Format must be https://url/path.
         // Example:
         // curl oidc.oidc-namespace (.discoveryURL field)
@@ -373,24 +369,24 @@ type JWTAuthenticator struct {
 
     ```go
     type ClaimValidationRule struct {
-        // Claim is the name of a required claim.
+        // claim is the name of a required claim.
         // Same as --oidc-required-claim flag.
         // Only string claims are supported.
         // Mutually exclusive with expression and message.
         // +optional
         Claim string `json:"claim"`
-        // RequiredValue is the value of a required claim.
+        // requiredValue is the value of a required claim.
         // Same as --oidc-required-claim flag.
         // Mutually exclusive with expression and message.
         // +optional
         RequiredValue string `json:"requiredValue"`
 
-        // Expression is a logical expression that is written in CEL https://github.com/google/cel-go.
+        // expression is a logical expression that is written in CEL https://github.com/google/cel-go.
         // Must return true for the validation to pass.
         // Mutually exclusive with claim and requiredValue.
         // +optional
         Expression string `json:"expression"`
-        // Message customizes the returned error message when expression returns false.
+        // message customizes the returned error message when expression returns false.
         // Mutually exclusive with claim and requiredValue.
         // Note that messageExpression is explicitly not supported to avoid
         // misconfigured expressions from leaking JWT payload contents.
@@ -421,7 +417,7 @@ type JWTAuthenticator struct {
 
     ```go
     type ClaimMappings struct {
-        // Username represents an option for the username attribute.
+        // username represents an option for the username attribute.
         // Claim must be a singular string claim.
         // TODO: decide whether to support a distributed claim for username (what are we required to correlate between the data retrieved for distributed claims? sub? something else?). Limit distributed claim support to OIDC things with clientID validation?
         // Expression must produce a string value.
@@ -431,55 +427,59 @@ type JWTAuthenticator struct {
         //     (3) if userName.expression is set instead, result of expression is used as-is without any implicit prefix
         // (1) and (2) ensure backward compatibility with the --oidc-username-claim and --oidc-username-prefix flags
         Username PrefixedClaimOrExpression `json:"username"`
-        // Groups represents an option for the groups attribute.
+        // groups represents an option for the groups attribute.
         // Claim must be a string or string array claim.
         // Expression must produce a string or string array value.
         // "", [], missing, and null values are treated as having no groups.
         // TODO: investigate if you could make a single expression to construct groups from multiple claims. If not, maybe []PrefixedClaimOrExpression?
         // +optional
         Groups PrefixedClaimOrExpression `json:"groups,omitempty"`
-        // UID represents an option for the uid attribute.
+        // uid represents an option for the uid attribute.
         // Claim must be a singular string claim.
         // Expression must produce a string value.
         // TODO: this is net new, should it just be expression?
         // +optional
         UID ClaimOrExpression `json:"uid,omitempty"`
-        // Extra represents an option for the extra attribute.
+        // extra represents an option for the extra attribute.
+        //
+        // # hard-coded extra key/value
+        // - key: "foo"
+        //   valueExpression: "bar"
         //
-        // TODO: examples for this?
+        // hard-coded key, value copying claim value
+        // - key: "foo"
+        //   valueExpression: "claims.some_claim"
         //
-        // # known key, value from claim
-        // - key: "example.com/myextrakey"
-        //   value:
-        //     claim: "hd"
+        // hard-coded key, value derived from claim value
+        // - key: "admin"
+        //   valueExpression: '(has(claims.is_admin) && claims.is_admin) ? "true":""'
         //
-        // # known key, value constructed by expression
-        // - key: "example.com/myextrakey"
-        //   value:
-        //     expression: claims.someclaim+":"+claims.someclaim
+        // If multiple mappings have the same key, the result will be a concatenation of all values
+        // with the order preserved.
+        // If the value is empty, the extra mapping will not be present.
         //
-        // # calculated key/value pairs? CEL returns [{key,value}, {key,[value, value...]}, ...] and we aggregate?
-        // TODO: ask joe/cici about CEL constructing/returning complex types
+        // possible future way to pull multiple extra values out via expression.
+        // TODO: confirm cel comprehensions/mapping is powerful enough to transform
+        // the input claims into a filtered / transformed map[string][]string output):
+        // # mutually exclusive with key/valueExpression
+        //     keyAndValueExpression: '{"key":"string-value", "key2": ["value1","value2"]}'
         //
         // +optional
         Extra []ExtraMapping `json:"extra,omitempty"`
     }
 
     type ExtraMapping struct {
-        // Key is a CEL expression to extract extra attribute key.
-        // Claim must be a singular string claim.
-        // Expression must produce a string value.
-        // "" and null values are treated as the extra mapping not being present.
+        // key is a string to use as the extra attribute key.
         Key string `json:"key"`
-        // Value is a CEL expression to extract extra attribute value.
-        // Claim must be a string or string array claim.
-        // Expression must produce a string or string array value.
+        // valueExpression is a CEL expression to extract extra attribute value.
+        // valueExpression must produce a string or string array value.
         // "", [], and null values are treated as the extra mapping not being present.
-        Value ClaimOrExpression `json:"value"`
+        // Empty string values contained within a string array are filtered out.
+        ValueExpression string `json:"valueExpression"`
     }
 
     type ClaimOrExpression struct {
-        // Claim is the JWT claim to use.
+        // claim is the JWT claim to use.
         // Either claim or expression must be set.
         // +optional
         Claim string `json:"claim"`
@@ -490,11 +490,11 @@ type JWTAuthenticator struct {
 
 
     type PrefixedClaimOrExpression struct {
-        // Claim is the JWT claim to use.
+        // claim is the JWT claim to use.
         // Either claim or expression must be set.
         // +optional
         Claim string `json:"claim"`
-        // Prefix is prepended to claim to prevent clashes with existing names.
+        // prefix is prepended to claim to prevent clashes with existing names.
         // Mutually exclusive with expression.
         // +optional
         Prefix string `json:"prefix"`