Skip to content

Conversation

@albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Jul 24, 2019

The X509AuthenticationToken contains the certificate chain that the PKI realm validates and parses out the principal from. This validation is guarded by a cache for which the key computation, given an X509AuthenticationToken, does a sha256 of the encoding of the first certificate in the chain. Given that the same end entity certificate cannot be validated by different CAs with different certification paths (at least in practice) the cache key computation saves cycles by not evaluating the complete chain. This assumption does not hold anymore, in the case that the certificate chain is passed by the delegate-pki-authn API, because in that case the certificate chain given must be a complete chain, it must not be a prefix of another chain. The current implementation exposes inconsistencies in cases where only the client certificate (not the full path) are passed in the API request after (and not before) the complete path for that same certificate has been cached.

Relates #34396

@albertzaharovits albertzaharovits added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jul 24, 2019
@albertzaharovits albertzaharovits self-assigned this Jul 24, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

Copy link
Contributor

@bizybot bizybot left a comment

Choose a reason for hiding this comment

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

Good catch! LGTM, Thank you.

@tvernum
Copy link
Contributor

tvernum commented Jul 26, 2019

Can you explain the source of these inconsistencies that you mention?
I don't follow why delegated PKI changes the behaviour here. The client cert is still the same.

@bizybot
Copy link
Contributor

bizybot commented Jul 26, 2019

Can you explain the source of these inconsistencies that you mention?
I don't follow why delegated PKI changes the behavior here. The client cert is still the same.

When we authenticate against cache we do not do cert chain validation, in case the incoming request has the same end-entity certificate but just different CA certs (ordered but not the CA certs that issued certificate) then we just authenticate using the certificate thumbprint and allow authentication, that would be wrong.
Now in case of PKI realm authentication (without delegation) the cert chain is trusted when TLS connection is established but not true in case of delegated PKI.
This may not be a major thing but with this change, I think we are safe as we allow authentication to proceed only if the cert chain matches.

@albertzaharovits
Copy link
Contributor Author

@tvernum Sorry I've been too abstract in the description.

A simple example is when a client calls the API with the full chain, and another client calls the API only with the end entity cert (the same end entity cert from the other client's chain). The first one is correct, according to our specs, and the second one is wrong. When you factor in the current cache implementation, the call from the second client might be successful if the call from the first client came in first and populated the cache.

@tvernum
Copy link
Contributor

tvernum commented Jul 29, 2019

I don't have a problem with this change, but I still don't understand why we need it.

You say

in that case the certificate chain given must be a complete chain

But where do we enforce that? I can't see it in PkiRealm, TransportDelegatePkiAuthenticationAction or X509AuthenticationToken

So if we don't enforce it, why does the caching behaviour make any difference?

@albertzaharovits
Copy link
Contributor Author

But we do enforce a complete chain.

PkiRealm#isCertificateChainTrusted calls javax.net.ssl.X509TrustManager.checkClientTrusted . This JRE method does the validation of the chain according to RFC5280. Given a trust anchor and an end entity certificate there must be a certification path for the end entity certificate to be considered valid. All the certificates in between the end entity and the trust anchor must be passed in the delegate-pki method call.

For example, given this chain:
client certificate -> intermediate CA -> root CA , and configuring root the root CA in the truststore of the PKIRealm, the client must pass the two (client certificate and intermediate CA) certificates in the delegate_pki API call. Only then the client certificate is valid and we can proceed extracting the principal from its Subject DN. So, if the client only passes the client certificate we reject the call, since there is no way to find the in-between certificates that complete the path. However, given the present cache implementation, the former behavior is not guaranteed. Say that there are two clients calling the delegate-pki API using the same end-entity certificate. One is passing the complete chain (both client certificate and intermediate CA) and the other is passing only the client certificate. The correct behavior would be to always reject the calls of the client passing only the client certificate. But when at some point in time the client passing the two certificate chain calls the API, that call would be successful and would populate the cache. Given that the current cache implementation only inspects the end entity to compute the cache key, the next time the client passing only the end entity certificate calls the API it will be validated because the cache says so.

@tvernum
Copy link
Contributor

tvernum commented Jul 30, 2019

Let's discuss offline.

@albertzaharovits
Copy link
Contributor Author

@tvernum
In the usual, non-delegated, scenario where we extract the client's certificate from the TLS session we use

sslEngine.getSession().getPeerCertificates();

where the javadoc for getPeerCertificates reads:

Returns the identity of the peer which was established as part of defining the session.

Note: This method can be used only when using certificate-based cipher suites; using it with non-certificate-based cipher suites, such as Kerberos, will throw an SSLPeerUnverifiedException.

Note: The returned value may not be a valid certificate chain and should not be relied on for trust decisions.

Returns:
--->> an ordered array of peer certificates, with the peer's own certificate first followed by any certificate authorities.
Throws:
SSLPeerUnverifiedException - if the peer's identity has not been verified
See Also:
getPeerPrincipal()

with the relevant part mentioning that this is an an ordered array of peer certificates.

Given what we've discussed, does this PR look OK for merging? (I think it does)

@albertzaharovits albertzaharovits requested a review from tvernum July 31, 2019 16:38
@tvernum
Copy link
Contributor

tvernum commented Aug 1, 2019

@albertzaharovits In that case (as a separate piece of work) should we require that the chain passed in to the delegated_pki API must also be a full chain from client to root CA?
I think that could be as simple as checking that the last element is self-signed.

I think it would be more consistent for the realm to know that chains are always complete, than have delegated-pki be a bit different.

From a Kibana perspective, the Node.js APIs are similar to the Java ones (at least according to the docs, I haven't done thorough testing) you can get the client certificate itself, or the end-to-end chain but I don't believe that Kibana will ever have, or pass to us, a partial chain.

@albertzaharovits albertzaharovits merged commit 6cf863f into elastic:proxied-pki Aug 1, 2019
@albertzaharovits albertzaharovits deleted the security-pki-delegation-realm-cache-chains branch August 1, 2019 08:46
@albertzaharovits
Copy link
Contributor Author

@tvernum

should we require that the chain passed in to the delegated_pki API must also be a full chain from client to root CA?
I think that could be as simple as checking that the last element is self-signed.

No this is not necessary. From RFC 5280:

When the trust anchor is provided in the form of a self-signed
certificate, this self-signed certificate is not included as part of
the prospective certification path. Information about trust anchors
is provided as inputs to the certification path validation algorithm

However, the JRE doesn't mind if it is included or not (this is covered in tests as well).

I think it would be more consistent for the realm to know that chains are always complete, than have delegated-pki be a bit different.

Just to be clear, it does not matter if we're talking about the delegation scenario or the usual one, the certificates in the X509AuthenticationToken are always ordered, and the last certificate might or might not be a self-signed certificate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants