Skip to content

Conversation

@mpldr
Copy link

@mpldr mpldr commented Dec 28, 2023

This adds a basic implementation of SSO using OIDC.

It is missing:

  • UI
  • Documentation
  • Feedback

Providing any of these would be much appreciated.

@mpldr
Copy link
Author

mpldr commented Dec 28, 2023

Note: I do not feel comfortable doing the UI part, so I would request assistance on that part.

@m90
Copy link
Member

m90 commented Dec 28, 2023

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.

@mpldr
Copy link
Author

mpldr commented Dec 28, 2023 via email

@m90
Copy link
Member

m90 commented Dec 28, 2023

Excuse if this is a bit rough, my last proper user story was a few months ago.

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
Copy link
Member

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?

Copy link
Author

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)
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

@m90
Copy link
Member

m90 commented Jan 4, 2024

Currently, an AccountUser as stored in the database has several properties like an AdminLevel and AccountUserRelationships, tying the user to the accounts they are able to access.

How would this work when using OIDC? Would someone who logs in the first time still create a AccountUser record that can then be used to describe their access?

Comment on lines +20 to +21
sha512Hash.Write([]byte(email))
sha512Hash.Write([]byte(salt))
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

@m90 m90 Jan 5, 2024

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.

Copy link
Author

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".

@mpldr
Copy link
Author

mpldr commented Jan 4, 2024

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 admin group, they should then get extended permissions. Regarding the AccountUserRelationships, this would be handled as it normally would, since OIDC creates a normal account. It's just that the possibilities for a login are limited.

@m90
Copy link
Member

m90 commented Jan 5, 2024

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?

@mpldr
Copy link
Author

mpldr commented Jan 5, 2024

should a deployment support users signing in with OIDC and default user accounts, or should a deployment always use a single method

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.

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?

Yes. Ideally, the first user shouldn't be anything special. Going by depolyment reality, it would likely be an admin anyway.

@m90
Copy link
Member

m90 commented Jan 5, 2024

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.

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.

@m90
Copy link
Member

m90 commented Jan 12, 2024

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 functionality

This chunk of work would need to be finished first, ideally by you @mpldr

Delegate Login to OIDC provider

This 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 login

As 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 type (or something similar) field is added to the AccountUser model. This holds either default or oidc. Each time a new account user is created, the deployment checks whether existing users are of the same type. In case they aren't, account creation is rejected. This means the first user created determines which method of login is used for a deployment. In addition to that it would be possible to check configuration on startup and try to match it with the users that exist in the database. In case there is a mismatch, the application would refuse to boot, which is probably helpful for users that inadvertently change their configuration to something unexpected.

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:

  • when rendering the HTML for the Auditorium, the server adds a window.capabilities object containing information about login methods and other possibly other things that the SPA can then read and act upon
  • a /capabilities endpoint is added to the JSON api which is then called once by the SPA

Based upon this, the screens for /login and /setup can then choose what to display.

Unit tests

New code should be accompanied by unit tests as far as this is possible.

Update Auditorium

Once 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 /login and /setup

When OIDC is used, render a single "Log in with OIDC" button instead of the login/setup form.

Do not render "change password" and "change email" fields in Admin console

These options make no sense when using OIDC and should be replaced by a message about how this deployment's account users are managed using OIDC.

Update translation strings

Once all of this is done, we'll need to reach out to our translators and have the new strings translated.

Update docs

This can be done in parallel to working on the Auditorium by @m90 .

We want to avoid pushing new users towards decision paralysis when trying out Offen, so we thought it makes sense to be low key about the new option and default to the current mechanism in all tutorials. In addition to that, a detailed article explains how OIDC can be used, including all of the caveats that come with it.


Let me know if this makes sense for you @mpldr and if you need any further feedback from our end to wrap up the inital chunk of work on this topic.

@m90
Copy link
Member

m90 commented Apr 4, 2024

Closing this as it's stale. In case you want to revive it, please feel free to reopen.

@m90 m90 closed this Apr 4, 2024
@mpldr
Copy link
Author

mpldr commented Apr 4, 2024

I will likely be able to get back to it in May :)

@mpldr
Copy link
Author

mpldr commented Jun 4, 2024

@m90 May has turned into June, I can't seem to reopen it, though. Would you mind doing the honors?

@m90 m90 reopened this Jun 4, 2024
@mpldr
Copy link
Author

mpldr commented Jun 18, 2024

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.

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":

  • "Default" is still a normal account
  • the account can be linked to an SSO-subject, but that disables "normal" login by overriding the password with a secure autogenerated one
  • Password-changes are then disabled. Accounts are identified by a non-empty subject field.
  • Normal Login can be disabled using an environment variable

Thoughts welcome.

@m90
Copy link
Member

m90 commented Jun 21, 2024

so what I would suggest is using SSO as a sort of "extension"

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?

@mpldr
Copy link
Author

mpldr commented Jun 22, 2024 via email

@m90
Copy link
Member

m90 commented Jun 23, 2024

Two things I still don't fully understand assuming OIDC is an "extension":

  • how does creation of the first account on setup work in this scenario?
  • when OIDC is not configured, what happens to UI and such?

@mpldr
Copy link
Author

mpldr commented Jun 23, 2024 via email

@m90
Copy link
Member

m90 commented Jun 26, 2024

As now. It can then be linked to a SSO subject

I start to understand, and it makes sense to me yes. Is there anything you need to implement it in that fashion still?

@mpldr
Copy link
Author

mpldr commented Jun 27, 2024

Is there anything you need to implement it in that fashion still?

Mostly information on what the frontend would need/want.

@m90
Copy link
Member

m90 commented Jun 27, 2024

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants