Repository navigation
Update HaProxySslClientCertificateLookup to load Base64 encoded DER Chain - #49181
Conversation
Depends on the changes in keycloak/keycloak#49181
|
@ahus1 @pruivo as agreed on the call earlier, these are the changes to make I have pushed a commit to keycloak/keycloak-quickstarts#762 which has the required changes to easily test this. |
Depends on the changes in keycloak/keycloak#49181 Signed-off-by: Ryan Emerson <remerson@ibm.com>
Depends on the changes in keycloak/keycloak#49181 Signed-off-by: Ryan Emerson <remerson@ibm.com>
Depends on the changes in keycloak/keycloak#49181 Signed-off-by: Ryan Emerson <remerson@ibm.com>
|
Upon further investigation we can update the provider in a backwards compatible way. The provider makes use of HAProxy: Keycloak: We can keep above handling when HAProxy: Keycloak: |
There was a problem hiding this comment.
Thanks @ryanemerson for the PR! But if I understanding this correctly the chain in HA proxy contains the leaf certificate too (it's the full chain presented by the client in the mTLS negitation), so the older header with just the leaf is not needed. In this regard is different to rfc 9440. Check if I'm not mistaken here.
I would try to get real headers from HA proxy for the tests and create new files with them instead of reusing the RFC 9440 ones. Just to be sure we are testing what HA proxy is really sending.
I'm also adding @shawkins for the review as he is more experienced with the certificate lookup implementations than me. 😄
|
Thanks for the review @rmartinc
That's not the case, it does not contain the leaf certificate, only the intermediaries. HAProxy relies upon the OpenSSL SSL_get_peer_cert_chain method:
https://docs.openssl.org/master/man3/SSL_get_peer_cert_chain/
Yes that's a good point, updated to use a Base64 cert and cert chain consisting of two intermediary certs generated by HAProxy. I have also updated the test to assert the expected cert Subject DN for each index in the chain. |
There was a problem hiding this comment.
Hello @ryanemerson - looks good overall. Some nitpicks below.
What I now wonder: Is haproxy rfc9440 compliant or not? I remember you referenced and issue in one of our meetings. If it is not, and if we're waiting for an issue to be implemented, it would be good to reference it somewhere in the docs or the code.
(Reading your comment to Ricardo, this is partly answered there, which is "no, it is not compliant?")
| * provide a built-in mechanism to define a header per intermediate cert in the chain . Example HAProxy config: | ||
| * <pre> | ||
| * http-request set-header Client-Cert %[ssl_c_der,base64] | ||
| * http-request set-header Client-Cert-Chain_0 %[ssl_c_chain_der,base64] |
There was a problem hiding this comment.
I'm confused: In this line, %[ssl_c_chain_der,base64] produces a single certificate, while below it produces the whole chain?
There was a problem hiding this comment.
It only works if a single intermediate cert exists in the chain. I can make that limitation clearer in the JavaDoc, as well as stating that this is now deprecated.
There was a problem hiding this comment.
JavaDocs updated, hopefully this makes the limitation of using the old prefix approach clearer.
| Example HAProxy configuration: | ||
|
|
||
| [source] | ||
| ---- | ||
| http-request set-header Client-Cert %[ssl_c_der,base64] if { ssl_c_used } { ssl_c_verify 0 } | ||
| http-request set-header Client-Cert-Chain %[ssl_c_chain_der,base64] if { ssl_c_used } { ssl_c_verify 0 } | ||
| ---- | ||
|
|
||
| Corresponding {project_name} configuration: | ||
|
|
||
| <@kc.start parameters="--spi-x509cert-lookup--provider=haproxy --spi-x509cert-lookup--haproxy--ssl-client-cert=Client-Cert --spi-x509cert-lookup--haproxy--ssl-cert-chain=Client-Cert-Chain --spi-x509cert-lookup--haproxy--certificate-chain-length=2"/> |
There was a problem hiding this comment.
I'd say this should better be described in the haproxy-reencrypt.adoc file. At the same time, having it here aigns it with the other proxies.
There was a problem hiding this comment.
I can add it to haproxy-reencrypt.adoc as an optional configuration as well. I think it makes sense to keep the changes in reverseproxy to be consistent with the other providers docs, as we don't plan to add a dedicated blueprint for each proxy.
To simplify the documentation and user steps, I could make it so that the haproxy provider has default header values for both the cert and cert-chain e.g. "Client-Cert" and "Client-Cert-Chain", that way there's two less KC CLI options that the user needs to specify. If a user configures ssl-cert-chain-prefix then we use that for backwards compatibility reasons and log a warning. WDYT @ahus1 @pruivo ?
There was a problem hiding this comment.
Some second thoughts: We must not change the default behavior, as people might not filter those headers in their proxy. With that, they would be vulnerable (or at least someone can pass in a header). They might today not have the parameter specified as they don't care about the intermediate certificates.
So we would need to keep that change until KC 27 (as it is a breaking change).
There was a problem hiding this comment.
Good catch. I'll leave the defaults as is then 👍
headers Closes keycloak#49180 Signed-off-by: Ryan Emerson <remerson@ibm.com>
Signed-off-by: Ryan Emerson <remerson@ibm.com>
…rt for ssl-cert-chain Signed-off-by: Ryan Emerson <remerson@ibm.com>
…d cert files and assert Subject DNs Signed-off-by: Ryan Emerson <remerson@ibm.com>
- Deprecate use of sslCertChainPrefix - Add note in upgrading guide - Clarify javadoc - Update HAProxy blueprint to include ssl-cert-chain usage - Add test cases for invalid HTTP header values Signed-off-by: Ryan Emerson <remerson@ibm.com>
f6df9c1 to
dc6b218
Compare
Not. I have updated the JavaDocs of Thanks for the reviews @ahus1 @pruivo. I believe I have addressed all comments, but I would like your input on #49181 (comment) |
There was a problem hiding this comment.
A couple of code suggestions :)
Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
Signed-off-by: Ryan Emerson <remerson@ibm.com>
Depends on the changes in keycloak/keycloak#49181 Signed-off-by: Ryan Emerson <remerson@ibm.com>
headers
Closes #49180