Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion config/v1/0000_10_config-operator_01_authentication.crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ metadata:
include.release.openshift.io/ibm-cloud-managed: "true"
include.release.openshift.io/self-managed-high-availability: "true"
include.release.openshift.io/single-node-developer: "true"
release.openshift.io/feature-set: Default
name: authentications.config.openshift.io
spec:
group: config.openshift.io
Expand Down Expand Up @@ -52,7 +53,7 @@ spec:
description: type identifies the cluster managed, user facing authentication mode in use. Specifically, it manages the component that responds to login attempts. The default is IntegratedOAuth.
type: string
webhookTokenAuthenticator:
description: webhookTokenAuthenticator configures a remote token reviewer. These remote authentication webhooks can be used to verify bearer tokens via the tokenreviews.authentication.k8s.io REST API. This is required to honor bearer tokens that are provisioned by an external authentication service.
description: "webhookTokenAuthenticator configures a remote token reviewer. These remote authentication webhooks can be used to verify bearer tokens via the tokenreviews.authentication.k8s.io REST API. This is required to honor bearer tokens that are provisioned by an external authentication service. \n Can only be set if \"Type\" is set to \"None\"."
type: object
required:
- kubeConfig
Expand Down Expand Up @@ -82,6 +83,7 @@ spec:
name:
description: name is the metadata.name of the referenced secret
type: string
x-kubernetes-list-type: atomic
status:
description: status holds observed values from the cluster. They may not be overridden.
type: object
Expand Down
14 changes: 14 additions & 0 deletions config/v1/custom.authentication.testsuite.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "[CustomNoUpgrade] Authentication"
crd: 0000_10_config-operator_01_authentication.crd-CustomNoUpgrade.yaml
tests:
onCreate:
- name: Should be able to create a minimal Authentication
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec: {} # No spec is required for a Authentication
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec: {}
110 changes: 110 additions & 0 deletions config/v1/techpreview.authentication.testsuite.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "[TechPreviewNoUpgrade] Authentication"
crd: 0000_10_config-operator_01_authentication.crd-TechPreviewNoUpgrade.yaml
tests:
onCreate:
- name: Should be able to create a minimal Authentication
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec: {} # No spec is required for a Authentication
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec: {}
- name: Cannot set username claim prefix with policy NoPrefix
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
prefixPolicy: NoPrefix
prefix:
prefixString: "myoidc:"
expectedError: "prefix must be set if prefixPolicy is 'Prefix', but must remain unset otherwise"
- name: Can set username claim prefix with policy Prefix
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
prefixPolicy: Prefix
prefix:
prefixString: "myoidc:"
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
prefixPolicy: Prefix
prefix:
prefixString: "myoidc:"
- name: Cannot leave username claim prefix blank with policy Prefix
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
prefixPolicy: Prefix
expectedError: "prefix must be set if prefixPolicy is 'Prefix', but must remain unset otherwise"
- name: Can set OIDC providers with no username prefixing
initial: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
prefixPolicy: NoPrefix
expected: |
apiVersion: config.openshift.io/v1
kind: Authentication
spec:
type: OIDC
oidcProviders:
- name: myoidc
issuer:
issuerURL: https://meh.tld
audiences: ['openshift-aud']
claimMappings:
username:
claim: "preferred_username"
prefixPolicy: NoPrefix
199 changes: 196 additions & 3 deletions config/v1/types_authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

