search

OpenConext / OpenConext-engineblock

OpenConext SAML 2.0 IdP/SP Gateway
14 stars 22 forks source link

/metadata endpoint should expose all the available keys #1223

Open tvdijen opened 1 year ago

tvdijen commented 1 year ago

The IDP- and SP-metadata endpoints should add KeyDescriptor-elements for all (or all non-expired) available keys to accommodate for a key-rollover. This has been discussed multiple times, but every time I have to do key rollover I run into this issue, esp. because we sign our AuthnRequests towards IDPs. See this old thread; https://groups.google.com/g/openconext/c/1oexBnx5KHA/m/Z9V9vYWIBwAJ

thijskh commented 1 year ago

The status quo is that engineblock metadata generation does not support this classic SAML keyrollover. Engineblock will accept them, so if you generate xml that includes both keys you can perform this key rollover - you can just not use the Engineblock built in generated metadata currently.

Engineblock does support a gradual key rollover for SP's (IdP endpoint) via generated metadata that lists a key and an SSO location with that key ID in the same metadata. This means that any SP consuming that metadata has a working combination. This has the advantage that the rollover is gradual (SPs that consume the new metadata will be using the new key immediately) and more importantly that one can monitor progress of phase out of the old key and which SPs still use it.

Adding all configured keys to the generated metadata endpoints should be possible. We just need to ensure not to break the gradual rollover scenario. Endpoints with a specific keyID should therefore only contain the respective key.

tvdijen commented 1 year ago

I don't see how classic key-rollover can 'bite' the Dutch way.? Anyway, reading your response I get the feeling I'm being misunderstood; SP's are not my concern and the Dutch way is usually fine.

The problem is that we sign our AuthnRequests towards IdP's and during a rollover we need the IdP's to be aware of both the old and the new cert. The Dutch way doesn't apply to IdP's, and so I think the key-slugs don't make any sense on the SP-metadata and we actually need a metadata file containing both old+new keys (EB SP metadata, but I see no harm in providing one for EB IdP metadata as well).

@ArnoutvdKnaap looked into this and the Twig-templates seem to be aware of the fact that the md:KeyDescriptor can be an array, but the array never contains more than one key.