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

Allow setting private signing key in SAML Service #4407

Merged
merged 11 commits into from Nov 10, 2019

Conversation

@tsschmidt
Copy link
Contributor

tsschmidt commented Nov 4, 2019

PR adds issuerSigningKeyLocation and issuerSigningAlgorithm to SamlRegisterdService. Setting theses fields allows request for the SP to use a different signing key then the IdP default. Useful for services such as Google Apps SSO integration.

@apereocas-bot apereocas-bot added this to the 6.2.0-RC1 milestone Nov 4, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 5, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@0fc14f3). Click here to learn what that means.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4407   +/-   ##
=========================================
  Coverage          ?   32.18%           
  Complexity        ?     4859           
=========================================
  Files             ?     2580           
  Lines             ?    52430           
  Branches          ?     4167           
=========================================
  Hits              ?    16874           
  Misses            ?    34258           
  Partials          ?     1298
Impacted Files Coverage Δ Complexity Δ
...s/support/saml/services/SamlRegisteredService.java 0% <ø> (ø) 0 <0> (?)
.../idp/profile/builders/enc/SamlIdPObjectSigner.java 72.28% <38.46%> (ø) 25 <1> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fc14f3...05105d0. Read the comment docs.

if (location.startsWith(ResourceUtils.CLASSPATH_URL_PREFIX)) {
return new ClassPathResource(StringUtils.removeStart(location, ResourceUtils.CLASSPATH_URL_PREFIX));
}
if (location.startsWith(ResourceUtils.FILE_URL_PREFIX)) {
return new FileSystemResource(StringUtils.removeStart(location, ResourceUtils.FILE_URL_PREFIX));
}
return new FileSystemResource(location);
Comment on lines +417 to +423

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Nov 5, 2019

Member

This is a good start, but it needs a few more changes. SAML2 idp metadata and/or keys should always be fetched from a locator instance. It would be contradictory to store metadata artifacts in a database while requiring a signing key for a service to be available on disk. We'll need to modify locator instances to allow for this behavior at the API level first, and then sort of make them service-aware...and this change needs to be applied to all locator types across the codebase.

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Nov 5, 2019

Member

For now, my suggestion would be to simply back out the signing-key changes until the API work is done and keep the service signing-algorithm changes.

private String issuerSigningKeyLocation;

@Column
private String issuerSigningAlgorithm;

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Nov 5, 2019

Member

Change this to "signingKeyAlgorithm".

This comment has been minimized.

Copy link
@mmoayyed

mmoayyed Nov 5, 2019

Member

Also, please document the field and its purpose and add a few test cases to demonstrate how the algorithm override may work.

Copy link
Member

mmoayyed left a comment

See inline comments please.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 5, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@7ada803). Click here to learn what that means.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4407   +/-   ##
=========================================
  Coverage          ?   36.33%           
  Complexity        ?     5794           
=========================================
  Files             ?     2580           
  Lines             ?    52465           
  Branches          ?     4173           
=========================================
  Hits              ?    19063           
  Misses            ?    31824           
  Partials          ?     1578
Impacted Files Coverage Δ Complexity Δ
...s/support/saml/services/SamlRegisteredService.java 0% <ø> (ø) 0 <0> (?)
.../idp/profile/builders/enc/SamlIdPObjectSigner.java 72.28% <38.46%> (ø) 25 <1> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ada803...1fa145e. Read the comment docs.

@mmoayyed mmoayyed merged commit 5c10a3e into master Nov 10, 2019
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Summary 2 potential rules
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@mmoayyed mmoayyed deleted the issuer-private-key branch Nov 10, 2019
@mmoayyed

This comment has been minimized.

Copy link
Member

mmoayyed commented Nov 11, 2019

This is now merged, with some follow-up improvements to allow metadata overrides per service. Should be functional for the most part, and I will continue to test and clean up, with a follow-up blog post. Thanks @tsschmidt.

At some point in the future and once this is tested out, I think we ought to begin deprecating (v6.2/6.3) and removing the google-apps module (v7.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.