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

Implement SAML authentication for GL3 #5476

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ithanil
Copy link
Contributor

@Ithanil Ithanil commented Oct 20, 2023

This PR enables SAML authentication for GL3 and is based on similar PRs for GL2: #1334 and #2270 . We are using this feature in production, but it was not developed with the goal to be merged upstream. Because SAML for GL2 was never merged, I guess it won't happen for GL3 as well. However, I'd like to provide our development to the public for anyone in need of this.

This is the part of a series of similar PR submissions (Redis Sentinel, SAML integration, LDAP integration).

Details / How to use:

  • SAML is added analogous to OIDC via omniauth
  • OIDC takes precedence, so configure any OIDC variables if you want to use SAML
  • Documention on configuration is provided in sample.env and SAMLConfiguration.md
  • Notably, some configuration may be obtained from IDP metadata, especially SAML_IDP_URL, SAML_IDP_CERT_FINGERPRINT and the SAML_NAME_IDENTIFIER, so try to not configure these manually if possible
  • The PR also removes any Sign-Up buttons if external authentication is used

@farhatahmad
Copy link
Collaborator

Thanks for these PR's - I will definitely keep these open and will mark them as approved by maintainers for others who are interested in deploying these

@Ithanil
Copy link
Contributor Author

Ithanil commented Oct 23, 2023

@farhatahmad Thank you, that's great.

FYI: I have made another PR #5480 , which is a subset of this one here. It is designed to be easily mergeable and lessen the burden of maintaining custom provider extensions. Please let me know your thoughts on that one.

Ithanil

This comment was marked as duplicate.

# Re-write LDAP and Google to greenlight
user_hash[:provider] = %w[greenlight ldap google openid_connect].include?(user_hash[:provider]) ? 'greenlight' : user_hash[:provider]
# Re-write list of providers to greenlight
user_hash[:provider] = %w[greenlight ldap google openid_connect saml].include?(user_hash[:provider]) ? 'greenlight' : user_hash[:provider]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that SAML is in this list, the V2 migration task should work without modification. Beforehand I had a modification in place which did match the provider to greenlight already in the migration task.

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@hex-m
Copy link

hex-m commented Sep 19, 2024

Common vulnerability with ruby projects implementing SAML: https://ssoready.com/blog/engineering/ruby-saml-pwned-by-xml-signature-wrapping-attacks/

(Not a dev - can't say if this implementation is affected)

@Ithanil
Copy link
Contributor Author

Ithanil commented Sep 19, 2024

Common vulnerability with ruby projects implementing SAML: https://ssoready.com/blog/engineering/ruby-saml-pwned-by-xml-signature-wrapping-attacks/

(Not a dev - can't say if this implementation is affected)

Many thanks for the heads-up. Indeed, it appears as if this PR is implementation is affected. I will update the branch later, bumping omniauth-saml to 2.2.1 and ruby-saml to 1.17.0.

EDIT: Fixed by 8eb2638

Copy link

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

Successfully merging this pull request may close these issues.

3 participants