-
Notifications
You must be signed in to change notification settings - Fork 40
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
Support for SAML as a Silo IdP, part 2 #1210
Conversation
This PR adds the second half of the SAML login: accepting the IdP's SAML response, creating (or looking up) a silo user, and establishing a session token for them. A silo identity provider will send authenticated users to Nexus along with some form of credentials, or Nexus will query an identity provider in order to determine if user-supplied credentials are correct. In the SAML case, the SAML IdP will POST credentials to Nexus. Silos now have a "user provision type" setting: if set to "fixed", users must be defined before the SAML IdP POSTs the SAMLResponse. If set to "jit", users will be created if they do not exist. A Silo's identity provider consumes an IdP response and emits an "authenticated subject". This authenticated subject is in terms of the IdP's realm, and contains an external ID along with groups that the subject is part of. Nexus will create or lookup a silo user based on that external id. Silo users are mapped to external subjects using this external id, and as a result, silo users now have an "external_id" field. This is a one to one mapping and assumes that the external id will not change. Future work: - add silo groups - get cached information from a user's session when logging in - SAML logout flow (requires samael crate PR)
common/src/sql/dbinit.sql
Outdated
time_deleted TIMESTAMPTZ, | ||
|
||
silo_id UUID NOT NULL, | ||
external_id TEXT |
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 is their username with the IdP, right? like their company email address?
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.
it could be that, it could also be some pseudo-random id - see section 8.3 of http://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf. we can't rely on this being anything legible like an email or username.
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 see. It's whatever they choose. It is not a display name.
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.
Although, we could say that if the type is emailAddress
then it could be used as a display name.
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.
Won't we be storing other metadata about the user that will let us reliably get display names? I think we need that, unless we're able to hit IdP endpoints for user data instead (which I think would have to be proxied through our API instead of hit directly from the client).
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.
When we JIT users like this, there's no other information other than what's in the name format. We also can't query a SAML IdP because that's not how SAML works - the IdP only sends us SAMLResponses. If we supported SCIM then users could be provisioned but we don't yet.
changed silo user index to not include id, as that already is the primary key
…kup and pop plus test
silo_user_from_authenticated_subject now accepts authz::Silo and db::model::Silo instead of a silo name.
silo_user_create now returns the user returned from diesel::insert_into.
@@ -260,7 +270,7 @@ pub async fn login( | |||
.to_str() | |||
.map_err(|e| { | |||
HttpError::for_internal_error(format!( | |||
"{}", | |||
"referer header to_str failed! {}", |
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.
super nitty but I imagine this should be a 400 for providing an unsupported header?
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.
Yeah, that makes sense. I originally thought that the user wouldn't control the referer header, but of course they do, because they navigate to a URL in the first place. Changed in d66038f
default send to / instead of organizations
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.
(meant to approve this earlier, sorry)
This PR adds the second half of the SAML login: accepting the IdP's SAML
response, creating (or looking up) a silo user, and establishing a
session token for them.
A silo identity provider will send authenticated users to Nexus along
with some form of credentials, or Nexus will query an identity provider
in order to determine if user-supplied credentials are correct. In the
SAML case, the SAML IdP will POST credentials to Nexus.
Silos now have a "user provision type" setting: if set to "fixed", users
must be defined before the SAML IdP POSTs the SAMLResponse. If set to
"jit", users will be created if they do not exist.
A Silo's identity provider consumes an IdP response and emits an
"authenticated subject". This authenticated subject is in terms of the
IdP's realm, and contains an external ID along with groups that the
subject is part of. Nexus will create or lookup a silo user based on
that external id. Silo users are mapped to external subjects using this
external id, and as a result, silo users now have an "external_id"
field. This is a one to one mapping and assumes that the external id
will not change.
Future work: