WRKLDS-875: Add oc login external OIDC issuer integration enhancement #1596

ardaguclu · 2024-03-14T07:30:33Z

This PR reflects the work has been done to enable oc login supporting external OIDC issuers.
Main motivation behind this PR is to store the historical context of analysis about the changes and
elaborate the reasons for future readers.

PRs have merged in oc;

openshift-ci-robot · 2024-03-14T07:30:37Z

@ardaguclu: This pull request references WRKLDS-875 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.16.0" version, but no target version was set.

In response to this:

This PR reflects the work has been done to enable oc login supporting external OIDC issuers.
Main motivation behind this PR is to store the historical context of analysis about the changes and
elaborate the reasons for future readers.

PRs have merged in oc;

WRKLDS-998: oc login: Built-in cred exec plugin implementation and wiring oc#1640

OCPBUGS-30162: Introduce --issuer-url flag in oc login oc#1694

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 openshift-eng/jira-lifecycle-plugin repository.

deads2k · 2024-03-14T13:46:12Z

enhancements/oc/oc-login-external-oidc-integration.md

+| | Authorization Code<br/>(public/confidential) | Authorization Code + PKCE<br/>(public/confidential) | Implicit Flow<br/>(public) | Client Credentials<br/>(confidential) | Refresh Token<br/>(public/confidential) | Device Code<br/>(public) | Password Grant<br/>(confidential) |
+|-----------|---------------------------------------------------------------------|---------------------------------------------------------------------|----------------------------|--------------------------------------------------------------------------|-----------------------------------------|--------------------------|-----------------------------------|
+| Supported | Yes | Yes | No | No | Yes | No | No |
+| Comment | Can be used for confidential clients<br/>by passing --client-secret | Can be used for confidential clients<br/>by passing --client-secret | Deprecated | Mostly used in CI systems that <br/>client-secret can be stored securely | If provider supports it | | Deprecated |


for the "can be used for confidential clients" cases, is the confidentiality (or not) controlled by the external OIDC provider configuration? if so, let's add a * and explicitly explain that. It's also worth a look for whether say Entra and Keycloak support both public and confidential clients.

I'd also like to see a row for "it just works from the console copy command".

deads2k · 2024-03-14T13:46:50Z

enhancements/oc/oc-login-external-oidc-integration.md

+
+Supportability (in terms of authentication flows) matrix of this feature is outlined below; 
+
+| | Authorization Code<br/>(public/confidential) | Authorization Code + PKCE<br/>(public/confidential) | Implicit Flow<br/>(public) | Client Credentials<br/>(confidential) | Refresh Token<br/>(public/confidential) | Device Code<br/>(public) | Password Grant<br/>(confidential) |


If the "it just works for the console" row will vary based on public or confidential (I bet it will), let's separate the public/confidential into two differnet columns.

Yes, it will vary based on public or confidential. Separated.

enhancements/oc/oc-login-external-oidc-integration.md

stlaz · 2024-03-15T11:49:42Z

enhancements/oc/oc-login-external-oidc-integration.md

+Moreover, `oc login` command triggers authentication process by sending a request to Project API's whoami endpoint automatically so that
+client-go talks to `oc get-token` command to obtain id token (and also refresh token, if provider supports it).


Not sure I fully understand. Project API doesn't have a whoami endpoint?

Are you trying to say that as a part of the login process with oc login, a request is being sent already to the Project API, which in turn triggers the exec credential plugin?

Are you trying to say that as a part of the login process with oc login, a request is being sent already to the Project API, which in turn triggers the exec credential plugin?

yes

stlaz · 2024-03-15T11:52:14Z

enhancements/oc/oc-login-external-oidc-integration.md

+client-go talks to `oc get-token` command to obtain id token (and also refresh token, if provider supports it).
+
+There are currently 7 authentication flows grouped around public and confidential clients. Due to the nature of `oc` that is being used widely on local, 
+we have decided to support authentication flows only for public clients for now but if there is any different use case in the future, this command can support confidential clients too. 


Why do we expose the --client-secret flag above, then?

In case cluster (or system) administrator may want to use Auth Code with client-secret.

But that's going against the claim here

we have decided to support authentication flows only for public clients for now

ah, good point, you are right, let me change it

Removed that "only supports public clients" statements altogether because it is not correct.

enhancements/oc/oc-login-external-oidc-integration.md

deads2k · 2024-03-15T13:16:07Z

enhancements/oc/oc-login-external-oidc-integration.md

+| | Authorization Code<br/>(public) | Authorization Code<br/>(confidential) | Authorization Code + PKCE<br/>(public) | Authorization Code + PKCE<br/>(confidential) | Implicit Flow<br/>(public) | Client Credentials<br/>(confidential) | Refresh Token<br/>(public/confidential) | Device Code<br/>(public) | Password Grant<br/>(confidential) |
+|-----------|----------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------|----------------------------|--------------------------------------------------------------------------|-----------------------------------------|--------------------------|-----------------------------------|
+| Supported | Yes | Yes | Yes | Yes | No | No | Yes | No | No |
+| Comment | | Only if provider supports this[1] | | Only if provider supports this[1] | Deprecated[3] | Mostly used in CI systems that <br/>client-secret can be stored securely | If provider supports it | | Deprecated[4] |


let's use this row to break down each cell with

keycloak - [default|not-default], [configurable|not-configurable]

entra - [default|not-default], [configurable|not-configurable]

enhancements/oc/oc-login-external-oidc-integration.md

deads2k · 2024-03-15T14:02:38Z

enhancements/oc/oc-login-external-oidc-integration.md

+| | Authorization Code<br/>(public) | Authorization Code<br/>(confidential) | Authorization Code + PKCE<br/>(public) | Authorization Code + PKCE<br/>(confidential) | Implicit Flow<br/>(public) | Client Credentials<br/>(confidential) | Device Code<br/>(public) | Password Grant<br/>(confidential) |
+|------------------|----------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------|----------------------------|-----------------------------------------------------------|-----------------------------------------------------------|-----------------------------------|
+| Supported | Yes | Yes | Yes | Yes | No | No | No | No |
+| Comment | Keycloak - [configurable] Azure Entra ID - [configurable] | Keycloak - [not-configurable] Azure Entra ID - [configurable] | Keycloak - [configurable] Azure Entra ID - [configurable] | Keycloak - [not-configurable] Azure Entra ID - [configurable] | Deprecated[1] | Keycloak - [configurable] Azure Entra ID - [configurable] | Keycloak - [configurable] Azure Entra ID - [configurable] | Deprecated[2] |


followup required with keycloak to determine if it's really impossible to configure a confidential client for authorization flows. I'd be extremely suprrised.

ardaguclu · 2024-03-15T14:07:54Z

/label tide-merge-method-squash

openshift-ci · 2024-03-15T14:08:01Z

@ardaguclu: The label(s) /label tide-merge-method-squash cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label tide-merge-method-squash

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.

ardaguclu · 2024-03-15T14:09:12Z

/label tide/merge-method-squash

deads2k · 2024-03-15T15:33:02Z

cell rendering still doesn't show newline on github, but I'm ok dealing with that later

/approve

will leave lgtm with standa.

openshift-ci · 2024-03-15T15:33:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

~~OWNERS~~ [deads2k]

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

deads2k · 2024-03-18T13:37:29Z

/lgtm

ardaguclu · 2024-03-18T14:20:12Z

/retest

openshift-ci · 2024-03-18T14:30:12Z

@ardaguclu: all tests passed!

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

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 14, 2024

openshift-ci bot requested review from jwmatthews and stlaz March 14, 2024 07:30

ardaguclu force-pushed the oc-login-external-oidc branch 2 times, most recently from 2991f31 to 5c34ae6 Compare March 14, 2024 07:39

Add oc login external OIDC issuer integration enhancement

00a3e4c

ardaguclu force-pushed the oc-login-external-oidc branch from eca5a7d to 00a3e4c Compare March 14, 2024 08:03

deads2k reviewed Mar 14, 2024

