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

Add sp_cert_multi to facilitate SP cert/key rotation #673

Merged
merged 1 commit into from
Jul 7, 2024

Conversation

johnnyshields
Copy link
Collaborator

@johnnyshields johnnyshields commented Nov 25, 2023

Fixes #560

This PR introduces sp_cert_multi parameter which is analogous to idp_cert_multi. It allows developers to have fine-grained control over SP certs and private keys, including:

  1. Allow SP to have separate SP signing and encryption keys (required by some governmental regulations.)
  2. Allow SP to rotate SP private keys, not just certificates (for example, if key becomes compromised.)
  3. Allow SP to have more than two concurrent pairs of SP certs/keys in rotation at once.

The changes are summarized as follows:

  1. Add SamlSettings sp_cert_multi parameter. It has the following shape:

    sp_cert_multi = {
      signing: [
        { certificate: cert1, private_key: private_key1 },
        { certificate: cert2, private_key: private_key2 }
      ],
      encryption: [
        { certificate: cert1, private_key: private_key1 },
        { certificate: cert3, private_key: private_key3 }
      ],
    }

    (Note: You can use same certs for signing/encryption, and same PK everywhere. It's completely backward compatible with current functionality.)

  2. sp_cert_multi is mutually exclusive with the following: certificate, certificate_new, private_key.

  3. If security[:check_sp_cert_expiry] is true, Ruby Saml automatically uses the first non-expired certificate in sp_cert_multi[:signing] for signing, and only uses private keys associated with non-expired certs in sp_cert_multi[:encryption] for decryption. This is evaluated in realtime, so as soon as your old cert expires your app automatically starts signing with the new one.

  4. The validation error :check_sp_cert_expiration is now raised only if ALL SP certs are expired. This is a slight behavior change;

    • Previously if Settings.certificate was expired but Settings.certificate_new was not, an error would be raised.
    • In this PR this case does not raise an error and starts automatically using certificate_new for signing. (This case was not previously in the tests, but I've now added a test for it with the new logic.)
  5. :check_sp_cert_expiration now also validates the certificate not_before condition; previously it was only validating not_after.

  6. If :check_sp_cert_expiration is true, we now no longer include expired certs in the generated SP metadata. This is a good practice because having expired certs may cause the IdP system to throw an error, depending on how strictly it does its validation.

    • Point of discussion: It's debatable if we want to include "not_before" certs here (i.e. not yet ready to use, but ok for IdP to pre-cache and use when ready.) For simplicity's sake I've excluded them, especially because "not_before" is relatively rare in the wild.
  7. Refactor so that internal references to get_sp_cert, get_sp_private_key, etc. now point to the new structure of multiple certs.

  8. When performing decryption, we now try all private keys under sp_cert_multi[:encryption] (this is analogous to how we try all IDP certs in idp_cert_multi[:signing] when verifiying the IDP signature.)

  9. Extract out OneLogin::RubySaml::Utils.build_cert_object and build_private_key_object.

  10. Deprecate the certificate_new parameter since sp_cert_multi fulfills the same role better. It still works but it is removed from the docs.

  11. When there are multiple SP certs, the ordering of SP KeyDescriptor node in the SP metadata XML will now be all signing keys first, and then all encryption keys. (Previously it would be signing, encryption, signing, encryption.) This does not affect XML integrity in any way.

  12. This PR contains unit tests and integration tests for all major SP signing flows (both Redirect and POST). Decryption is covered as well.

@johnnyshields johnnyshields marked this pull request as draft November 25, 2023 11:24
@johnnyshields johnnyshields changed the title Add sp_cert_multi to facilitate key rotation Add sp_cert_multi to facilitate SP cert/key rotation Nov 25, 2023
@johnnyshields johnnyshields marked this pull request as ready for review November 25, 2023 14:41
@johnnyshields
Copy link
Collaborator Author

johnnyshields commented Nov 25, 2023

@pitbulk this is ready for final review. Let me know what I can do to help get this merged.

@johnnyshields
Copy link
Collaborator Author

FYI I am using this in production now without issues.

@oliverguenther
Copy link

I was just looking into how to build multi_cert support for SP signing myself and great to see you already did it @johnnyshields . We're interested in getting this upstream, anything we can support in to move this forward?

@johnnyshields
Copy link
Collaborator Author

@pitbulk what do you think?

@pitbulk
Copy link
Collaborator

pitbulk commented May 20, 2024

I will be adding this on next ruby-saml release. Hopefully soon.

@johnnyshields
Copy link
Collaborator Author

@pitbulk any update? I've been using this in prod for 6 different SAML integrations, with IdPs on Azure AD, PingFederate, etc. I think it's safe to merge.

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 7, 2024

Sorry for the delay on merging it, I will be more active now with ruby-saml

@pitbulk pitbulk merged commit f40c59b into SAML-Toolkits:master Jul 7, 2024
47 checks passed
@oliverguenther
Copy link

Excellent news, thank you!

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

Successfully merging this pull request may close these issues.

Different certificates (and private keys) for SP signing and encryption
3 participants