Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[WIP]: Add missing authentication config fields to OpenShift API #1763

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ShazaAldawamneh
Copy link

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2025
@openshift-ci openshift-ci bot requested review from 2uasimojo and sadasu March 3, 2025 09:49
Copy link
Contributor

openshift-ci bot commented Mar 3, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign pacevedom for approval. For more information see the 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

@ShazaAldawamneh ShazaAldawamneh force-pushed the enhance-auth-config-api branch from ab1b580 to c330ea1 Compare March 4, 2025 09:35
Copy link
Contributor

openshift-ci bot commented Mar 19, 2025

@ShazaAldawamneh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/markdownlint 42e99ef link true /test markdownlint

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.


1. **Changes to `cluster-authentication-operator` Code:**

The generateAuthConfig method of the ExternalOIDCController is used to map OIDC provider configurations in the authentication.config.openshift.io custom resource to the structured authentication configuration format that the Kubernetes API server understands. This method will be updated to include logic to map the new fields introduced in the authentication.config.openshift.io CRD to their existing counterparts in the Kubernetes structured authentication configuration types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Link the method?

#### Security Risks
Adding new authentication-related API fields could allow cluster administrators to misconfigure their authentication layer leading to security vulnerabilities. The new API fields themselves don't introduce any security risk.
- **Mitigation:**
- Ensure we have robust admission and runtime validations to ensure that misconfigurations are prevented as much as possible prior to rolling out the authentication layer changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Ensure we have robust admission and runtime validations to ensure that misconfigurations are prevented as much as possible prior to rolling out the authentication layer changes.
- Ensure we have robust admission and runtime validations to ensure that misconfigurations are prevented as much as possible prior to rolling out the authentication layer changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

(add a newline between this and the next section)

Comment on lines +429 to +436
### Dev Preview -> Tech Preview

- Ability to utilize the enhancement end to end
- End user documentation, relative API stability
- Sufficient test coverage
- Gather feedback from users rather than just developers
- Enumerate service level indicators (SLIs), expose SLIs as metrics
- Write symptoms-based alerts for the component(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

We are going straight to TP with these changes right? Do we need this section?


- No new feature gate will be introduced.
- The ExternalOIDC feature will not be promoted to GA without these new API fields based on product management input.
- This approach is subject to change depending on discussions regarding HyperShift and Standalone OpenShift's unified feature maturity. For now, we will proceed with expectations for standalone OpenShift.
Copy link
Contributor

Choose a reason for hiding this comment

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

After further discussions with HyperShift folks it seems unlikely that we will be able to achieve this from a technical perspective. Should be safe to remove this bullet point.


## Mitigation Strategy

We plan to add a combination of admission time and runtime checks to prevent invalid configurations from being applied. These checks will ensure that only properly formatted and functional configurations are accepted before they are rolled out to the kube-apiserver.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense to include that there if the external OIDC authentication layer is broken, using a kubeconfig should still function correctly as a break-glass scenario.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants