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

Different certificates (and private keys) for SP signing and encryption #560

Open
pavolzbell opened this issue Dec 13, 2020 · 18 comments · May be fixed by #673
Open

Different certificates (and private keys) for SP signing and encryption #560

pavolzbell opened this issue Dec 13, 2020 · 18 comments · May be fixed by #673

Comments

@pavolzbell
Copy link

Hello, I am facing an IDP who requires different certificates (and private keys) for singing and encryption. Is it possible to configure my SP in such way with this gem?

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 27, 2021

Right now at the toolkit you can only register

  • Multiple IdP public certs
  • 1 SP public cert and SP private key. (and temp an extra SP public cert for Key rotation)

The toolkit publishes the same public cert to allow the IdP to validate Signatures generated by the SP as well as encrypt the SAML Assertions.

Are you sure you are not able to import the generated SP Metadata XML on the IdP side?

@pavolzbell
Copy link
Author

@pitbulk yes, I am absolutely sure that we need different SP certificates and private keys for singing and encryption as this is both required and then verified by the IDP.

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 27, 2021

What software is used on the IdP side?
I 'm not aware of this kind restriction of restriction in the SAML standard.

The toolkit right now does not support registering different pairs to sign and decrypt

@pavolzbell
Copy link
Author

I am not sure what kind of software is on the IDP side and it is not relevant as this is a new restriction placed by Slovak eGov (slovensko.sk) IDP. It will take effect in the upcoming months. In other words we can not do anything about it but obey.

Here is a part of the related docs:

Screenshot 2021-01-27 at 23 15 33

The last (highlighted) sentence translates to "Certificate in SP SAML metadata used for encryption must be different from certificate used for signing."

I have verified this with them (twice) and they will simply not register an SP metadata with the same certificates.

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 27, 2021

I see

P.S Strange that in page 20 appears a screenshot of the regsitered SP and only appears 1 certificate

@pavolzbell
Copy link
Author

P.S Strange that in page 20 appears a screenshot of the regsitered SP and only appears 1 certificate

Well, the docs are always with some mistakes in this case, but there has been an online workshop addressing this and other upcoming changes, so it is absolutely legit.

@pavolzbell
Copy link
Author

@pitbulk anyway, are you willing to accept a PR on this issue? I think we can manage to prepare one in the near future.

@pitbulk
Copy link
Collaborator

pitbulk commented Feb 4, 2021

It depends on how you gonna implement it and how much complexity will add to the toolkit.

@johnnyshields
Copy link
Collaborator

johnnyshields commented Aug 8, 2021

👍 I also encountered this. This is important to be able to implement key rotation on the SP side.

The requirement should be:

  1. Multiple encryption certs AND
  2. Multiple signing certs
  3. Each cert having its own private key

Currently, RubySaml already supports multiple certs on the IdP side using the idp_cert_multi setting. This is a Hash<Symbol, Array<String>> in the format:

setting.idp_cert_multi = { encryption: [cert1, cert2], 
                           signing: [cert3, cert4] }

I would like to propose we add sp_cert_multi, however, we also need to add in the private keys. So perhaps it could look like this Hash<Symbol, Array<Hash<Symbol, String>>> :

setting.sp_cert_multi = { encryption: [ { cert: cert1, private_key: pk1 },
                                        { cert: cert2, private_key: pk2 } ], 
                          signing: [ { cert: cert3, private_key: pk3 },
                                     { cert: cert4, private_key: pk4 } ] }
  • We can retain certificate and private_key as simple aliases; would propose to rename/alias them to sp_cert and sp_private_key for congruency with idp
  • certificate_new should probably be deprecated in favor of sp_cert_multi.

Thoughts / comments? @pitbulk

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 12, 2021

I see the use cases different.

At the SP side, we support multiple IdP's certs in order to be able to validate the Signature of the SAMLResponse. Some IdPs support multiple private_ley/certs at the same time, so the scenario of supporting multiple makes sense not only in the cert rotation scenario.

The key rotation on ruby SAML was implemented in a very easy way.

When there is a new private_key/public cert to be used, the public cert is added as
certificate_new setting. Then all IdPs have time to update its metadata and register the new cert.
After that, once all IdPs are updated, SP admin only needs to move the certificate_new value on certificate and remove the certificate_new setting and at the same time, update the new private_key value. That way, we don't have any interruption on the service.

I don't understand the requirement from the slovensko.sk IDP requiring 2 different SP certs involved on the signature validation and the encryption process, to be honest. There is no other SAML profile with this kind of requirement. See saml2int

@johnnyshields
Copy link
Collaborator

johnnyshields commented Aug 17, 2021

I don't understand the requirement from the slovensko.sk IDP requiring 2 different SP certs involved on the signature validation and the encryption process, to be honest. There is no other SAML profile with this kind of requirement. See saml2int

The fact that SAML allows different signing and encryption certs, however silly it may be in practice, still means we ought to support it. The APIs provided by this gem should be relatively unopinionated (within reason)

@johnnyshields
Copy link
Collaborator

@pitbulk it appears governments are requiring this, even if it is silly in practice, it means the poor schmucks developers implementing SAML will need to address this.

@johnnyshields
Copy link
Collaborator

@pitbulk any further thoughts on this?

@johnnyshields
Copy link
Collaborator

@pavolzbell @jsuchal I've implemented this here: #673

I've added lots of test cases so I'm fairly confident in it. Please try it out.

@ducthien1490
Copy link

@johnnyshields : My project also needs signing/encryption certificates, when will it release?

@johnnyshields
Copy link
Collaborator

johnnyshields commented May 20, 2024

@ducthien1490 you can use my branch here. The merge/release is up to @pitbulk as the project maintainer.

@pitbulk
Copy link
Collaborator

pitbulk commented May 20, 2024

I plan to add this on next release.

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

Successfully merging a pull request may close this issue.

5 participants