// +genclient
// +genclient:nonNamespaced
// +kubebuilder:subresource:status
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// Authentication specifies cluster-wide settings for authentication (like OAuth and
Expand Down Expand Up @@ -50,12 +51,16 @@ type AuthenticationSpec struct {
OAuthMetadata ConfigMapNameReference `json:"oauthMetadata"`

// webhookTokenAuthenticators is DEPRECATED, setting it has no effect.
// +listType=atomic
WebhookTokenAuthenticators []DeprecatedWebhookTokenAuthenticator `json:"webhookTokenAuthenticators,omitempty"`

// webhookTokenAuthenticator configures a remote token reviewer.
// These remote authentication webhooks can be used to verify bearer tokens
// via the tokenreviews.authentication.k8s.io REST API. This is required to
// honor bearer tokens that are provisioned by an external authentication service.
//
// Can only be set if "Type" is set to "None".
//
// +optional
WebhookTokenAuthenticator *WebhookTokenAuthenticator `json:"webhookTokenAuthenticator,omitempty"`

Expand All @@ -69,6 +74,18 @@ type AuthenticationSpec struct {
// This allows internal components to transition to use new service account issuer without service distruption.
// +optional
ServiceAccountIssuer string `json:"serviceAccountIssuer"`

// OIDCProviders are OIDC identity providers that can issue tokens
// for this cluster
// Can only be set if "Type" is set to "OIDC".
//
// At most one provider can be configured.
//
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MaxItems=1
// +openshift:enable:FeatureSets=CustomNoUpgrade;TechPreviewNoUpgrade
OIDCProviders []OIDCProvider `json:"oidcProviders,omitempty"`
}

type AuthenticationStatus struct {
Expand Down Expand Up @@ -110,15 +127,17 @@ type AuthenticationType string
const (
// None means that no cluster managed authentication system is in place.
// Note that user login will only work if a manually configured system is in place and
// referenced in authentication spec via oauthMetadata and webhookTokenAuthenticators.
// referenced in authentication spec via oauthMetadata and
// webhookTokenAuthenticator/oidcProviders
AuthenticationTypeNone AuthenticationType = "None"

// IntegratedOAuth refers to the cluster managed OAuth server.
// It is configured via the top level OAuth config.
AuthenticationTypeIntegratedOAuth AuthenticationType = "IntegratedOAuth"

// TODO if we add support for an in-cluster operator managed Keycloak instance
// AuthenticationTypeKeycloak AuthenticationType = "Keycloak"
// AuthenticationTypeOIDC refers to a configuration with an external
// OIDC server configured directly with the kube-apiserver.
AuthenticationTypeOIDC AuthenticationType = "OIDC"
)

// deprecatedWebhookTokenAuthenticator holds the necessary configuration options for a remote token authenticator.
Expand Down Expand Up @@ -159,3 +178,177 @@ const (
// KubeConfigKey is the key for the kube config file data in a secret
KubeConfigKey = "kubeConfig"
)

type OIDCProvider struct {
Copy link
Contributor

@deads2k deads2k Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider for future, no need right now.

Is this the right scope for configuring a clientID and reference to a clientSecret for the console

Pros:

  1. keeps configuration close together from a user perspective
  2. will allow different clientIDs for different oidc providers for console

Cons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each client for each application should be different, or should be provided with means to request tokens with different audiences (e.g. the resource parameter). That means that should we choose to configure client parameters via this API, we need to make it configurable for a number of clients. That can get messy, but is still doable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesterday, Seth argued the opposite: specifically name every clientID/secret pair. We should get on the same page.

// Name of the OIDC provider
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Required
// +required
Name string `json:"name"`
// Issuer describes atributes of the OIDC token issuer
//
// +kubebuilder:validation:Required
// +required
Issuer TokenIssuer `json:"issuer"`

// ClaimMappings describes rules on how to transform information from an
// ID token into a cluster identity
ClaimMappings TokenClaimMappings `json:"claimMappings"`

// ClaimValidationRules are rules that are applied to validate token claims to authenticate users.
//
// +listType=atomic
ClaimValidationRules []TokenClaimValidationRule `json:"claimValidationRules,omitempty"`
}

// +kubebuilder:validation:MinLength=1
type TokenAudience string

type TokenIssuer struct {
// URL is the serving URL of the token issuer.
// Must use the https:// scheme.
//
// +kubebuilder:validation:Pattern=`^https:\/\/[^\s]`
// +kubebuilder:validation:Required
// +required
URL string `json:"issuerURL"`

// Audiences is an array of audiences that the token was issued for.
// Valid tokens must include at least one of these values in their
// "aud" claim.
// Must be set to exactly one value.
//
// +listType=set
// +kubebuilder:validation:Required
// +kubebuilder:validation:MaxItems=1
// +required
Audiences []TokenAudience `json:"audiences"`

// CertificateAuthority is a reference to a config map in the
// configuration namespace. The .data of the configMap must contain
// the "ca-bundle.crt" key.
// If unset, system trust is used instead.
CertificateAuthority ConfigMapNameReference `json:"issuerCertificateAuthority"`
}

type TokenClaimMappings struct {
// Username is a name of the claim that should be used to construct
// usernames for the cluster identity.
//
// Default value: "sub"
Username UsernameClaimMapping `json:"username,omitempty"`

// Groups is a name of the claim that should be used to construct
// groups for the cluster identity.
// The referenced claim must use array of strings values.
Groups PrefixedClaimMapping `json:"groups,omitempty"`
}

type TokenClaimMapping struct {
// Claim is a JWT token claim to be used in the mapping
//
// +kubebuilder:validation:Required
// +required
Claim string `json:"claim"`
}

// +kubebuilder:validation:XValidation:rule="has(self.prefixPolicy) && self.prefixPolicy == 'Prefix' ? (has(self.prefix) && size(self.prefix.prefixString) > 0) : !has(self.prefix)",message="prefix must be set if prefixPolicy is 'Prefix', but must remain unset otherwise"
type UsernameClaimMapping struct {
TokenClaimMapping `json:",inline"`

// PrefixPolicy specifies how a prefix should apply.
//
// By default, claims other than `email` will be prefixed with the issuer URL to
// prevent naming clashes with other plugins.
//
// Set to "NoPrefix" to disable prefixing.
//
// Example:
// (1) `prefix` is set to "myoidc:" and `claim` is set to "username".
// If the JWT claim `username` contains value `userA`, the resulting
// mapped value will be "myoidc:userA".
// (2) `prefix` is set to "myoidc:" and `claim` is set to "email". If the
// JWT `email` claim contains value "userA@myoidc.tld", the resulting
// mapped value will be "myoidc:userA@myoidc.tld".
// (3) `prefix` is unset, `issuerURL` is set to `https://myoidc.tld`,
// the JWT claims include "username":"userA" and "email":"userA@myoidc.tld",
// and `claim` is set to:
// (a) "username": the mapped value will be "https://myoidc.tld#userA"
// (b) "email": the mapped value will be "userA@myoidc.tld"
//
// +kubebuilder:validation:Enum={"", "NoPrefix", "Prefix"}
PrefixPolicy UsernamePrefixPolicy `json:"prefixPolicy"`

Prefix *UsernamePrefix `json:"prefix"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still no CEL here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test should be able to prove CEL is missing.

Copy link
Contributor Author

@stlaz stlaz Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure which CEL validation is missing. I defined some rules on the object level.

I added tests that prove that the current CEL works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see my review mistake. Realized it in the shower this morning. Agreed.

}

type UsernamePrefixPolicy string

var (
// NoOpinion let's the cluster assign prefixes. If the username claim is email, there is no prefix
// If the username claim is anything else, it is prefixed by the issuerURL
NoOpinion UsernamePrefixPolicy = ""

// NoPrefix means the username claim value will not have any prefix
NoPrefix UsernamePrefixPolicy = "NoPrefix"

// Prefix means the prefix value must be specified. It cannot be empty
Prefix UsernamePrefixPolicy = "Prefix"
)

type UsernamePrefix struct {
// +kubebuilder:validation:Required
// +kubebuilder:validation:MinLength=1
// +required
PrefixString string `json:"prefixString"`
}

type PrefixedClaimMapping struct {
TokenClaimMapping `json:",inline"`

// Prefix is a string to prefix the value from the token in the result of the
// claim mapping.
//
// By default, no prefixing occurs.
//
// Example: if `prefix` is set to "myoidc:"" and the `claim` in JWT contains
// an array of strings "a", "b" and "c", the mapping will result in an
// array of string "myoidc:a", "myoidc:b" and "myoidc:c".
Prefix string `json:"prefix"`
}

type TokenValidationRuleType string

const (
TokenValidationRuleTypeRequiredClaim = "RequiredClaim"
)

type TokenClaimValidationRule struct {
// Type sets the type of the validation rule
//
// +kubebuilder:validation:Enum={"RequiredClaim"}
// +kubebuilder:default="RequiredClaim"
Type TokenValidationRuleType `json:"type"`

// RequiredClaim allows configuring a required claim name and its expected
// value
RequiredClaim *TokenRequiredClaim `json:"requiredClaim"`
}

type TokenRequiredClaim struct {
// Claim is a name of a required claim. Only claims with string values are
// supported.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Required
// +required
Claim string `json:"claim"`

// RequiredValue is the required value for the claim.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Required
// +required
RequiredValue string `json:"requiredValue"`
}
Loading