lsh123 / xmlsec

XML Security Library
Other
130 stars 100 forks source link

XMLSec should support multiple certificates with the same name #820

Closed cc-int3 closed 1 month ago

cc-int3 commented 1 month ago

I will write a specific use case, but I think the desired outcome can help in other scenarios which I have not thought of.

I have a project to validate SAML assertions/tokens. Primarily the tokens are issued by ADFS. ADFS uses as self-signed certificate (and associated private key) to digitally sign the xml. About a month before the certificate expires, ADFS will generate a new self-signed certificate/key with the exact same subject, new key and new expiration date and add both old and new certificates to the published metatdata.

When reading the metadata, I put both certificates in the xmlsec certificate store, but when verifying a SAML token, xmlsec will only use one cert from the store for verification. If it happens to get the right one, great. However, if it picks the wrong one, the signature validation will fail, despite the proper cert being available it the store. I feel xmlsec should search the store for additional 'same name' certificates and perform the signature check again if another cert is available.

Thank you for your consideration of this issue.

lsh123 commented 1 month ago

Thanks, I understand the use case. It is a bit complex on xmlsec side since the code assumes that key is uniquely identified in the keys store. Said that -- it's only software. Let me think about it.

lsh123 commented 1 month ago

Want to check something -- is there an option for you to switch from self-signed certificate to a chain?

root cert -> rotating cert

Then you can embed rotating cert into the signature and verify it against root cert.

cc-int3 commented 1 month ago

You can use chained certs, but you lose the automatic rollover feature ( doc )

If you don't use the default, automatically generated, self-signed token signing and token decryption certificates, you must renew and configure these certificates manually.

That's probably a feature a lot of my customers don't want to lose.

lsh123 commented 1 month ago

Oh well, can you try https://github.com/lsh123/xmlsec/pull/827 to see if it works for you?

lsh123 commented 1 month ago

Interestingly, all other crypto libraries except msng / mscrypto already support the same functionality.

cc-int3 commented 1 month ago

Thanks, I have to travel overnight, but I'll test in the morning. Thank you for all the work you've done

cc-int3 commented 1 month ago

Actually, I already had a test written...works great.

thanks again

lsh123 commented 1 month ago

Great, I also have a test but wanted to make sure it works for you real use case. No rush at all -- absolutely not urgent.

cc-int3 commented 1 month ago

cool, i'll see if someone in the office can build/deploy a test version while I'm out

lsh123 commented 1 month ago

@cc-int3 wanted to check if you got a chance to test this?

cc-int3 commented 1 month ago

I have not been able to get the production guys to throw it into a real world test, but my limited testing shows it works. You can close whenever you want. Thank you

On Mon, Jul 29, 2024, 08:39 lsh123 @.***> wrote:

@cc-int3 https://github.com/cc-int3 wanted to check if you got a chance to test this?

— Reply to this email directly, view it on GitHub https://github.com/lsh123/xmlsec/issues/820#issuecomment-2255823586, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEBMRE2TFDL76ISL5CXZDLTZOYZYRAVCNFSM6AAAAABLDCO322VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJVHAZDGNJYGY . You are receiving this because you were mentioned.Message ID: @.***>

lsh123 commented 1 month ago

Thanks!