gitea icon indicating copy to clipboard operation
gitea copied to clipboard

feat(auth): add SAML

Open techknowlogick opened this issue 2 years ago • 16 comments

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

techknowlogick avatar Jun 09 '23 03:06 techknowlogick

FYI, I'm doing some testing of this against a CAS IdP. A few things I'm noticing so far:

  1. 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.
  2. Ditto pulling the signing cert and NameIDFormat out of the metadata.
  3. 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 avatar Jul 07 '23 21:07 bwinston-sdp

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

techknowlogick avatar Jul 07 '23 21:07 techknowlogick

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.

kdumontnu avatar Aug 08 '23 15:08 kdumontnu

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

bwinston-sdp avatar Aug 10 '23 15:08 bwinston-sdp

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.

garymoon avatar Aug 10 '23 16:08 garymoon

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.

bwinston-sdp avatar Aug 10 '23 16:08 bwinston-sdp

@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?

kdumontnu avatar Aug 25 '23 15:08 kdumontnu

@techknowlogick Anything I can contribute to help with this feature?

jackHay22 avatar Sep 12 '23 18:09 jackHay22

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 avatar Sep 25 '23 14:09 senare

@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 avatar Sep 26 '23 01:09 techknowlogick

@techknowlogick See https://github.com/techknowlogick/gitea/pull/1963

jackHay22 avatar Oct 02 '23 19:10 jackHay22

@techknowlogick See https://github.com/techknowlogick/gitea/pull/1964

jackHay22 avatar Oct 30 '23 19:10 jackHay22

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)

kdumontnu avatar Nov 30 '23 17:11 kdumontnu

@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!

kdumontnu avatar Dec 21 '23 17:12 kdumontnu

@techknowlogick This PR should fix the CI failure: https://github.com/techknowlogick/gitea/pull/1969

jackHay22 avatar Dec 22 '23 16:12 jackHay22

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.

kdumontnu avatar Jan 23 '24 20:01 kdumontnu

Some lint failures.

silverwind avatar Feb 16 '24 21:02 silverwind

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

techknowlogick avatar Feb 23 '24 03:02 techknowlogick

I'm getting a 500 error when reverting this, so I'll try again in a few hours.

image

techknowlogick avatar Feb 23 '24 03:02 techknowlogick

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

6543 avatar Feb 23 '24 04:02 6543

resubmitted for review: #29403

6543 avatar Feb 25 '24 19:02 6543

To all reviewers here, pleas rereview this at https://github.com/go-gitea/gitea/pull/29403

6543 avatar Mar 07 '24 17:03 6543