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

Proposal for KeyStore SPI #345

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

tsaarni
Copy link

@tsaarni tsaarni commented Oct 18, 2022

This PR proposes new SPI called "KeyStore". The purpose of the SPI is to allow an arbitrary number of key stores to be provisioned.

This work is split from old PR keycloak/keycloak#7365. I'm hoping that by creating a separate design document, and splitting the work into smaller chunks it would be possible to get reviews and feedback.

@pedroigor
Copy link
Contributor

@tsaarni Nice write-up.

If I got it correctly, we are going to have multiple identifiers so that we can use key|trust stores for different purposes: LDAP client, SMTP, HTTP Client, HTTP Server, etc.

These identifiers are going to potentially map to configuration options that we should make available from the distribution.

Does it make sense?

Also, should the proposed SPI be used too to configure realm keys? The hot-reload capabilities being proposed might be useful there too.

@tsaarni
Copy link
Author

tsaarni commented Oct 19, 2022

Thanks @pedroigor!

If I got it correctly, we are going to have multiple identifiers so that we can use key|trust stores for different purposes: LDAP client, SMTP, HTTP Client, HTTP Server, etc.

These identifiers are going to potentially map to configuration options that we should make available from the distribution.

Yes, exactly.

Also, should the proposed SPI be used too to configure realm keys? The hot-reload capabilities being proposed might be useful there too.

Oh, that sounds interesting! I assume that possibility must apply only those cases when realm keys are loaded from keystore, right?

So would there need to be a "realm dimension" when loading keystores? I guess same problem applies to some other cases too. For example, LDAP federation is configured per realm, so in theory administrator might need multiple LDAP keystores (one per realm at max)? Realm specific config seems difficult to "model" with config properties.

@pedroigor
Copy link
Contributor

pedroigor commented Oct 19, 2022

So would there need to be a "realm dimension" when loading keystores? I guess the same problem applies to some other cases too. For example, LDAP federation is configured per realm, so in theory administrator might need multiple LDAP keystores (one per realm at max)? Realm specific config seems difficult to "model" with config properties.

I'm not pushing for including realm keys at this point but just something we might consider in the future given that we do have realm keys based on keystores. Although key rotation is already possible with realm keys, doing it automatically as herein proposed, would be an improvement.

So, back to the scope of the proposed design, using an identifier to map keystores should work for "server-level" key stores such as the one used to enable HTTPS. However, I'm unsure if the same is true for components (such as user federation providers like LDAP) as they are bound to a realm.

We could potentially append to the identifier the name of the realm to map keystores to a realm but realms are dynamic and you would need to lazily load keystores from the filesystem.

If you keep using the same keystore for all realms you are kinda in the same situation where the same keys and trust anchors are shared across distinct TLS domains.

@tsaarni
Copy link
Author

tsaarni commented Oct 20, 2022

@pedroigor I was also thinking a bit about realm specific trust|key store problem when responding to keycloak/keycloak#13300 (comment).

In a way there is discrepancy in component configuration when it comes to trust anchors and credentials. For example, today, in component configuration for LDAP user federation we set everything else needed to connect to the server except the path to trust store. Admin had to anticipate this and set trust store as config properties before starting Keycloak. The same would be true even if we would add realm part into the identifier in the proposed KeyStore SPI - administrator would still need to set the configuration in advance. This discrepancy has been OK for me so far, but I can understand why this can be a problem for others.

If going one step further, I could imagine that the administrator provisions relevant trust|key store as part of component config. In simplest form, it could be just paths pointing to local files on the server - what was previously given as startup config. It is doable: split the KeyStore SPI between server-level vs realm-specific domains. Server-level would be using the identifiers mapped from parameters given at startup. In realm-specific case, the consumer of the SPI would need to pass the config directly. It would add new impact to SPI consumers. For example, LDAP user federation implementation needs to store and pass the config for KeyStore SPI.

There is more complicated scenario as well: imagine the realm admin is not able or allowed to put the files on the server(s) and therefore trust|key stores should be part of component config somehow: they could be uploaded / downloaded, stored to / fetched from database etc. In that case the identifier passed by consumer is not path but maybe e.g. some other reference that can be resolved by the KeyStore SPI implementation. There might be some resemblance here to Vault SPI.

One shortcut that crossed my mind previously, was that component config could refer to a key alias in a store that was pre-configured at startup. After hot-reload is supported, it would be possible to add new entries on demand e.g., lazily at the time when LDAP user federation is configured. But I do not think this is the optimal user experience.

@pedroigor
Copy link
Contributor

Thanks.

As I mentioned before, it should be fine for now to support server-level keystores. If we agree that the current proposal is OK even though realms would still be sharing the same keystores, I agree to move it forward.

I was thinking about how certs/keys are going to be reloaded in kubernetes. Looks like we would need to restart the server pods anyways, right?

@tsaarni
Copy link
Author

tsaarni commented Oct 25, 2022

Thank you @pedroigor.

As I mentioned before, it should be fine for now to support server-level keystores. If we agree that the current proposal is OK even though realms would still be sharing the same keystores, I agree to move it forward.

An alternative approach could be to have KeyStore SPI load key stores and PEM files only by file path (assuming credentials are always files and not e.g. in SQL database). The caller could be then responsible for resolving the config into paths, instead of the KeyStore SPI implementation doing that. That could avoid namespacing server-level vs realm-level and move the problem towards the caller.

I was thinking about how certs/keys are going to be reloaded in kubernetes. Looks like we would need to restart the server pods anyways, right?

I would like to avoid requiring restart for server pods. I'm also running Keycloak on Kubernetes so this use case is close to me. Assuming the certs/keys are loaded from secret mount, the complication is due to the way how Kubernetes updates the files. I've previously recorded details about the update process and inotify events emitted by it (link). It is bit challenging to use inotify with this update scheme, and most importantly, the same implementation would not work for every other file update schemes. For example, Envoy ended up later adding more configurability for administrator to set the watched directory via config. It seems bit complicated to me, and I wish administrator would not need to care.

So far, I did not include default implementation details in the KeyStore SPI proposal document, but my personal preference for watching cert/key file updates would be to avoid inotify (with some additional reasons I collected here in this paragraph) and I would prefer just "clever" polling instead: when Entries are fetched from KeyStore (i.e. during TLS handshake), lazily check if the files on disk have been modified since they were last loaded. Define a "cache TTL" period that caps how frequently this check is executed at most. The benefit in this approach is that it works regardless of how the files are updated, including the scheme that Kubernetes uses for secret mount updates.

@tsaarni
Copy link
Author

tsaarni commented Nov 14, 2022

@pedroigor and @mposolda May I ask for your help, it would be great if we could proceed with this feature!

@mposolda from your comment keycloak/keycloak#7365 (comment) I understand that you prefer that KeyStore SPI would support single keystore, similarly as TrustStore SPI does today. I can modify the proposal accordingly. But I wonder what is your opinion on my follow up question in keycloak/keycloak#7365 (comment)? As far as I can see, user would need to configure multiple keystores if targeting both LDAP SASL EXTERNAL and HTTPS server certificate hot-reload use case as well.

@tsaarni
Copy link
Author

tsaarni commented Feb 22, 2023

I wanted to ask again if it would be possible to push keystore SPI / certificate hot-reload forward?

Keycloak lost hot-reload support when moving from Wildfly to Quarkus (Elytron default SSL context could be hot-reloaded via Wildfly CLI). In my use case we cannot live without hot-reload so I've needed to fork Keycloak for now, and it is getting painful to us.

@pedroigor
Copy link
Contributor

An alternative approach could be to have KeyStore SPI load key stores and PEM files only by file path (assuming credentials are always files and not e.g. in SQL database). The caller could be then responsible for resolving the config into paths, instead of the KeyStore SPI implementation doing that. That could avoid namespacing server-level vs realm-level and move the problem toward the caller.

The SPI should naturally support both levels because it is going to have access to the realm. For now, it should be fine to move forward with the server-level key material.

I was thinking about how certs/keys are going to be reloaded in kubernetes. Looks like we would need to restart the server pods anyways, right?

