Enhancement for OAuth2 Authorization Grant Login for oc #955
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
|
||
| This process can be simplified to the following steps: | ||
|
|
||
| 1. User runs `oc login --auto $CLUSTER_API_URL` |
There was a problem hiding this comment.
oc login --code-grant maybe?
Why would I have to supply $CLUSTER_API_URL?
There was a problem hiding this comment.
When opening the authorization URL the client(oc) would need the domain and the path. When the path and the domain are not modified it can derive that from the base domain. If it is it needs the extra parameter. Also this is similar to how oc login --token $TOKEN $CLUSTER_API_URL works right now.
There was a problem hiding this comment.
I see, so this is just an optional argument, please have it in []
There was a problem hiding this comment.
So just to clarify the $CLUSTER_API_URL is required to do the first login. On subsequent logins the URL can be retrieved from the existing kubeconfig. Correct?
arjunrn
force-pushed
the
login-workflow
branch
3 times, most recently
from
d8916e7
to
11e8a58
Compare
|
/uncc |
|
|
||
| ## Summary | ||
|
|
||
| The current CLI login process involves multiple interactive steps where the user has to enter credentials multiple |
There was a problem hiding this comment.
I think the summary is mixing several auth flows:
ocdoes not require user to enter credentials multiple times- web console does required multiple logins when going through oauth
- in certain cases it is required to login into web console and copy the login command to
oc
Please clarify this, since you're focusing on no. 3 here.
There was a problem hiding this comment.
oc may not require it but the user has to perform login at least twice to get the token after which they can login/configure oc. Yes oc does support login with username/password but only if a suitable identity provider is present. In most cases the identity provider may not support such a login. I will update the Summary to highlight this.
| 2. Login to console with the configured identity provider. | ||
| 3. Click on the **"Copy Login Command"** button which redirects the page to the OAuth application. | ||
| 4. Perform login again like in step 2. | ||
| 5. Copy the displayed login command. |
There was a problem hiding this comment.
So again, you're describing no. 2 and no. 3 from my above list, which process you're addressing with this proposal. I'd like you to make it clear.
There was a problem hiding this comment.
These step in themselves are not problems, just steps. The proposal aims to provide a way to side-step them for a faster login process.
|
|
||
| #### _oc login_ command | ||
|
|
||
| The oc login command will have a new flag `--auto` which indicates to `oc` that it should use the Oauth2 Authorization |
There was a problem hiding this comment.
Why do we need a new flag, if this is improving the UX I'm fine having this as the default with eventual option for opt-out. I'd like to see pros & cons laid out for opt-in or opt-out.
There was a problem hiding this comment.
IIUC making this the default would change existing behavior. The existing behavior is to call the authorization endpoint and set the redirect URI to the implicit token endpoint and if there is an identity provider which supports challenge based authentication it takes the username and password passed along with the login command and performs authorization. @stlaz please correct me if I'm wrong.
That said it might be possible for the server to indicate how to proceed and then use that to determine behavior. Let me investigate further.
There was a problem hiding this comment.
That's why I asked for alternatives and their pros & cons, so that we know in the future what was considered and why we did what we did :)
There was a problem hiding this comment.
I"ve explained why the new flow cannot be the default behavior and why a new flag is required.
There was a problem hiding this comment.
If the user is capable of supplying any valid credentials (username/password, client cert, Kerberos ticket) that are requested, you probably shouldn't fallback to this but may consider the cases where this would be the good default.
There was a problem hiding this comment.
There already is default behavior in oc even when no credentials are provided(prompt for credentials) I don't think it's possible to have the new login flow as default in any scenario.
There was a problem hiding this comment.
Rename --auto to --browser, maybe? --auto really does sound confusing.
It'd be nice to be able to default to this in case there are no HTTP challenges/redirects from the oauth-server, instead of printing a message for the users to go to login to the oauth-server. That means:
- the kubeadmin user is disabled
- no IdPs are configured as challengers
Basically, we're talking this case: https://github.com/openshift/oauth-server/blob/b5790c9a24f45ca817628498dbab8091e29528a6/pkg/oauthserver/auth.go#L423-L426
Do you think it would still not be possible even for that?
There was a problem hiding this comment.
It'd be nice to be able to default to this in case there are no HTTP challenges/redirects from the oauth-server, instead of printing a message for the users to go to login to the oauth-server
What about the inverse case. When there are identity providers which support challenges but the user wants to log in with the browser because their identity is present in an identity provider which doesn't support challenges. This would would mean a flag is required. So if we support the fallback case and the flag case that means there are 2 ways launch a browser for login. And one if them is conditional. I'm not sure that is the best user experience.
There was a problem hiding this comment.
I'm not sure if --browser is the most suitable name, but I can't think of anything better yet, so let's go with that for now. If I come up with something better, we can always change that during implementation.
| for OAuth Grant Flow. Therefore, another client is required where the `respondWithChallenges` is set to `false` which | ||
| should also be provisioned by the operator. | ||
|
|
||
| ### Risks and Mitigations |
There was a problem hiding this comment.
What are the risks, especially when we go the default in oc vs an opt-in.
There was a problem hiding this comment.
No limitations. Tested by cross compiling a simple server which listens on port 8080 and running it on Windows and MacOS.
There was a problem hiding this comment.
Say so in the enhancement, don't leave empty sections for second guessing.
There was a problem hiding this comment.
Added details about how if an OS prevents oc from opening a listener on a certain port an error message will be displayed.
| CVO does not currently delete resources that no longer exist in | ||
| the target version. | ||
|
|
||
| ### Version Skew Strategy |
There was a problem hiding this comment.
This is a must for oc since we support +/- 1 version compatibility
There was a problem hiding this comment.
Added some details here.
arjunrn
force-pushed
the
login-workflow
branch
from
11e8a58
to
802060d
Compare
arjunrn
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
arjunrn
force-pushed
the
login-workflow
branch
from
802060d
to
145b299
Compare
|
|
||
| #### _oc login_ command | ||
|
|
||
| The oc login command will have a new flag `--auto` which indicates to `oc` that it should use the Oauth2 Authorization |
There was a problem hiding this comment.
That's why I asked for alternatives and their pros & cons, so that we know in the future what was considered and why we did what we did :)
| for OAuth Grant Flow. Therefore, another client is required where the `respondWithChallenges` is set to `false` which | ||
| should also be provisioned by the operator. | ||
|
|
||
| ### Risks and Mitigations |
There was a problem hiding this comment.
Say so in the enhancement, don't leave empty sections for second guessing.
|
|
||
| This is where to call out areas of the design that require closure before deciding to implement the design. For | ||
| instance, | ||
| > 1. This requires exposing previously private resources which contain sensitive information. Can we do this? |
There was a problem hiding this comment.
That looks like a security risk to me.
There was a problem hiding this comment.
I've added how callback to localhost can be a risk and how it's mitigated with PKCE.
arjunrn
force-pushed
the
login-workflow
branch
from
145b299
to
e3bfdc1
Compare
|
Not able to figure out how do I pass Identity Provider flags and values through |
@Anandnatraj I looked into this and there's no straight-forward way to specify the identity provider when calling the Openshift oauth-server. Since the oauth-server manages the identity providers internally and only exposes endpoints with parameters which is specified in the authorization endpoint spec. It then displays a list of identity providers to the user. So after this enhancement is implemented the user will be shown the list of configured IDPs and they can select one and proceed. But maybe there's a way to specify that with the |
There was a problem hiding this comment.
please read https://www.rfc-editor.org/rfc/rfc8252.html and apply it to this enhancement
|
|
||
| #### _oc login_ command | ||
|
|
||
| The oc login command will have a new flag `--auto` which indicates to `oc` that it should use the Oauth2 Authorization |
There was a problem hiding this comment.
If the user is capable of supplying any valid credentials (username/password, client cert, Kerberos ticket) that are requested, you probably shouldn't fallback to this but may consider the cases where this would be the good default.
arjunrn
force-pushed
the
login-workflow
branch
2 times, most recently
from
4695506
to
fd6be83
Compare
|
|
||
| #### _oc login_ command | ||
|
|
||
| The oc login command will have a new flag `--auto` which indicates to `oc` that it should use the Oauth2 Authorization |
There was a problem hiding this comment.
Rename --auto to --browser, maybe? --auto really does sound confusing.
It'd be nice to be able to default to this in case there are no HTTP challenges/redirects from the oauth-server, instead of printing a message for the users to go to login to the oauth-server. That means:
- the kubeadmin user is disabled
- no IdPs are configured as challengers
Basically, we're talking this case: https://github.com/openshift/oauth-server/blob/b5790c9a24f45ca817628498dbab8091e29528a6/pkg/oauthserver/auth.go#L423-L426
Do you think it would still not be possible even for that?
arjunrn
force-pushed
the
login-workflow
branch
from
fd6be83
to
ca736de
Compare
arjunrn
force-pushed
the
login-workflow
branch
from
ca736de
to
acf8e0a
Compare
|
|
||
| #### _oc login_ command | ||
|
|
||
| The oc login command will have a new flag `--auto` which indicates to `oc` that it should use the Oauth2 Authorization |
There was a problem hiding this comment.
I'm not sure if --browser is the most suitable name, but I can't think of anything better yet, so let's go with that for now. If I come up with something better, we can always change that during implementation.
|
|
||
| Because this login workflow relies on a browser being launched and some user interaction in the browser it is not | ||
| trivial to implement an end-to-end test which tests the entire workflow. However, the existing unit tests for the _oc | ||
| login_ command can be extended to verify the following scenarios: |
There was a problem hiding this comment.
Make sure QA adds a manual test case to ensure this flow always works.
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: soltysh 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
arjunrn
force-pushed
the
login-workflow
branch
from
acf8e0a
to
f0d5fde
Compare
|
|
||
| ### Goals | ||
|
|
||
| Provide a single-step login in `oc` for identity providers that don't post HTTP challenges. |
There was a problem hiding this comment.
For me the phrasing "single-step" confused me completely. As far as I can tell there are more steps for OAuth2 for Native Apps:
- Native App triggers flow (oc login)
- Browser opens
- if not logged in, user needs to enter credentials
- User needs to approve the resources to be shared
- redirect
There was a problem hiding this comment.
What I meant to say in there was - provide a way for users to login such that they only have to authenticate once. Currently users have to open the console, then click on the login command button which opens the token page where they have to login all over again before they copy the command and paste it into the console. The new approach instead means that if they user wants to login again to a cluster they have previous logged into again all they have to do is run oc login --browser and authenticate if required. The oc client will then be reconfigured automatically with the new token.
If you can think of a good way of describing this in the goal I'm don't mind changing it. I understand it's not the best description.
Add enhancement for OAuth2 Authorization Grant login for oc with PKCE. Only contains Summary, Motivation and Implementation sections.
arjunrn
force-pushed
the
login-workflow
branch
from
f0d5fde
to
79f549a
Compare
|
@arjunrn: 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. |
|
/lgtm |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Add enhancement for OAuth2 Authorization Grant login for oc with PKCE. Only contains Summary, Motivation and Implementation sections.