View reviewed changes

enhancements/oc/oc-login-external-oidc-integration.md Outdated Show resolved Hide resolved

deads2k reviewed Mar 14, 2024

View reviewed changes

enhancements/oc/oc-login-external-oidc-integration.md Show resolved Hide resolved

Incorporate requested changes

138b6b8

stlaz reviewed Mar 15, 2024

View reviewed changes

Apply more nit-picks

4887433

stlaz reviewed Mar 15, 2024

View reviewed changes

enhancements/oc/oc-login-external-oidc-integration.md Outdated Show resolved Hide resolved

ardaguclu added 2 commits March 15, 2024 16:12

Remove partially incorrect statement

17c1a52

Simplify mitigation section example

9a80f6d

deads2k reviewed Mar 15, 2024

View reviewed changes

Reorganize plugin table

f84aedd

deads2k reviewed Mar 15, 2024

View reviewed changes

enhancements/oc/oc-login-external-oidc-integration.md Outdated Show resolved Hide resolved

deads2k reviewed Mar 15, 2024

View reviewed changes

enhancements/oc/oc-login-external-oidc-integration.md Outdated Show resolved Hide resolved

deads2k reviewed Mar 15, 2024

View reviewed changes

Remove unnecessary html tag

aff049e

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 15, 2024

Reformat tables

6b6f557

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2024

Reformat table one last time

bbd69ed

openshift-ci bot assigned deads2k Mar 18, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2024

openshift-merge-bot bot merged commit 0a68409 into openshift:master Mar 18, 2024
2 checks passed

ardaguclu deleted the oc-login-external-oidc branch March 18, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WRKLDS-875: Add oc login external OIDC issuer integration enhancement #1596

WRKLDS-875: Add oc login external OIDC issuer integration enhancement #1596

ardaguclu commented Mar 14, 2024

openshift-ci-robot commented Mar 14, 2024 •

edited by openshift-ci bot

Loading

deads2k Mar 14, 2024

ardaguclu Mar 15, 2024

deads2k Mar 14, 2024

ardaguclu Mar 15, 2024

stlaz Mar 15, 2024

ardaguclu Mar 15, 2024

stlaz Mar 15, 2024

ardaguclu Mar 15, 2024

stlaz Mar 15, 2024

ardaguclu Mar 15, 2024

ardaguclu Mar 15, 2024

deads2k Mar 15, 2024 •

edited

Loading

deads2k Mar 15, 2024

ardaguclu commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

ardaguclu commented Mar 15, 2024

deads2k commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

deads2k commented Mar 18, 2024

ardaguclu commented Mar 18, 2024

openshift-ci bot commented Mar 18, 2024


		Supportability (in terms of authentication flows) matrix of this feature is outlined below;

		\| \| Authorization Code<br/>(public/confidential) \| Authorization Code + PKCE<br/>(public/confidential) \| Implicit Flow<br/>(public) \| Client Credentials<br/>(confidential) \| Refresh Token<br/>(public/confidential) \| Device Code<br/>(public) \| Password Grant<br/>(confidential) \|

		Moreover, `oc login` command triggers authentication process by sending a request to Project API's whoami endpoint automatically so that
		client-go talks to `oc get-token` command to obtain id token (and also refresh token, if provider supports it).

WRKLDS-875: Add oc login external OIDC issuer integration enhancement #1596

WRKLDS-875: Add oc login external OIDC issuer integration enhancement #1596

Conversation

ardaguclu commented Mar 14, 2024

openshift-ci-robot commented Mar 14, 2024 • edited by openshift-ci bot Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardaguclu commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

ardaguclu commented Mar 15, 2024

deads2k commented Mar 15, 2024

openshift-ci bot commented Mar 15, 2024

deads2k commented Mar 18, 2024

ardaguclu commented Mar 18, 2024

openshift-ci bot commented Mar 18, 2024

openshift-ci-robot commented Mar 14, 2024 •

edited by openshift-ci bot

Loading

deads2k Mar 15, 2024 •

edited

Loading