Investigate how to configure specific certificates for SAML and OIDC

NicolasLiampotis commented 3 years ago

We need to be able to specify:

[ ] Certificate for signing/encrypting SAML messages on the SAML IdP interface of the Keycloak realm
[ ] Certificate for signing/encrypting SAML messages on the SAML SP side of the Keycloak realm
[x] Certificates for the OIDC OP interface of the Keycloak realm

It should be possible to store the certificates in keystore

NicolasLiampotis commented 3 years ago

Note: SAML and OIDC certificates typically have different rotation requirements, i.e. in SAML every 10 years while in OIDC every 6 six months

asieraguado commented 3 years ago

We are also interested in this feature, would it make sense to request this to the Keycloak community?

hannahshort commented 3 years ago

To clarify, Asier took a look and it doesn't seem possible in Keycloak itself to configure different RSA keys for different protocols. I'm not sure you'll be able to solve this with ansible config

laskasn commented 3 years ago

We'll definitely need to alter the keycloak's core code to allow using different certificates. With ansible (actually using the keycloak admin client or the rest api), you can add into any realm as many as {certificate and private key} pairs you wish. The tricky part is to differentiate the way keycloak selects the ones it will use for each of the bullet list cases - and that's where we need to alter the keycloak code. I'll take a look into the code and let you know how this can be tackled.

I think it's faster to code it ourselves and just issue a pull request with a nice motivation/description for that.

laskasn commented 3 years ago

It seems that we just need to add another -new- attribute in the (buggy) filter-by list they have for the certificates/keys, which will be about the protocol type {oidc,saml}. Currently, they distinguish the keys by

key use {encryption, signing}.
algorith {RSAxxx, AES, etc}

They do have a priority index, and they also publish the certificates anywhere needed with the configured priotity 0-Integer.MAX().

However, digging into the code i stumbled upon a major bug, which we need to first resolve before proceeding with our extra fine tuning. I already opened a bug ticket in keycloak issue management system. See here I paste the ticket text below, in case you don't have access there

We stumbled upon an as it seems major bug, which i'll try to explain below.

By default (upon creation), the realm comes with 2 RSA keys generated for you. One of the keys is for Encryption, while the other one is for Signing. If you add an IdP in the current realm (let's call it "realm-sp") and configure it to need Want AuthnRequests Signed -> ON Want Assertions Signed -> ON Want Assertions Encrypted -> ON save it, and then click the SAML 2.0 Service Provider Metadata to get the XML to be used on the client side of the other IdP, you get an XML with 2 key descriptors, one for signing and one for encryption. However, the key is the same, and it is the one configured in the realm as the signing key!!! The realm's encryption key is just ignored!!!

Digging inside the code, i found the buggy code and it's because you have hardcoded the fitering of any other KeyUse apart from the KeyUse.SIG .

The "offending" code lies within the

SAMLIdentityProvider.java public Response export(UriInfo uriInfo, RealmModel realm, String format) { ...

session.keys().getKeysStream(realm, KeyUse.SIG, Algorithm.RS256)

... }

This means that it just picks all the signature keys of the realm, and use them also for the encryption. Moreover, you just throw away any other algorithms than the RSA256 !!!! But let's say that this is a minor one, since you rely on the RSA256 which is almost always existent in the JVM.

I'll try to fix that and submit a pull request, because we need this fix!

laskasn commented 3 years ago

Ok, that bug seems to be quite simple. I have issued a pull request. I marked it as of "high" priority and also as "security relevant".

hannahshort commented 3 years ago

That’s great news!

Sent from my mobile

On 29 Jul 2021, at 11:48, Nikolas Laskaris @.***> wrote:

Ok, that bug seems to be quite simple. I have issued a pull request. I marked it as of "high" priority and also as "security relevant".

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

laskasn commented 3 years ago

Now that we fixed the "bug" (let's cross our fingers they accept it soon), we can try to also add support for using different keys per SAML/OIDC protocol and per IdP/SP side ;) But, before that, i noticed that they are missing another thing: when an admin adds a custom key (through the Realm Settings -> Keys -> Providers -> Add keystore -> rsa), they "forgot" to add on the UI the tickbox of specifying the use type of the key {enc/sig}, and adds it by default always as "sig". There's no such option {sig,enc} for java keystores as well. Awww that's going to be painful... It seems that all this chaos was introduced a couple of weeks before, when they decided to split the generated RSA keys by "use type" into "signature" and "encryption" :(

laskasn commented 3 years ago

I have found another bug and i issued another pull request. Hopefully they resolve them quickly.

The ticket can be found here, but i also copy my description below in case you don't have access.

Since a few weeks before, the RSA generated keys have also the distinct use type "enc" or "sig", with "sig" being the default. However, it seems that there is no such option to define a key.use {sig|enc} for the

JavaKeystoreKeyProvider ImportedRsaKeyProvider as it happens with the GeneratedRsaKeyProvider (which you can select from the UI).

Any keys added in keycloak with those 2 providers always get the type of "sig".

So, if for instance you want to import an RSA key into keycloak from a java keystore, it will always be for "sig" usage. You will never be able to use it for encrypting (for example in SAML assertion encryption).

I submit a pull request where i have modified the ImportedRsaKeyProviderFactory and the JavaKeystoreKeyProviderFactory to fix this.

laskasn commented 3 years ago

So far, we have made the appropriate changes to keycloak to be able to specify apart from the signature|encryption usages for each key, but also the OIDC|SAML|other usage for each key. So, now, for instance, if a key is marked as "saml" and "sig", it will appear (and used) only in tasks which are about oidc protocol and require signing of -let's say- something. Similarly, if a key is marked as "other" and "enc", it will be used for example to encrypt keycloak kerberos-related tasks. A drawback by the keycloak design is that adding more dimensions to the (type, protocol) vector, (i.e adding a "side" dimension with values "op" and "sp") the complexity escalates a lot, making it very difficult to handle and configure. So, if we really need to add also a distinguishing parameter for OP/SP side keys, i think it will become very complicated (even for an admin to understand the uses and configure all required keys).

laskasn commented 3 years ago

A proposal can be found here https://groups.google.com/g/keycloak-dev/c/L_Wt3lRwdoM

hannahshort commented 2 years ago

Hi @NicolasLiampotis, do you think it will be possible to make some progress here? Hoping that we might even be able to contribute after Christmas

laskasn commented 2 years ago

Hello @hannahshort We have already submitted 2 pull requests

(1st PR) for the uses {enc/sig} . To allow using a key not exclusively for one of {sig, enc}, but also for both.
(2nd PR) to enable specifying a protocol use for a key. For instance, exclusively for OIDC protocol (or SAML, etc)

You can find the ongoing discussion here. It seems that the first PR is accepted, but as you see, it takes some time to get it upstream. Probably they're having some hard time with the transition to the Keycloak.X, so all their responses to PRs are delayed ;)

laskasn commented 2 years ago

FUI @hannahshort @NicolasLiampotis @asieraguado

Unfortunately, they have delayed the reviewing of the submitted pull request(s), at such an extent, that a member of the keycloak developers team has made a change on the keys structure/definition. It's this commit on their master branch, described here In that commit, the developer among other changes, defined the following allowed algorithms for each of the enc and sig uses:

enc: {RSA1_5, RSA_OAEP, RSA_OAEP_256}
sig: {RS256, PS256, RS384, PS384, RS512, PS512}

Previously, the algorithms of sig (RSxxx and PSxxx) were used for encryption too. See files GeneratedRsaKeyProviderFactory.java and GeneratedRsaEncKeyProviderFactory.java of the above commit.

As mentioned in the description of the ticket, the changes regarding the algorithms, are done in order to comply to this spec, which -if i am not wrong- is OIDC specific.

A sample jwks_uri response now becomes like the following one (note the 'alg' fields)

{
    "keys": [
        {
            "kid": "j_8ma-cyBc-t2IPBMwldv_82Iz1lmTikWTTBsCozYT8",
            "kty": "RSA",
            "alg": "RS256",
            "use": "sig",
            "n": "pHFq..........",
            "e": "AQAB",
            "x5c": [
                "MIICmzCCAYMCBgF+bGLrZTANBgkqhkiG9w0BAQ.........."
            ],
            "x5t": "4WGvfgcUMZLYBRWdnhm92Eix004",
            "x5t#S256": "yd2Zt6GoyBOPOUMqunDx_cVOa35be5zB-_jW_HrhWes"
        },
        {
            "kid": "uJf7ntTUajg8IJJZ0uPIsPcjMaafrujQExgzlTpTfX8",
            "kty": "RSA",
            "alg": "RSA-OAEP",
            "use": "enc",
            "n": "sAHl..........",
            "e": "AQAB",
            "x5c": [ "MIICmzCCAYMCBgF.........."
            ],
            "x5t": "rYIwqMLFXOcfBN3j1LE4fUflwtI",
            "x5t#S256": "EtzIoqG51S1Y18dVWai7mfFcYJD95wFU8imIUGuVXZU"
        }
    ]
}

I wonder if it is desirable to use the same algorithms (RSA_OAEP etc) for the encryption in SAML (i.e. the assertion encryption).

Well, i need to check if they have already fixed that, because last time i checked, they always used "sig" key in SAML, even for encryption, which is definitely wrong...

They just ignored the second pull request which was about defining different keys for different protocols {oidc, saml, other} , which in fact becomes quite topical now, after their changes.

What's your take on that?

NicolasLiampotis commented 2 years ago

For our SAML federation uses cases we need to support the algorithms defined in the SAML V2.0 Deployment Profile for Federation Interoperability. If the required algorithms in SAML from that profile are incompatible with the algorithms from the JSON Web Algorithms (JWA) spec than that might be another good reason to support the per-protocol use of sig/enc keys (besides the different rotation policy requirements).

dpulrichth commented 2 years ago

I just spent a day and a half figuring out that Keycloak ignores encryption certificates when decrypting a SAML Assertion from another Identity Provider. While I guess we can sort of work around this by using the same certificate for signing and encrypting, this is not always possible since some Identity Providers insist on separate certificates, citing potential security problems when using the same certificate for both signature and encryption.

If nothing else, please fix this ASAP. If possible it would be great if we could specify the signature / encryption certificate to use per Identity Provider, as they have different requirements regarding rotation periods.

laskasn commented 2 years ago

I just spent a day and a half figuring out that Keycloak ignores encryption certificates when decrypting a SAML Assertion from another Identity Provider. While I guess we can sort of work around this by using the same certificate for signing and encrypting, this is not always possible since some Identity Providers insist on separate certificates, citing potential security problems when using the same certificate for both signature and encryption.

If nothing else, please fix this ASAP. If possible it would be great if we could specify the signature / encryption certificate to use per Identity Provider, as they have different requirements regarding rotation periods.

I have updated the pull request which fixes this. Hope this time they won't postpone the review. The keycloak dev team normally responds with a large delay to any enhancement/fix we propose...

eosc-kc / keycloak

Investigate how to configure specific certificates for SAML and OIDC #110