Skip to content
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 SAML authentication #25165

Merged
merged 113 commits into from
Feb 23, 2024
Merged

Support SAML authentication #25165

merged 113 commits into from
Feb 23, 2024

Conversation

techknowlogick
Copy link
Member

@techknowlogick techknowlogick commented Jun 9, 2023

Closes #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

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 9, 2023
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 9, 2023
@techknowlogick techknowlogick added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/changelog Adds the changelog for a new Gitea version topic/authentication labels Jun 9, 2023
@bwinston-sdp
Copy link

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.

@techknowlogick
Copy link
Member Author

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

@kdumontnu
Copy link
Contributor

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.

@bwinston-sdp
Copy link

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

@garymoon
Copy link
Contributor

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.

@bwinston-sdp
Copy link

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.

@kdumontnu
Copy link
Contributor

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

@6543 6543 requested a review from a team February 22, 2024 19:28
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubber stamp

@6543 6543 enabled auto-merge (squash) February 22, 2024 23:42
@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2024
@6543 6543 added the type/changelog Adds the changelog for a new Gitea version label Feb 22, 2024
@6543 6543 merged commit 5bb8d19 into go-gitea:main Feb 23, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 23, 2024
@techknowlogick
Copy link
Member Author

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

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

image

zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 23, 2024
* giteaofficial/main:
  Start to migrate from `util.OptionalBool` to `optional.Option[bool]` (go-gitea#29329)
  Add slow SQL query warning (go-gitea#27545)
  Unify organizations header (go-gitea#29248)
  Frontport changelogs of minor releases (go-gitea#29337)
  Support SAML authentication (go-gitea#25165)
  Upgrade to fabric 6 (go-gitea#29334)
  Don't show third-party JS errors in production builds (go-gitea#29303)
  Remove bountysource (go-gitea#29330)
  Remove unnecessary "Str2html" modifier from templates (go-gitea#29319)
  Ignore the linux anchor point to avoid linux migrate failure (go-gitea#29295)
  Remove jQuery from the repo commit functions (go-gitea#29230)
  Remove unnecessary "Safe" modifier from templates (go-gitea#29318)
  Remove jQuery from the image pasting functionality (go-gitea#29324)
  Improve the `issue_comment` workflow trigger event (go-gitea#29277)
  Properly migrate automatic merge GitLab comments (go-gitea#27873)
  Refactor cmd setup and remove deadcode (go-gitea#29313)
  small cache when get user id on interation (go-gitea#29296)
  Discard unread data of `git cat-file` (go-gitea#29297)
  Don't install playwright twice (go-gitea#29302)

# Conflicts:
#	templates/home.tmpl
@6543
Copy link
Member

6543 commented Feb 23, 2024

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 added a commit to 6543-forks/gitea that referenced this pull request Feb 24, 2024
lunny pushed a commit that referenced this pull request Feb 24, 2024
This reverts #25165 (5bb8d19), as there
was a chance some important reviews got missed.

so after reverting this patch it will be resubmitted for reviewing again

#25165 (comment)

temporary Open #5512 again
6543 added a commit to 6543-forks/gitea that referenced this pull request Feb 25, 2024
Closes go-gitea#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

---------

Co-authored-by: jackHay22 <jack@allspice.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: morphelinho <morphelinho@users.noreply.github.com>
Co-authored-by: Zettat123 <zettat123@gmail.com>
Co-authored-by: Yarden Shoham <git@yardenshoham.com>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: silverwind <me@silverwind.io>
@6543
Copy link
Member

6543 commented Feb 25, 2024

resubmitted for review: #29403

@6543 6543 added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed type/changelog Adds the changelog for a new Gitea version type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/ui Change the appearance of the Gitea UI modifies/translation topic/authentication size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. modifies/frontend modifies/docs modifies/internal labels Feb 25, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2024
@6543
Copy link
Member

6543 commented Mar 7, 2024

To all reviewers here, pleas rereview this at #29403

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SAML 2.0 as Login-Source (Service Provider)