Skip to content

fix google idp saml redirect url - #6451

Closed
neogismm wants to merge 1 commit into
apache:mainfrom
neogismm:fix-saml-googleIdp-redirect-url
Closed

fix google idp saml redirect url#6451
neogismm wants to merge 1 commit into
apache:mainfrom
neogismm:fix-saml-googleIdp-redirect-url

Conversation

@neogismm

@neogismmneogismm commented Jun 11, 2022•
edited
Loading

Copy link
Copy Markdown

Fixes#6427

This PR fixes the redirect URL for SAML authentication while using Google as IdP.

Note: I could not test my fix because choosing google as idp for a user requires the SSO url which also contains the unique IDP id created by google.

Example:
SSO URL: https://accounts.google.com/o/saml2/idp?dpid={IDP-ID}
Entity ID: https://accounts.google.com/o/saml2?idpid={IDP-ID}

To set up a custom google saml app requires a google workspace account which was not possible for me to create at the moment.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@boring-cyborg

boring-cyborgBot commented Jun 11, 2022

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

@neogismmneogismm changed the title fix for google idp saml redirect urlfix google idp saml redirect urlJun 11, 2022
@sonarqubecloud

sonarqubecloudBot commented Jun 11, 2022

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed! Quality Gate passed

BugA0 Bugs
VulnerabilityA0 Vulnerabilities
Security HotspotA0 Security Hotspots
Code SmellA0 Code Smells

33.3%33.3% Coverage
0.0%0.0% Duplication

@nvazqueznvazquez added this to the 4.17.1.0 milestone Jun 13, 2022
nvazquez
nvazquez reviewed Jun 13, 2022
View reviewed changes
Comment threadplugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java
}
if (idpMetadata.getEntityId().startsWith("https://accounts.google.com/o/saml2?idpid=")) {
redirectUrl = idpMetadata.getSsoUrl() + SAMLUtils.generateSAMLRequestSignature(SAMLUtils.encodeSAMLRequest(authnRequest), privateKey, signatureAlgorithm);
}

@nvazqueznvazquezJun 13, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need an else block here? Otherwise the redirectUrl will be overridden on the next line

@nvazquez

nvazquez commented Jun 13, 2022

Copy link
Copy Markdown
Contributor

Thanks @neogismm can you please rebase and target the PR to the 4.17 branch?

yadvr
yadvr reviewed Jun 13, 2022
View reviewed changes
Comment threadplugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAMLUtils.java
if (spMetadata.getKeyPair() != null) {
privateKey = spMetadata.getKeyPair().getPrivate();
}
if (idpMetadata.getEntityId().startsWith("https://accounts.google.com/o/saml2?idpid=")) {

@yadvryadvrJun 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we use URIBuilder instead, https://hc.apache.org/httpcomponents-core-5.1.x/5.1.1/httpcore5/apidocs/org/apache/hc/core5/net/URIBuilder.html
... or similar that helps generate the url based on a base URL and query params.

@yadvryadvrJun 13, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or search for ? in the idp entity url and based on that use correct concatenation strategy? (+ plus add unit tests)

@Luis-3MLuis-3MJun 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

searching for ? in the IdP url should be sufficient, since we only care about appending the SAML request query param to the URL.

#6457

@nvazqueznvazquez mentioned this pull request Jun 15, 2022
Merged
12 tasks
@nvazquez

nvazquez commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

@Luis-3M@neogismm it seems we have 2 PRs solving the same issue, can you work on unifying them? (#6457)

@neogismm

neogismm commented Jun 16, 2022

Copy link
Copy Markdown
Author

Closing this PR as @Luis-3M's PR has a better solution and also includes the tests.

@neogismm
neogismm deleted the fix-saml-googleIdp-redirect-url branch June 16, 2022 06:10
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvryadvryadvr left review comments
@nvazqueznvazqueznvazquez left review comments
+1 more reviewer
@Luis-3MLuis-3MLuis-3M left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:samltype:bug

Projects

None yet

Milestone

4.17.1.0

Development

Successfully merging this pull request may close these issues.

Broken SAML SSO plugin with malformed URL when authenticating against Google IdP

4 participants

@neogismm@nvazquez@yadvr@Luis-3M