[CILogon] Restrict access based on domain for username claims different than email

[GeorgianaElena]

Context

Currently, the CILogonOAuthenticator can restrict access based on domains, for usernames containing an @ 👉🏼

oauthenticator/oauthenticator/cilogon.py

Line 341 in b84fe8c

gotten_name, gotten_domain = username.split('@')

There are two small issues with this approach:

• It is incomplete, as the current approach assumes an email claim is used for the username, but this condition is not enforced.
• It doesn't cover the cases where a different claim than email claim is used for the username, but domain based restriction is still desired.

Proposed change

• Allow using the email claim to restrict user's access even when other claims are user for the hub username.

Who would use this feature?

The use-case that this come up in 2i2c land was the following:

• a hub is using the oidc claim for usernames, so that they are opaque for user privacy
• user access should still be granted based on user's domain
• otherwise the alternative solutions would be: use a manually-manged allowed list or leave the hub open

(Optional): Suggest a solution

[yuvipanda]

YESSS! I think perhaps an allowed_email_domains or something that can be configured on a per-IDP basis?

[consideRatio]

I propose we add allowed_domains_claim, letting it default to the configured username_claim if left unspecified. What do you think @GeorgianaElena and @yuvipanda?

[GeorgianaElena]

@consideRatio, I don't know if this would work? A domain makes sense if we're talking about the email claim, or any other claim that matches a regex like .*@.* (From https://www.cilogon.org/oidc#h.p_PEQXL8QUjsQm it looks like eppn and affiliation), I think?

And at the moment we can specify idp.allowed_domains even if idp.username_claim is not email. More, when evaluating access based on idp.allowed_domains, we're using idp.username_claim which is not correct when there is no domain in that claim and this will fail with an error that's not very suggestive for what's going on.

If we default this new config allowed_domains_claim to username_claim, it would fail in the same way as it does right now.

Instead, I believe with this new config allowed_domains_claim it makes sense to either:

• use emailas default
• make this a config that is mandatory to define together with allowed_domains, otherwise fail to set allowed_domains

WDYT?

[yuvipanda]

Given that you two have been deep in the weeds of OAuthanticator, I'll leave it to y'all to figure out :)

[consideRatio]

A domains check or any kind of check?

@consideRatio, I don't know if this would work? A domain makes sense if we're talking about the email claim, or any other claim that matches a regex like .*@.* (From https://www.cilogon.org/oidc#h.p_PEQXL8QUjsQm it looks like eppn and affiliation), I think?

This has a history around domains specifically, and I think its too complicated to transition this config to generalize. For example, we have assumed that we handled "allowed domains", and then we can probably strip any parts before @ and ignore the case - but if we don't handle domains we can't! Due to this, I think we should stick to that this configuration is entirely about domain names etc. For anything beyond it, I I think we should add separate options instead.

Change proposal discussed

If we default this new config allowed_domains_claim to username_claim, it would fail in the same way as it does right now.

It would fail, but it would be possible to re-configure it to succeed in such situations.

Instead, I believe with this new config allowed_domains_claim it makes sense to either:

• use emailas default
• make this a config that is mandatory to define together with allowed_domains, otherwise fail to set allowed_domains

These are both breaking changes, and I don't want to make a breaking change yet. I suggest that we add allowed_domains_claim to default to using the same claim as the username to begin with, but that we change this default to email before the next major release.

@GeorgianaElena what do you think about that approach? If so, we could get a 16.2.0 release out quite soon and have oauthenticator bumped in z2jh etc quite soon as well.

[GeorgianaElena]

@consideRatio, personally, I am not convinced that half-solving an issue for the sake of synchronizing releases with other projects is the way to go, especially since we don't have a formal process set in place about it.

But you are more engaged in z2jh and understand better the release process of that project, so I'm +1 on whatever you decide to do next in terms of releases. Let me know if there's anything I can do to help.

[consideRatio]

Thank you @GeorgianaElena for the quick response!

I tried to make a few implicit ideas about major releases explicit in jupyterhub/team-compass#696, where I think this change is suitable to be phased in by being opt-in initially before it ships out to everyone by default.

[consideRatio]

If we default this new config allowed_domains_claim to username_claim, it would fail in the same way as it does right now.

Instead, I believe with this new config allowed_domains_claim it makes sense to either:

• use emailas default
• make this a config that is mandatory to define together with allowed_domains, otherwise fail to set allowed_domains

Thinking about email as default

If we would switch to email as default, it could be a security issue and its hard to rule out its not. We need to check allowed_domains against a trusted user-to-domain association - can we rely on email to be that? I'm thinking for example if username_claim has been configured to eppn - then could email be different and possibly not verified etc?

About mandatory to define

I think making the new config mandatory could be reasonable. I appreciate how doing this would improve config readability and reduce implementation complexity a small bit. A downside is that it would require an intervention from users upgrading and be a breaking change forcing users attention when upgrading.

---

@GeorgianaElena do you want us to take another action point as part of this issue?

[GeorgianaElena]

@GeorgianaElena do you want us to take another action point as part of this issue?

@consideRatio, no, I believe we can close this issue!