-
-
Notifications
You must be signed in to change notification settings - Fork 54
WIP: auth: implement SSO using OIDC #816
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
base: development
Are you sure you want to change the base?
Conversation
|
Note: I do not feel comfortable doing the UI part, so I would request assistance on that part. |
|
Thanks for opening this PR. From reading the code alone I am a bit lost on what exactly this is trying to achieve. Could you add a few sentences about the "user story" this is trying to cover? This will also help us flesh out ideas around UI and docs and all of that. |
|
In order to enhance the user experience of offen when an OIDC provider
is available by implementing Single Sign-On (SSO) with OpenID Connect
(OIDC), this way users can seamlessly log in with their existing
credentials without the need for multiple logins. Especially in
commercial deployments, Domain Controllers or other forms of SSO like
Keycloak are rather common.
From a users perspective:
- visit offen.offen.dev/login/
- click on "Login with SSO"-Button
- they are redirected to the issuer (for example a Keycloak server)
- they log in with their existing credentials
- they are redirected back to offen
- their account is provisioned if it does not exist already
- they are logged in
From an operator's perspective:
- add required values (well-known-address, client id, client secret)
- offen now starts in "SSO"-mode, which entails the following:
- no login with username and password
- no password reset
- if someone logs in with an email that does not have an
account, an account is created transparently
Excuse if this is a bit rough, my last proper user story was a few months ago.
…--
Moritz Poldrack
https://moritz.sh
This space intentionally left blank.
|
Haha, no worries, this is perfect already and helps a lot. I'll have a look and think about it a bit. It's a welcome feature in any case. |
|
|
||
| require ( | ||
| golang.org/x/oauth2 v0.15.0 // indirect | ||
| mpldr.codes/oidc v0.0.0-20231223203712-a59dee5fc440 |
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.
If I understand it correctly, this package is both authored and hosted by you. I don't mind this, but I'd be interested in learning more about:
- why you chose to write such a package when others exist
- what happens if your server goes away and this application still references the package using your domain. Does the Go package proxy kick in?
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.
The package proxy kicks in. Worst case scenario is that there has to be a replace directive in the go.mod that points to the backing repo over at https://git.sr.ht/~mpldr/oidc
The main reason for writing it was to make OIDC stupid easy to use, since I've run into it a few times now. OIDC is basically oauth2, but the token contains extra details by default and the plan is to expose them in an easy to use manner.
Basically: you shouldn't have to understand how OIDC is implemented, what the tokens are signed with, and all that fun stuff.
| api.POST("/share-account", accountAuth, rt.postShareAccount) | ||
| api.POST("/join", rt.postJoin) | ||
| } else { | ||
| api.POST("/login", rt.oauthLogin) |
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.
Would there be any harm in always exposing these routes in addition to the existing ones and just 40x out when someone calls them and OIDC is not configured? I would assume the URLs could be namespaced like /oidc/login or something.
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.
Sure, namespacing them would be easy. My main thought was: if the operator has an issuer set up, they likely won't use the internal login anyway, so there's no need for the form to be shown. One of my thoughts regarding the UI part was that it might be the easiest option to just use the "Login as operator" button as the "use SSO" button, which is why it's right now colliding.
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.
Hmm, then maybe let's defer the decision here until we know what the UI will look like. I'll talk about this with @hendr-ik beginning of next year and we'll feedback on how to proceed here.
|
Currently, an How would this work when using OIDC? Would someone who logs in the first time still create a |
| sha512Hash.Write([]byte(email)) | ||
| sha512Hash.Write([]byte(salt)) |
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.
Write could potentially return an error, so this should be checked and handled as well.
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 can not. see crypto/sha512/sha512.go:256
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.
Even if the current implementation does never assign a value to err, the signature still tells us it could do so at any time when someone changes the code.
I think it makes sense to treat the (stable) signature as the contract, not the (unstable) current implementation.
I do know this is a merely theoretical concern, but I think it makes sense to check errors if the signature tells us an error could be returned. Else, you'd need to start having intricate knowledge about all your dependencies and how they actually implement the things they are supposed to do.
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.
While I agree in principle, in this case the only reason it would fail was if there were issues in writing to memory. And if that happens, you have bigger issues. The error return is simply part of the io.Writer interface, and thus required to satisfy it. Hashing static data isn't something that can "fail".
|
Thank you for the review. In OIDC something like this is returned: {
"iss": "https://id.moritz.sh/application/o/codimd/",
"sub": "1af2deecbcff74e06f140b10373d0907f3436f0c9451fe24df5c5e5963ebeb29",
"aud": "b7bFpUvCVbUN00eNCFrNI6dyaYpCJcwuWT2gtCzm",
"exp": 1704413169,
"iat": 1704411369,
"auth_time": 1704411369,
"acr": "goauthentik.io/providers/oauth2/default",
"amr": [
"pwd",
"mfa"
],
"email": "moritz@poldrack.dev",
"email_verified": true,
"name": "Moritz Poldrack",
"given_name": "Moritz Poldrack",
"preferred_username": "moritz",
"nickname": "moritz",
"groups": [
"admin",
"users",
"vpn",
]
}Custom properties are of course possible, but the most reasonable approach to me seems to be using groups for this. If someone logs in for the first time, a "default" account is created for them. If, for example, a user was a member of the |
|
The interesting question probably is: should a deployment support users signing in with OIDC and default user accounts, or should a deployment always use a single method. In case the latter is used, how does an operator configure to do what they want it to do? I could see how the former is possible, but maybe it can get very confusing and is not a real world use case. Another thing I was wondering: in case a deployment supports OIDC only, how is the first user account even created. Is it just the first person signing in using OIDC? Should there be a special workflow for doing this? |
I would argue that it's the latter. That's pretty much the entire point. This would also allow getting rid of all kinds of potential edge-cases in user interactions.
Yes. Ideally, the first user shouldn't be anything special. Going by depolyment reality, it would likely be an admin anyway. |
I agree. In that case, however we might need to come up with a mechanism that "locks" a deployment to a single method of sign in and prevents it from being changed in the lifetime of such a deployment. I'll think about this for a bit, but if you have any ideas on the topic, feel free to voice them here. |
|
So today I met with @hendr-ik and we talked about how to get this feature shipped. The plan would be to complete the following steps: Server side functionalityThis chunk of work would need to be finished first, ideally by you @mpldr Delegate Login to OIDC providerThis is what you have already implemented here, so that's probably to be considered done. Devise mechanism that locks a deployment to a certain method of loginAs previously discussed in this PR, it would make sense to lock a deployment to a single method of managing users. Once a deployment has users that have been created using the default method, it should not be possible to change the login mechanism anymore and vice versa. On a technical level, this could look like this: a Add an endpoint to the server that lets the UI read "capabilities"The Auditorium will now need to know which login screen display to users based on the server configuration. This could be done in two ways:
Based upon this, the screens for Unit testsNew code should be accompanied by unit tests as far as this is possible. Update AuditoriumOnce this is done, we can work on the additions for the user interface. This will probably be worked on by @hendr-ik Additional option for
|
|
Closing this as it's stale. In case you want to revive it, please feel free to reopen. |
|
I will likely be able to get back to it in May :) |
|
@m90 May has turned into June, I can't seem to reopen it, though. Would you mind doing the honors? |
The approach I've chosen is adding the subject to the account information. For performance-reasons, since the setup only happens on startup, this would require a restart, though. This seems like an unnecessary requirement to me, so what I would suggest is using SSO as a sort of "extension":
Thoughts welcome. |
I'm not entirely sure I understand yet. Does this mean you could have SSO user accounts as well as password user accounts in parallel as long as password login isn't disabled in the config? |
|
On Fri 21 Jun 2024 00:51:02, Frederik Ring wrote:
I'm not entirely sure I understand yet. Does this mean you could have SSO user accounts as well as password user accounts in parallel as long as password login isn't disabled in the config?
The main idea is providing a "migration path" for users who wish to
introduce SSO into an existing system, but it would also reduce the
complexity of the implementation itself.
…--
Moritz Poldrack
https://moritz.sh
Serving suggestion.
|
|
Two things I still don't fully understand assuming OIDC is an "extension":
|
|
On Sun 23 Jun 2024 04:44:23, Frederik Ring wrote:
- how does creation of the first account on setup work in this scenario?
As now. It can then be linked to a SSO subject
- when OIDC is not configured, what happens to UI and such?
The signin-with-SSO button isn't shown and no option to link the account
is provided. As for how to achieve that, I would leave the details on
how to do this to someone who knows what they're doing UI-wise and would
happily assist in getting the data to the frontend.
…--
Moritz Poldrack
https://moritz.sh
If not completely satisfied, return for full refund of purchase price.
|
I start to understand, and it makes sense to me yes. Is there anything you need to implement it in that fashion still? |
Mostly information on what the frontend would need/want. |
|
I would think most of what's written in here #816 (comment) still applies, apart from the things that say "either OIDC or password-based" are now "password-based and OIDC if configured". |
This adds a basic implementation of SSO using OIDC.
It is missing:
Providing any of these would be much appreciated.