Skip to content
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

Add OIDC Federation Settings #479

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

d34dh0r53
Copy link

This templates the OIDC federation settings needed to configure Keystone to perform federation authentication.

Copy link
Contributor

openshift-ci bot commented Oct 9, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: d34dh0r53
Once this PR has been reviewed and has the lgtm label, please assign olliewalsh for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

// +kubebuilder:validation:Optional
// +kubebuilder:default=false
// Enablement of Federation configuration
EnableFederation bool `json:"enableFederation,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed then - just check for len(instance.spec.OIDCFederation) >0

Copy link
Author

Choose a reason for hiding this comment

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

Right now I'm going to leave the toggle for testing. It also follows the pattern of Secure RBAC in that the deployer can explicitly enable or disable it.

Copy link
Contributor

@vakwetu vakwetu Oct 11, 2024

Choose a reason for hiding this comment

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

OK - just realize of course that the reason upstream tests are failing is because you are not checking for len(instance.spec.OIDCFederation)>0. The reason this parameter is not like srbac is because the srbac setting does not have "omitEmpty"

api/bases/keystone.openstack.org_keystoneapis.yaml Outdated Show resolved Hide resolved
api/v1beta1/keystoneapi_types.go Outdated Show resolved Hide resolved
This templates the OIDC federation settings needed to configure Keystone
to perform federation authentication.
This addresses some missing brackets in the keystone.conf configuration
as well as the incorrect handling of secrets.  I've also added
kubebuilder defaults and the missing OIDCMemCacheServers parameter.
Passwordselectors now return the error if the operation is unsuccessful.
Field was missing from the controller
Since this is a feature that requires enablement I think it makes more
sense to do that direclty in the spec and then define the parameters for
OIDC Federation. I've also moved the default setter for EnableFederation
to the top to be consistent with the way other enablements work.
Makes the OIDC password selectors optional and provides defaults.
Then enableFederation boolean needs to be availalbe regardless.
This adds the KeystoneFederationIdentityProviderName parameter as
we need that to compute the location match strings.  This also removes
the OIDCRedirectURI parameter as that is computed in the template.
d34dh0r53 and others added 12 commits November 12, 2024 11:31
Add configuration for specifying the number of
fernet keys stored in the keystone secret.
More than 2 keys are needed, since rotating 2
keys would expire sessions on every rotation.

After configuration change, keys need to be
added/removed and rotated in the proper order,
to ensure that the sessions don't expire
prematurely.

Fernet key rotation is triggered in the reconcile
loop. The "rotated at" timestamp is set in the
secret annotation.

Co-Authored-By: Grzegorz Grasza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants