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.

Sign up for GitHub

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

Jump to bottom

Support multiple and dynamic signing certificates #218

Merged
merged 4 commits into from
Nov 1, 2017
Merged

Support multiple and dynamic signing certificates #218

merged 4 commits into from
Nov 1, 2017

Conversation

richjharris
Copy link
Contributor

In our organisation we roll the signing certificate for our ADFS servers every year. During the roll over period the server provides two certificates which may be valid, so when a SAML response is provided it is valid if it is signed with either of those signatures.

This change has two methods that can be used when passport-saml is used by a long running service that will persist over the certificate roll over period. The first change is to allow the cert configuration key to be an array, this allows for the old and new certificates to be checked during the roll over period. The second change is to allow the cert configuration key to be a function that returns the valid certificate or certificates, this allows the service to poll the ADFS server for what the valid certificates are at this moment and update the signing certificates used by passport-saml as they change.

@richjharris richjharris mentioned this pull request Sep 27, 2017
Closed
markstos
markstos reviewed Oct 12, 2017
View reviewed changes
.travis.yml Outdated
@@ -6,9 +6,6 @@ node_js:
- "4.0"
- "stable"

before_install:
- npm install -g npm

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this commit. I'm not sure which older version of Node.js it was trying to support, but we probably don't support it anymore. Recently support for Node.js verisons < 4 was dropped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it was breaking versions < 4 on travis

@markstos
Copy link
Contributor

Hi, I'm a new co-maintainer trying to help out a little.

Please reference the portions of the SAML spec that relate to the changes you are making. If your proposed changes are spec-compliant, I'm inclined to accept a version of this, since supporting multiple certs during a transition seems useful.

Also take a look at the latest version of tests/test.js. I refactored to create TEST_CERT at the top to store a single copy of a cert that was commonly being re-used, so it doesn't need to be copy/pasted throughout the test file, making it harder to read.

At some point soon, we should also think about breaking up the growing test file into multiple files.

@markstos
Copy link
Contributor

It would also be helpful if you could explain how this relates to the other multiple certs PR that was pre-existing: #204. This if branch goes forward, it also needs to updated to resolve conflicts.

@richjharris
Copy link
Contributor Author

@markstos I wasn't aware of #204 until you pointed it out. It is doing part of what is being done in this PR. The differences between #218 and #204 are:

In terms of referencing these changes in the SAML spec I found this:

A relying party SHOULD allow for the use of any of the included keys.

when referring to signing/encryption keys in the SAML metadata https://www.oasis-open.org/committees/download.php/56785/sstc-saml-metadata-errata-2.0-wd-05.pdf 2.4.1.1 [E68].

I am happy to base this change off #204 or go as is, if you can let me know what your preference is.

@markstos
Copy link
Contributor

@richjharris your approach seems more flexible, although the author of #204, @austburn is welcome to weigh in.

One thing I noticed in this PR is when using cert as a function, it's synchronous, but the suggested use is to poll a server, which would be an async operation. So using a callback pattern seems more appropriate there. Or have I missed something about how it's used as a sync function to check for certs?

@austburn
Copy link

It's been a while since I've been active on this change -- initially I threw together #204 to address a looming cert rollover. #218 seems to provide more flexibility and I like the ability to poll for cert changes 👍 . I'm fine if this change represents the availability to support cert rollover as it's supported my many other SAML libs.

@richjharris
Copy link
Contributor Author

@markstos the way I use the function version of the cert option is I have a scheduled task that polls the current valid certs and stores them and in the function which is passed as the cert option I return the certificates which were fetched in the poll. This allows the function to be synchronous.

I've had a look at where validateSignature is called and it wouldn't require any breaking changes to make it async rather than sync if you would prefer it to work that way.

@richjharris
Copy link
Contributor Author

@markstos I've rebased this PR from master but I haven't made any changes on making the function async pending your feedback

@richjharris
Support multiple and dynamic signing certificates
d3a9bae 
In our organisation we roll the signing certificate for our ADFS servers every year.  During the roll over period the server provides two certificates which may be valid, so when a SAML response is provided it is valid if it is signed with either of those signatures.

This change has two methods that can be used when `passport-saml` is used by a long running service that will persist over the certificate roll over period.  The first change is to allow the `cert` configuration key to be an array, this allows for the old and new certificates to be checked during the roll over period.  The second change is to allow the `cert` configuration key to be a function that returns the valid certificate or certificates, this allows the service to poll the ADFS server for what the valid certificates are at this moment and update the signing certificates used by `passport-saml` as they change.
@markstos
Copy link
Contributor

@richjharris Let's make the cert function async to support that usage model.It's easy to make your sync use-case run async, but it's unwieldy if you want to make an async call with a sync API.

@richjharris
Copy link
Contributor Author

@markstos I've pushed a commit to make the cert function async.

@cadesalaberry
Copy link
Contributor

This update looks great ! I would love to see it merged.

@kallelat
Copy link

Indeed something I was just looking for. Hope it gets merged soon!

@markstos
Copy link
Contributor

@richjharris Assuming my doc-update to reflect the async change looks good, I think this is ready to release. 707b315

@cadesalaberry, @kallelat thanks for the peer-feedback. It's helpful to know others are looking for the same feature.

@richjharris
Copy link
Contributor Author

@markstos 👍 looks good.

@markstos markstos merged commit 631aaa7 into node-saml:master Nov 1, 2017
@markstos
Copy link
Contributor

markstos commented Nov 1, 2017 via email

This was referenced Jan 3, 2018
Closed
Closed
cjbarth added a commit to cjbarth/passport-saml that referenced this pull request Sep 10, 2018
@cjbarth
Merge commit 'a94fbfa730dd0aca0e0fa2cedcbabbea1765aee6' into CB/query…
24f5537 
…-string-signing

* commit 'a94fbfa730dd0aca0e0fa2cedcbabbea1765aee6':
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
cjbarth added a commit to cjbarth/passport-saml that referenced this pull request Sep 10, 2018
@cjbarth
Merge commit 'a94fbfa730dd0aca0e0fa2cedcbabbea1765aee6' into session-…
9a5a208 
…url-params

* commit 'a94fbfa730dd0aca0e0fa2cedcbabbea1765aee6':
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
cjbarth added a commit to cjbarth/passport-saml that referenced this pull request Sep 10, 2018
@cjbarth
Merge branch 'master' into session-url-params-on-head
c917ab6 
* master: (51 commits)
  start to use the debug module.
  v0.34.0: release
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
  package.json: bump version to 0.33.0
  docs: mention that disableRequestAuthnContext helps with AD FS
  New Feature: allow customizing the name of the strategy.
  bump version to v0.32.1
  README: link to where our Changes are documented.
  Audience validation
  README: fix typo `s/ADSF/ADFS/`
  jshint: fix jshint violation.
  v0.31.0 release
  README: update link description for ADFS docs.
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
  v0.30.0
  Ignore .tern-port files
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  ...

# Conflicts:
#	README.md
#	test/samlTests.js
#	test/tests.js
cjbarth added a commit to cjbarth/passport-saml that referenced this pull request Sep 10, 2018
@cjbarth
Merge branch 'master' into session-url-params
735509f 
* master: (51 commits)
  start to use the debug module.
  v0.34.0: release
  internal: provide unique failure messages for invalid signatures. Fixes node-saml#146
  package.json: bump version to 0.33.0
  docs: mention that disableRequestAuthnContext helps with AD FS
  New Feature: allow customizing the name of the strategy.
  bump version to v0.32.1
  README: link to where our Changes are documented.
  Audience validation
  README: fix typo `s/ADSF/ADFS/`
  jshint: fix jshint violation.
  v0.31.0 release
  README: update link description for ADFS docs.
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
  v0.30.0
  Ignore .tern-port files
  Use crypto.randomBytes for ID generation (node-saml#235)
  BugFix: Fail gracefully when SAML Response is invalid. Fixes node-saml#238
  docs: Improve docs for privateKey format. Ref node-saml#230
  ...

# Conflicts:
#	README.md
#	test/samlTests.js
cjbarth added a commit to cjbarth/passport-saml that referenced this pull request Sep 11, 2018
@cjbarth
Merge commit 'a94fbfa730dd0aca0e0fa2cedcbabbea1765aee6' into multiple…
c9a1bbf 
…-authn-context

* commit 'a94fbfa730dd0aca0e0fa2cedcbabbea1765aee6':
  Upd: Mention ADFS 2016 with NameIDFormatError. (node-saml#242)
  Support multiple and dynamic signing certificates (node-saml#218)
@vid vid mentioned this pull request Mar 15, 2021
Closed
@devsmart devsmart mentioned this pull request Mar 16, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Mar 26, 2021
Open
@snyk-bot snyk-bot mentioned this pull request May 6, 2021
Open
@cjbarth cjbarth mentioned this pull request Jul 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markstos markstos markstos left review comments

Assignees
No one assigned
Labels
new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@richjharris @markstos @austburn @cadesalaberry @kallelat @cjbarth