-
Notifications
You must be signed in to change notification settings - Fork 66
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
Full Cilogon integration #1089
Full Cilogon integration #1089
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice to me! A few comments and questions. I'll let others on the @2i2c-org/tech-team give a more technical review!
- Could we add the start of some lightweight docs for how we'd use CILogon auth? I think it could be helpful exercise to think about the changes from a user's perspective. It doesn't need to be totally complete since I know you'd like to get the PR in quickly, but something to help people start would be useful.
- Given what you've seen, are there any major decision-points that you can think of where we'd want to use Auth0 vs. CILogon?
- Have you encountered any major downsides or concerns about this approach? Or have you encountered any major benefits?
config/clusters/2i2c/cluster.yaml
Outdated
connection: google-oauth2 | ||
enabled: false | ||
# connection: google-oauth2 | ||
cilogon: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we are allowing them to co-exist for a bit! As long as we don't have too much headaches maintaining both, this should give us time to try out CILogon without disrupting our current workflows as much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this, @GeorgianaElena!!!!
The primary reason the auth0 stuff was baked into the deployer was so we won't have to keep secret files in the repo. However, given that is not possible with CILogon (we can not retrieve secret key multiple times), I think we should instead change this to something where the script just spits out appropriate sops encrypted files as enc-<hub-name>.secret.values.yaml
file (just using the CILogon authenticator), which we can just check-in. This also simplifies the deployer (once we can get rid of auth0 at least), and enhances right to replicate.
So we could have something like a new-hub script or cookie-cutter that talks to the CILogon API, generates the appropriate secrets and puts it in place. I think the code for this already exists here.
Once we move off auth0 completely, we can just get rid of the dynamic auth part of the deployer completely.
Thanks for the feedback @yuvipanda. I just pushed some changes with the cilogon client provider turned into a script. Also, weird thing, I just got a 500 error with a message saying that the cilogon credentials I was trying to use don't have an "admin" role. @choldgraf do you mind asking the CILogon folks if they've revoked our admin credentials? I'm afraid they think we've abused their system since I've created quite a few dummy clients while testing the implementation 😕
|
@GeorgianaElena will send them an email today! |
Thanks a lot @choldgraf 🌼 |
@GeorgianaElena i haven't done a deep dive into the code but overall this seems the right direction to me! |
for more information, see https://pre-commit.ci
Co-authored-by: Sarah Gibson <[email protected]>
…ams are not implemented
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@2i2c-org/tech-team, this should be now ready for a new review 👀 or a merge |
We can put default values there even if they aren't enabled
@GeorgianaElena i've left some suggested changes in GeorgianaElena#2! I'm going to now test deploying this on the demo hub. |
GeorgianaElena#3 is me trying to deploy this to the demo hub, but ran into problems :( I've detailed them in the PR. I think if we can resolve this, this is good to go. This is such a huge step forward, thank you for working on this @GeorgianaElena! |
The test failure is a link to a file introduced in this PR, so is alright to let fail. |
Suggested changes
Co-authored-by: Georgiana Elena <[email protected]>
Move demo hub to use cilogon directly
Continue to rely on username_pattern right now as we figure out how to use IDPs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgianaElena this is super awesome, am very excited to get this to land!
- Remove allowed_idps for demo hub GeorgianaElena/pilot-hubs#4 keeps status quo for demo hub, and we can figure out what to do with IDPs
- Remove CILogonOAuthenticator default scopes GeorgianaElena/pilot-hubs#5 removes some extra config
- Can you update the PR description here too?
I think you can merge after these two are done.
These are already actually set in the base CILogonOAuthenticator itself! https://github.com/jupyterhub/oauthenticator/blob/8c1cf0c616d5e19d1f93238673f17db2daf57fa0/oauthenticator/cilogon.py#L69
Remove allowed_idps for demo hub
Remove CILogonOAuthenticator default scopes
Merging this now! THANK YOU! 🚀 |
Lovely piece of work, @GeorgianaElena!! |
This PR adds another way to use CILogon auth for our hubs, one that doesn't require relying on Auth0.
It adds a script,
cilogon_app.py
that:create/udpate/delete/get
approved OAuth applications for our hubs.The process to activate CILogon for a hub using the credentials generated above is very similar to the current process of using GitHubOAuthenticator.
Also, this PR enables CILogon for our
2i2c dask-staging
hub and2i2c demo
hub. It also adds credentials for the 2i2c staging hub also, but without activating it for this hub.The
dask-staging
hub uses this, so you can play with it. It works from what I've tested so far.Reference #967