Repository navigation
Initial draft to check CA subject DN in X.509 client authenticator - #48550
Conversation
| @@ -129,7 +129,7 @@ public void init(Config.Scope config) { | |||
|
|
|||
| TruststoreCertificatesLoader certsLoader = new TruststoreCertificatesLoader(truststore); | |||
| provider = new FileTruststoreProvider(truststore, verificationPolicy, Collections.unmodifiableMap(certsLoader.trustedRootCerts) | |||
| , Collections.unmodifiableMap(certsLoader.intermediateCerts)); | |||
| , Collections.unmodifiableMap(certsLoader.intermediateCerts), TruststoreBuilder.getHttpsTruststore()); | |||
There was a problem hiding this comment.
If there's really no build time work that needs captured, instead of the static / system properties on the TruststoreBuilder, you can directly use calls like config.root().get(HttpOptions.HTTPS_TRUST_STORE_FILE.getKey()) here.
IMHO this is not how it should work to validate certificates. The standard way is:
Yes, the server could "help" by providing those intermediate certificates, but that's not their job, but maybe that's ok. If you would allow an intermediate by just looking at its common name, then any other CA could present such an intermediate CA common name, and then you would be IMHO vulnerable. So I'd vote for not configuring any intermediate CAs. |
|
@ahus1 Argh! Yes, I defaulted again to normal certificate thinking. And yes, that is possible, using a CertPathBuilderResult. I just need to separate trust and intermediate certs. Thanks for spotting this! OK, I'm going to change that and set just one CA Subject DN for the authenticator. I'm going to try what @shawkins comments. I also decided what to do with the subject DN check. I think that I'm going to deprecate the regex switch option and the CA Subject DN is only checked with full DN. The regex makes no sense. |
Please stop me if you think I'm too strict. +1 for not having a regex for the CA Subject DN. About multiple CAs: Thinking about this more, a setup might have different CAs that it trusts, with a different set of user CNs/rules what to accept. For such a setup, would I then have multiple authenticators that are available as alternatives? When for such a setup multiple authenticators are used, only a single DN for the CA would be necessary. |
|
For the moment this is just for the X.509 client authenticator. It's big enough to not add more things. For the client I think that we don't need multiple CAs, just one cert for the client, so one CA. For the users I would say that maybe we need more than one, maybe they have different CAs valid for users, so users can present different valid certificates but the data to match (for example email) is always in the certs. But this is more for the next PR. |
|
@rmartinc - Hm, might still be the same for the client CAs. Can I combine multiple client authenticators in that flow? |
|
@tsaarni - I got a question for you as you implemented "Authorizing clients that are allowed to send XFCC headers" for the envoy proxy setup: https://github.com/Nordix/keycloak-client-cert-lookup-for-envoy I see that you added an option With this issue at hand, I wonder if you would consider specifying the lead subject CN plus the root CA CN of similar ease of use and security. Or in other words, when authenticating clients, is Subject CN + Root CN the same as Subject CN + Intermediate CN + Root CN? Would you be missing a feature if it would not allow Subject CN + Intermediate CN? Thanks! |
|
@ahus1 In theory the certificate chain sent to the server is all the cert chain minus one (the CA is not present). But sometimes only the initial cert is sent by the client, I have always understood that this was a mistake, but it happens. In order to be able to verify the cert in this situation (missing intermediate), keycloak should also have the intermediate in the truststore to complete the validation (it's more or less the same for the TLS part). There is a test that is doing exactly this, so we are already testing that a single cert (missing intermediate) is working. I'm finishing the change to have a second round of comments. |
|
While root CAs are more commonly used as trust anchors, intermediate CAs can also be configured for partial chain validation. I think this is generally a convenience feature rather than a necessity. In my client certificate lookup provider project, I allowed the combinations (leaf alone, leaf plus intermediates, or the full chain) just for flexibility. I guess one could have used partial chain validation if wanted (we don't). If the check supported only the leaf and root, then edge cases like below could not be expressed unambiguously. |
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#testPostBadAssertionSignatureKeycloak CI - Adapter IT Strict Cookies |
|
Okis, tests passed OK. Now the changes are:
There are missing things like upgrading notes, better labels and things like that. But I think the functionality is complete. Probably I will add another change to not use the https truststore if the |
|
I think that now it's complete. I have added the notes for 26.7. Don't know if we want this to be backported but probably we better maintain it for 26.7. Please take a look when you have time. |
|
@rmartinc - this looks good to me, thank you for the change. I reviewed the docs, and it makes sense to include this only in 26.7 as it is a breaking change (though for good reasons). I also see that it also logs warnings. The only real breaking change is IMHO that the import would fail if the information is missing. Could we be more gratuitous here? Maybe make this field only mandatory in the UI, and add the breaking changes only in 27? Some nitpicks:
|
| /** | ||
| * Creates a SSL context configured with passed client keystore and trustore files | ||
| * obtained from the classpath. | ||
| * | ||
| * @param keystore Keystore file in classpath | ||
| * @param truststore Truststore file in classpath | ||
| * @param mTlsEnabled If mutual TLS is enabled | ||
| * @return The SSL context | ||
| */ | ||
| public SSLContext createClientSSLContext(String keystore, String truststore, boolean mTlsEnabled) { | ||
| Path truststorePath = checkPath(truststore); | ||
| KeyStore truststoreStore = load(truststorePath); | ||
| if (mTlsEnabled) { | ||
| Path keystorePath = mTlsEnabled ? checkPath(keystore) : null; | ||
| KeyStore keystoreStore = load(keystorePath); | ||
| return createClientSSLContext(keystoreStore, truststoreStore, mTlsEnabled); | ||
| } else { | ||
| return createClientSSLContext(null, truststoreStore, false); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This class should only return an SSLContext created from the KeyStore and TrustStore loaded at server startup. If a test needs a different SSL context for testing purposes, I think the responsibility should be up to them to create it.
There was a problem hiding this comment.
I have not done this point for the moment. Maybe we can create a KeystoreUtils or similar and this class and the test class use the utility methods. But IMO the test that needs to use different client certificates is going to always inject the certificates, so here is enough. If you really prefer to use a utility class just let me know.
| this.serverKeystore = ManagedCertificates.checkPath(serverKeystore); | ||
| this.serverTruststore = ManagedCertificates.checkPath(serverTruststore); | ||
| this.clientKeystore = ManagedCertificates.checkPath(clientKeystore); | ||
| this.clientTruststore = ManagedCertificates.checkPath(clientTruststore); |
There was a problem hiding this comment.
The conversion from String to Path and any checks shouldn't be in a configuration class. Putting these four lines in the ManagedCertificates class is a better option here.
| * @param clientTruststore | ||
| * @return this | ||
| */ | ||
| public CertificatesConfigBuilder stores(String serverKeystore, String serverTruststore, String clientKeystore, String clientTruststore) { |
There was a problem hiding this comment.
Since this is a builder, it begs the question of whether this method should be split into 4 methods for each store. But all four arguments need to be set anyway, so it wouldn't change much.
There was a problem hiding this comment.
Yep, my idea was showing all the stores are needed. If you want to separate them in different methods is OK to me, but I preferred this way to show all of them are necessary.
It makes sense, but then the kc.sh or direct endpoints can be used to set a configuration without the CA subject DN. For me it's OK and just doing the check in the console is enough (but I wanted to be extra secure). I have added the change. Thinking now maybe we can also add the warning in the validation but let it goes through. For the moment I have just commented the previous validation. I wanted to see the tests. I tested locally and it seems to work OK but I want to be sure. I'll add the warning tomorrow if I see the CI passes OK.
Done.
Done in both, clientId and realm name.
Done. |
|
rebasing |
There was a problem hiding this comment.
Approving the test-related changes
|
Rebased because of conflicts. |
There was a problem hiding this comment.
@rmartinc The changes makes sense to me, Thanks for the PR!
Approving, but did not review deeply whole implementation as I rely on other reviews of this PR.
Just some points:
-
This PR will possibly cause some issues with dynamic client registration as the metadata mentioned in the OAuth 2 MTLS client authentication RFC does not have anything like
Certificate authority subject DNor similar. So the dynamically registered clients, which rely on the MTLS likely will not work OOTB. Maybe next step is to make client-policy executor, which will configure theCertificate authority subject DNfor the dynamically registered clients. But that is for a follow-up though? -
Another point (slightly related to the above): When the option
Certificate authority subject DNis configured at the client level, it means that client administrator has permission to configure it. So the client administrator would have permission to configure it to the possibly untrusted CA. I can think of some example scenario like:
- Server has 2 certificate authorities in truststore
ca=myroot1andca=myroot2. But realm admin of realmfoowants to trust only certificates issued byca=myroot1 - Client administrator of client
myclientin realmfoohas certificate with subjectca=mysubjectsigned byca=myroot2(which is unexpected CA as realm admin wants justca=myroot1) . But client administrator will be able to configureCertificate authority subject DNtoca=myroot2and hence bypass the requirements by realm admin.
Maybe this is a corner-case though? At the same time, the assumption is that client administrators are not so trusted guys and their permission should be limited... However maybe the client policies are the solution to this? The client-policy executor I've mentioned in point 1 can be possibly used to also enforce that Certificate authority subject DN is as expected.
I am approving as these are likely corner-cases and this PR improves the situation. But just something to consider :-)
|
Thanks @mposolda! I think that we need to add something for point 1, at least the new attribute should be added to the representation and be passed to the endpoint. I'll update the upgrading notes with this point saying that now two attributes should be passed to configure X509 authentication vie registration endpoint. For point 2 I think that we are OK. If the client admin change the jwks URL is exactly the same for JWT authetnication, they can obviously change how the client authenticates. I see the X509 CA the same. If the client admin wants to do bad things to authenticate client, he can do it (note he can even change the client to use a secret and use it directly). The problem here is someone else different to the admin uses another CA for another realm for example. So wait for the moment, I will add changes for point 1. |
|
@rmartinc The problem is that OIDC client representation does not have anything like If you want to address this, I see the possibility is either have the |
|
Yep, that's the problem of doing weird things with certificates. 😄 For the moment we are not enforcing the attribute, just in the admin console. So it will work. So we have to think about that problem in the future (if we enforce it). |
|
I think we can add a client registration policy to add a default CA subject DN. I will try that too. |
|
Yes, client registration policy is also an option. I was rather thinking the "client policy" (as client registration policies are maybe going to be deprecated/removed at some point in favour of client policies, but not sure when it happens...). But client registration policy is also OK |
Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
|
@mposolda I have added a client policy to add a default CA subject DN for the client registration. A custom medatadata attribute is created too. I added a extra boolean property to reject any other CA (this limits the client admins too). Changes in the last commit. WDYT? |
| @@ -103,6 +103,9 @@ public class OIDCClientRepresentation { | |||
|
|
|||
| private String tls_client_auth_subject_dn; | |||
|
|
|||
| // custom attribute to set the CA subject DN in X509 authentication | |||
| private String tls_client_auth_ca_subject_dn; | |||
There was a problem hiding this comment.
My vote is to go simple in this PR and not introduce the custom attribute and just always make executor to assign configured value.
The parties, which use OIDC dynamic client registration usually need to be specs compliant and using custom (KC specific) attributes does not work for those. We do not use the custom attribute for anything else in the OIDCClientRepresentation. Moreover, it is not documented (besides changes-26_7_0.adoc, but those are just upgrading instruction, but not a proper docs).
…tion Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com>
* Initial draft to check CA subject DN in X.509 client authenticator Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> * Add the test to the FIPS suites and doc changes Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> * Delete AbstractMutualTLSClientTest to move the old test Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> * Move the old test to the new test suite Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> * final test changes Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> * First round of review changes. Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> * Add a client executor to add default CA subject DN to client registration Closes keycloak#48412 Signed-off-by: rmartinc <rmartinc@redhat.com> --------- Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes #48412
Initial draft to also check the CA Subject DN in the client X.509 authenticator. The main ideas are the following:
TruststoreBuilder. This part is nasty and I don't like it (I usesSystem.setPropertyfollowing what it's done today for the common TLS properties). We can improve it, I accept ideas (maybe creating a truststore directly in the builder? But for the moment I want to receive some feedback for the whole PR).X509ClientAuthenticatorclass. Currently the class is usingcertificate.getSubjectDN().getName()to match the regex (when using regex), which is deprecated and in the javadoc mentions that is internal implementation that should not be used (I have even checked that the result is different for Sun or BCFIPS implementation for example). I don't know what to do here, it only affects when the regular expression is used. I would say that we need to use RFC2253 (getX500Principal().getName(X500Principal.RFC2253, CUSTOM_OIDS)). But this is totally a breaking change. And we cannot adapt the regexp at upgrade or similar. For the moment I'm maintaining the samegetSubjectDN, but it's incorrect for sure. Maybe we can just change and add a switch in the factory to revert to previous (incorrect) behavior.ManagedCertificateshelper now allows you to configure stores directly (instead of creating them). This way we can add complex configurations with CA, intermediate,... I'm re-using the certificates from the old TS. I have not removed the old test in the draft.@stianst @mposolda @ahus1 @vmuzikar @shawkins WDYT?