gitea
gitea copied to clipboard
feat(auth): add SAML
Closes https://github.com/go-gitea/gitea/issues/5512
This PR adds basic SAML support
- Adds SAML 2.0 as an auth source
- Adds SAML configuration documentation
- Adds integration test:
- Use bare-bones SAML IdP to test protocol flow and test account is linked successfully (only runs on Postgres by default)
- Adds documentation for configuring and running SAML integration test locally
Future PRs:
- Support group mapping
- Support auto-registration (account linking)
Co-Authored-By: @jackHay22
FYI, I'm doing some testing of this against a CAS IdP. A few things I'm noticing so far:
- It may make sense to surface to the user (and let them change, if applicable) the login endpoint and/or the request type (unless Gitea will only do one of GET or POST). For example, our IdP metadata provides several binding locations depending on the type of request being made, and it looks like the current impl just picks the first one.
- Ditto pulling the signing cert and NameIDFormat out of the metadata.
- I also echo what @wfjake mentioned about the SignedAuthnRequest -- I believe that should be populated with the "Sign SAML Requests" checkbox, and perhaps not have the "Skip Assertion Signature" option for the reasons mentioned.
I've not gotten a successful handshake to work yet (getting an "invalid transform" from the AuthN signature on my IdP, but I'm thinking that's an IdP setting), but I'm imagining that ultimately the response is validated (somehow) against the "Authentication Sign-In Name" on the "Edit User" page (my original thought was matching NameIDs but that obv won't work for transients). Were you thinking of the ability to map specific response attributes to Gitea user fields? And/or auto-provisioning?
I'm happy to start picking away at the things I mentioned above, I just don't want to step on any toes.
@bwinston-sdp thanks so much for testing out this PR, especially against the idp you are using. I've just spun up a simple idp for my testing so having an actual one is so helpful.
Should this be part of 1.21 milestone? Anything we can do to help support it? We're also getting a significant number of requests for SAML support.
Don't want to step on @techknowlogick or @wfjake toes, but from my perspective, if the freeze is still targeted for 9/3 (per #25123), I don't see this being ready. I'm happy to do more testing and/or development as time permits, but my August is not forgiving and so I'll likely not be able to contribute as much as I'd like pre-1.21.
I also don't know what the Gitea methodology is for incorporating features like this -- I find SAML integrations to be wildly different in complexity and what is supported. Would you want "Gitea supports SAML any which way", or "Gitea supports SAML, so long as you do it in this specific way"? I like the latter for simplicity (and this PR approaches that), but in my experience SAML tends to be used by larger institutions, who often like the ability to pull various levers. It may make sense to just try and get the simple thing over the finish line, which may bring folks who are interested in lever-pulling out of the woodwork to enhance.
As far as support, I think the biggest thing I can think of that would help move this along is testing/feedback from folks using various IdPs (ideally bigger ones -- Azure, Okta, Clever, etc). Maybe from some of the many requestors :)
I have run this PR against OKTA and the round-trip went smoothly but there's not much to test beyond that with the callback being unimplemented (unless I'm missing something?). I did have some issues with the UI while configuring it, which I can provide feedback on if we're okay devitating to minutia at this stage.
For those interested in testing big players themselves, OKTA offers a free developer account with most features available. This is how I test against it.
Thanks @garymoon! You're not missing anything -- without the callback there's not much else to test from a login perspective, it's mostly just configuration at this point.
@techknowlogick this is increasing in priority for us - do you have any additional development on that that needs to be pushed? What are else needs to be completed to get this initial version merged? Are you open to stacked branches here?
@techknowlogick Anything I can contribute to help with this feature?
Yeah, I would like to use this with Authentnik (https://goauthentik.io/) ... how can I help ?
Is this ready for testing can I get a preview somewhere ?
Sry if asking the wrong questions here a bit new to the helping part !
@senare you can use authentik already w/ Gitea, I think you can use the oidc connector.
for anyone else following this, I'm meeting up w/ @jackHay22 (and a few others) tomorrow, who will hopefully take this over the finish line.
@techknowlogick See https://github.com/techknowlogick/gitea/pull/1963
@techknowlogick See https://github.com/techknowlogick/gitea/pull/1964
After https://github.com/techknowlogick/gitea/pull/1965 is merged, can you update the PR description to be a little more description to something like:
Closes https://github.com/go-gitea/gitea/issues/5512
This PR adds basic SAML support
- Adds SAML 2.0 as an auth source
- [To Do] Adds SAML configuration documentation
- Adds integration test:
- Use bare-bones SAML IdP to test protocol flow and test account is linked successfully (only runs on Postgres by default)
- Adds documentation for configuring and running SAML integration test locally
Future PRs:
- Support group mapping
- Support auto-registration (account linking)
@techknowlogick I think there is a merge issue or something else causing lint to fail. Can you also remove the "To Do" from the subject now that we merged sub-pr#1968? Also v1.22.0 milestone?
@bwinston-sdp @mxctx-jake This SAML implementation should be ready to go. We've done some validation on it ourselves. You seem to have some good experience with SAML, so it would be great to get your review if you're able!
@techknowlogick This PR should fix the CI failure: https://github.com/techknowlogick/gitea/pull/1969
There are still a few TODOs on the code need to be finished.
The complete TODOs are now removed. The remaining TODOs are for features that can still be added later and are also now included in the documentation.
Some lint failures.
@6543 there was no last call on this, and we should've given the reviewers who blocked it a chance to review again without dismissing them or at least pinging them. This is an XXL sized PR, and we shouldn't rush to merge them.
I'll revert to give everyone an appropriate chance to review, and ask follow up questions.
I'm getting a 500 error when reverting this, so I'll try again in a few hours.
the reviews todos where addresed and i do prevere a followup pull if issues get discovered to be addressed. But if you want to revert and redo reviewing it i'm also fine with that
resubmitted for review: #29403
To all reviewers here, pleas rereview this at https://github.com/go-gitea/gitea/pull/29403