openshift oauth (PROJQUAY-673) #473
Conversation
|
Can one of the admins verify this patch? |
|
Re-submission of #321 with a bunch of things fixed. |
mosen
force-pushed
the
PROJQUAY-673-openshift-oauth
branch
from
c393561
to
c4345a7
Compare
|
Rebased over quay/quay master as requested and force pushed |
|
@alecmerdler tom says to ping you regarding this PR |
|
@mosen Looks like some tests are failing, but I'll check this out later today and try running it. |
|
@alecmerdler yep this one is my fault, eg |
|
Getting back on this after a vacation. Update soon |
Add oauth.services.openshift based mostly on oidc.py Add a basic test Make necessary provisions for a new config item called OPENSHIFT_LOGIN_CONFIG
PROJQUAY-673: Add OpenShift Config Validator class PROJQUAY-673: Add OpenShift Authentication Documentation via openshift-authentication.md
PROJQUAY-673: do not rely on the presence of the config var OPENSHIFT_SERVER PROJQUAY-673: more explanation in markdown docs
…e network as it seems the openshift.default.svc endpoint is untrusted.
PROJQUAY-673: documentation explains troubleshooting of incorrect redirect uri
…nge_code_for_login() so they don't diverge at some point.
PROJQUAY-673: add more notes about caveats RE emails PROJQUAY-673: change config variable for API endpoint to OPENSHIFT_API_URL as it makes more sense.
mosen
force-pushed
the
PROJQUAY-673-openshift-oauth
branch
from
c4345a7
to
dceb868
Compare
|
Rebased on master, will fix CI test failures |
|
@alecmerdler clear to review |
|
Any insight into when we may see this PR get reviewed and merged in? This is a feature my team is waiting for. Thanks! |
|
@werne2j I spent some time attempting to deploy and run this on an OpenShift cluster using the new Quay Operator, and I may have done something incorrectly because I ran into an error. I will swing back to testing this again soon. |
|
happy to give it a test if you can point me to the error. There's a bit of setup on the openshift side so im happy to walk through that. |
|
@mosen: PR needs rebase. 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. |
|
Won't rebase unless its considered for merge |
HammerMeetNail
changed the title
|
hey @mosen, did you have any plans of implementing |
|
Not at this time. I left the team using red hat quay. Unsure where you want to go from here |
|
Gotcha. I'm probing to team about getting this moving, but can't say where we'll go from here just yet. Will keep you posted. |
|
@flavianmissi Any updates regarding this feature? We are looking into authentication options for Quay and we are very interested in this feature. |
|
I'd probably be happy to rebase again. I'm kinda worried about dumping code on the core team and then they have to support it but there's nothing super magical included. |
|
@mosen Thank you for your comment. I have direct contacts within Red Hat. I can ask them to review your code to see if they can support this in the Quay team(s). Do you think this is a good way to get this rolling within the Quay project? |
|
I brought this up internally with the team - will report back when I have news. |
|
Our PM is positive towards this feature. @mosen I just noticed this has some config app related changes - we have moved the config app to its own repository: https://github.com/quay/config-tool/tree/master/pkg/lib/editor/js/core-config-setup, so we'll need a PR against that repo for the configuration related changes. |
|
Hi @mosen Thanks for submitting this. I have a few comments. The OAuth implementation looks correct at a quick glance, although I have not yet tested it. As @flavianmissi mentioned, the config app and the validation logic have been moved to the config-tool repository. Your changes to the front end can be copied to that repo, as for the validator logic we have rewritten that tool in Go so there may be some rewrites there. I would say reduce this PR to just the OAuth implementation and it should be good for testing. |
flavianmissi
requested review from
jonathankingfc
and removed request for
josephschorr and
alecmerdler
|
Red Hat is pushing the OpenShift Platform Plus which bundles Quay on OCP. This feature is a Quality of Life feature that aligns well with the with the goal of the OPP bundle. I'm very interested to see OpenShift OAuth added to Quay. |
Description of Changes
This change adds features and configuration for integrating with the OpenShift 3.x/4.x integrated OAuth service.
When using the config app, the administrator is presented with the option to configure Integrated OAuth, such that logging into the OpenShift console confers access to Project Quay.
The functionality will not impact Project Quay permissions at this time. The user is authenticated to Quay but there will be no further provisioning. This is regarded as beyond the scope of this change.
As OpenShift does not store e-mail addresses. This functionality will not work if the
MAILINGfeature is enabled.The end user motivation for the change: To provide a seamless experience when deploying Project Quay into an OpenShift v3/v4 cluster so that no further configuration of Authentication is required, unless there is a requirement to separate Project Quay from the operation of the cluster.
The current behaviour: When deploying Project Quay into an OpenShift cluster, none of the available authentication methods support integration with the OpenShift RBAC/console identity. It is up to the user to establish an LDAP or OIDC connection via an external IdP which matches their OpenShift identity.
Changes:
Issue: PROJQUAY-673
TESTING ->
Manually validated against OpenShift 4.4.10
We could possibly look at a pytest-docker which brings up the openshift-oauth proxy as a standalone container in the CI if that was required.
BREAKING CHANGE ->
New functionality, only modifications made are to the config schema.
Reviewer Checklist