I would like to avoid requiring restart for server pods. I'm also running Keycloak on Kubernetes so this use case is close to me. Assuming the certs/keys are loaded from secret mount, the complication is due to the way how Kubernetes updates the files. I've previously recorded details about the update process and inotify events emitted by it (link). It is bit challenging to use inotify with this update scheme, and most importantly, the same implementation would not work for every other file update schemes. For example, Envoy ended up later adding more configurability for administrator to set the watched directory via config. It seems bit complicated to me, and I wish administrator would not need to care.

So far, I did not include default implementation details in the KeyStore SPI proposal document, but my personal preference for watching cert/key file updates would be to avoid inotify (with some additional reasons I collected here in this paragraph) and I would prefer just "clever" polling instead: when Entries are fetched from KeyStore (i.e. during TLS handshake), lazily check if the files on disk have been modified since they were last loaded. Define a "cache TTL" period that caps how frequently this check is executed at most. The benefit in this approach is that it works regardless of how the files are updated, including the scheme that Kubernetes uses for secret mount updates.

+1

@tsaarni
Copy link
Author

tsaarni commented Feb 28, 2023

I've updated the proposal: Instead of using "keystore identifiers" to request a keystore for a specific use case, I simply added just file paths. I'm hoping this could avoid the "namespace" discussion (e.g. here and here) and move the "addressing" problem to SPI consumers instead.

As a result, for example LDAP provider would need to become aware of the files. I'm thinking: if we assume that certs/key + CA cert becomes part of LDAP config in realm, isn't it then natural consequence that LDAP provider needs to become aware of the files?

This raises new questions to me:

  1. Should the realm administrator be able to specify the full path for realm-specific credentials paths, or should there be limitations for security reasons? E.g. a base path for realm-specific files. In other words: can the realm administrator be trusted to load arbitrary files from the filesystem?
  2. How to handle credentials when they are not files? For example, the user might want to use PKCS#11 to offload credentials to a hardware token.

Copy link
Contributor

@stianst stianst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite following this proposal. Is this basically about supporting multiple different truststores for different contexts? Or, is it about providing the keystore part for Keycloak's own private certs when connecting to external services? Or, both? How does this overlap with vault for example?

Can you explain how an end-user would use what is proposed here? For example to "configure LDAP or SMTP" using mTLS authentication?

@tsaarni
Copy link
Author

tsaarni commented Mar 2, 2023

@stianst Thank you for your comment!

Originally, this proposal included only the ability to configure multiple keystores globally. The intention is to allow end-user to set specific client certificate (+ chain certificates) when communicating towards LDAP servers using mutually authenticated LDAPS or LDAP + StartTLS connections.

I received comments (above) where it was highlighted that the configuration is in fact realm specific.

Therefore, I'm proposing the ability to configure multiple keystores and truststores (both are technically keystores). The intention is to allow end-user to set specific set of client/server certs and CA trust anchors for different contexts, both in global and in realm specific context.

Another use case that I'd like to address is hot-reload of client and server certificates, without requiring restart of Keycloak. Since it is very strongly related to the keystore implementation, it is included in this proposal. Example use case here is rotation and hot-reload of HTTPS server certificate. It is also an example of a globally configured certificate (in contrast to realm context).

The situation today:

  • It is only possible to use Java default keystore for LDAP connections.
  • It is possible to set up a single truststore (via truststore SPI or Java default truststore), which is shared between different use cases and realms i.e. trust "leaks" between use cases, and realms, regardless of end-user wanting that or not.
  • Configuration of LDAP provider is part of realm, but configuring certificates for the LDAP connection is not.
  • Certificate rotation requires restart of Keycloak.

@tsaarni
Copy link
Author

tsaarni commented Mar 2, 2023

I do not see overlap with Vault SPI, since it is addressing opaque data blobs and it does not support KeyStore.

There is overlap with TrustStore SPI. My thinking here is that KeyStore SPI could allow unifying the provisioning of both client/server certificates and CA certificates. Technically truststores are KeyStores with only trust anchors loaded in it.

@vmuzikar
Copy link
Contributor

vmuzikar commented Oct 6, 2023

CC @shawkins
Related to our effort for improving Truststore config UX.

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

Successfully merging this pull request may close these issues.

4 participants