Skip to content
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

Discussion: don't default to allowing all users? #609

Closed
manics opened this issue Apr 27, 2023 · 6 comments · Fixed by #625
Closed

Discussion: don't default to allowing all users? #609

manics opened this issue Apr 27, 2023 · 6 comments · Fixed by #625

Comments

@manics
Copy link
Member

manics commented Apr 27, 2023

Proposed change

Don't default to allowing all users to login.

Alternative options

Leave unchanged

Who would use this feature?

Sysadmins who expect a "secure by default" configuration

(Optional): Suggest a solution

As mentioned in jupyterhub/the-littlest-jupyterhub#884 authenticators default to allowing anyone to login. Whilst this has been the case for a while it'd be safer to default to no-one unless explicitly configured, e.g. with a new configurable, or perhaps by using a special placeholder such as allowed_users = "*" (deliberately overriding the configurable to be list-or-string to allow this placeholder string).

The big downside is it's a breaking change, but with the changes in #594 now might be a good time to do this.

@GeorgianaElena
Copy link
Member

I agree this is an unexpected default behavior, though documented in JupyterHub I believe.

I believe it's worth changing its default directly in jupyterhub.auth as I think it would benefit all the authenticators that are used in production deployments and not just oauthenticator.

I expect that it's more often that a hub will be configured as "closed" rather than leaving its access allowed to anyone.

But in order to make a step in this direction I think it's ok to start by releasing this in oauthenticator as this is more likely to be used in prod deployments very often.

@manics
Copy link
Member Author

manics commented Apr 27, 2023

I think it was justified for JupyterHub as originally designed where users also had to have system accounts first- it's reasonable that if a user can login to a system they should have JupyterHub access since they could just run Jupyter notebook from their login shell anyway.

It's much more of a problem for other authenticators, especially ones like GitHub and Google which allow millions of users by default. There are cases where this is intended (e.g. public JupyterHubs which are open to everyone but want some form of identification) but it's unlikely to be what most admins want.

On balance I think changing the parent class default for consistency is justified (and maybe override it in PamAuthenticator only?), but that's a separate discussion. As long as this OAuthenticator change can be directly ported to the parent authenticator there's no downside to making the change here first.

@Bougakov
Copy link

Bougakov commented Apr 28, 2023

As the original issues' author I'd suggest adding a fix to the GitHub and Google Auth methods, not the parent method. It will solve the security implications on a wide scale, but won't add headaches to the admins deploying JupyterHub in the walled corporate or university environments.

An edge case here would be the corporate or free legacy users of https://workspace.google.com - but these orgs/individuals living in the cloud are better to be set safe than sorry by default.

Current setup can inflict serious pain on the individuals. My experience shows that JH+GoogleAuth installs are targeted by a crypto miners, maybe in an automated manner. I was lucky I have a tiny machine on a flat tariff with 1 gig of RAM and a shared CPU. If I were renting a machine with ten Nvidia A100 I'd be in the red for thousands of dollars just in the time it took to figure out what's happening.

@Bougakov
Copy link

But anyway the issue for now deserves an explicit warning in the docs about the security implications, on the same page in the Google Auth and GitHub tutorials. When I followed the manual, it didn't ring any bells that not just the people I've added as admins would have access. Indeed, the reference mentions it, but in a spirit of "on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying ‘Beware of the Leopard.”"

@minrk
Copy link
Member

minrk commented May 4, 2023

I think it's reasonable to change the default in oauthenticator, but probably not upstream.

I agree that it should be communicated better (beyond the warning you already get on every Hub start when this is the case), but I'm not sure I agree the default should change. At least at the Hub level. Maybe my use cases are weird, but I've almost exclusively deployed hubs with GitHub oauth without limiting users, or with institutional providers where all authenticated users should be allowed.

Certainly with PAM (the default Authenticator) allowing any successful login is the most logical and least surprising behavior, not the other way around, and I think that's true for most Authenticator implementations other than oauthenticator.

I do recognize that it's reasonable to be surprised that once some users are specified (I.e. an admin list), that regular users aren't limited yet.

I think it is an issue mostly specific to oauthenticator and not Authenticators in general that it is likely users who successfully authenticate should not be allowed, so that makes me think if it should change, oauthenticator is the place to make allow_all_authenticated opt-in instead of opt-out, which seems a reasonable, safer default and only a very minor inconvenience for folks who want to opt-in to the current default behavior.

@consideRatio
Copy link
Member

I hear agreement on changing the default behavior here, which I also agree with.

Following discussion upstream in jupyterhub/jupyterhub related to adding a config like this in the base class jupyterhub/jupyterhub#4484 (comment), I suggest we move onwards with a new config called allow_all that we implement and declare False.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants