Kong Client side TLS Vulnerability/Optimization problems

[jeremyjpj0916]

Kong has some support for Client side TLS for upstream API security, see more here from NGINX on it:
https://www.nginx.com/resources/admin-guide/nginx-tcp-ssl-upstreams/

Kong enables the settings of this feature via the kong.conf or the KONG_ environment variable override associations:
client_ssl = on
client_ssl_cert = /usr/local/kong/ssl/testCert.crt -> In Nginx: proxy_ssl_certificate
client_ssl_cert_key = /usr/local/kong/ssl/testKey.key -> In Nginx: proxy_ssl_certificate_key

The above is a good start, but is vulnerable to MITM attacks because Kong has not given users a way to specify the CA signer crt value proxy_ssl_trusted_certificate NGINX param via the kong.conf or environment variables, see here in the nginx_kong.lua file:

> if client_ssl then
    proxy_ssl_certificate ${{CLIENT_SSL_CERT}};
    proxy_ssl_certificate_key ${{CLIENT_SSL_CERT_KEY}};
> end

I propose one extra optional field in kong.conf
client_ssl_trusted_cert = /usr/local/kong/ssl/CaCert.crt

I also recommend using proxy_ssl_session_reuse on; to have NGINX proxy previously negotiated connection parameters and use a so-called abbreviated handshake, otherwise CPU is taking a unnecessary hit:
https://blogs.msdn.microsoft.com/huizhu/2009/12/17/ssl-tls-full-handshake-vs-abbreviated-handshake/

Which can all be addressed in the lua code above with:

> if client_ssl then
    proxy_ssl_certificate ${{CLIENT_SSL_CERT}};
    proxy_ssl_certificate_key ${{CLIENT_SSL_CERT_KEY}};
    proxy_ssl_session_reuse on;
    > if client_ssl_trusted_cert then
         proxy_ssl_verify on;
         proxy_ssl_verify_depth ${{CLIENT_SSL_VERIFY_DEPTH}};
         proxy_ssl_trusted_certificate ${{CLIENT_SSL_TRUSTED_CERT}};
    > end
> end

Not sure where you do the whole KONG_{config_value} switcharoo for the conf file with the env variables but that would need to be in place as well for the CLIENT_SSL_TRUSTED_CERT field.

Additional Details & Logs

• Kong version 11.1

Concerns/Thoughts?

[bungle]

@jeremyjpj0916, what you propose sounds like a great addition. Do you want to propose a PR for that. I can possibly take a look at it as well. We also then need to document such a feature, so we need additional PR for documentation around here: https://github.com/Kong/getkong.org/blob/master/app/docs/0.11.x/configuration.md#client_ssl_cert_key

Ultimately at some point this can be fixed with (otherwise we are replicating one-by-one all the Nginx directives):
#2355

BTW, I'm sure you were already aware of writing a custom Nginx configuration that you can possibly use until we have added this: https://github.com/Kong/getkong.org/blob/master/app/docs/0.11.x/configuration.md#custom-nginx-configuration

[jeremyjpj0916]

@bungle Glad you are in agreement, will look into pushing a PR request shortly 👍 . And yes I know of writing a custom Nginx config, I just get anxious deviating from the "baseline" because future changes could cause problems with custom configs potentially. The more I can point out to Kong that we agree upon could use a fix up the better, plus everyone wins rather than just myself :) .

[jeremyjpj0916]

@bungle @thibaultcha Better late than never eh :) I had some free time this weekend for an hour or two, I have made a fork and began committing to the next branch. A few points to bring up here before I PR this:

Kong working with self signed certs, would yah like to just add one that stems from the kong's self signed certs at the https://github.com/jeremyjpj0916/kong/tree/next/spec/fixtures/ location a kong_spec.pem file? Or if you want me to add it then got any specific CA's in mind?

Same thing here:
https://github.com/jeremyjpj0916/kong/blob/next/kong/conf_loader.lua#L54
I imagine you want me to just leave that to yall when you build out correct?

Other changes I was not sure on(unit testing):
https://github.com/jeremyjpj0916/kong/blob/next/spec/01-unit/003-prefix_handler_spec.lua#L128

How to handle the on case for an optional parameter like client_ssl_trusted_cert (proxy_ssl_trusted_certificate directive). Got the snippet for what I need to additionally add or do you want me to PR and let you clean up the ON case? OFF case was pretty simple even with my lack of knowledge on how to do these.

https://github.com/jeremyjpj0916/kong/blob/next/spec/01-unit/002-conf_loader_spec.lua#L552
^ This guy as well I am not sure how to add the new param to this case either.

And lastly the documentation too:
https://github.com/Kong/getkong.org/blob/master/app/docs/0.13.x/configuration.md#client_ssl_cert_key

Since we are supposed to push code to "next" branches to avoid new features in existing versions I was expecting to see a WIP 0.14.x branch of getkong.org prior to release but it seems you keep that branch private or something until release? Otherwise I would have added something like this to the configuration.md

client_ssl_trusted_cert
If `client_ssl` is enabled, specifies the absolute path to a file with trusted CA certificates in the PEM format used to verify the certificate of the proxied HTTPS server for the `proxy_ssl_trusted_certificate` directive. Note this value is statically defined on the node, and currently cannot be configured on a per-API basis.

Default: none

client_ssl_verify_depth
If `client_ssl_trusted_cert` is configured, sets the verification depth in the proxied HTTPS server certificates chain enabled by the `proxy_ssl_verify_depth` directive.

Default: 1

Other than that I think I have modified every place in the actual code that would need this to work barring the unit test changes I am sort of confused on, they feel so much different than the junit tests I am used to doing in Java micro-service development hah.

We won't be using this pattern internally where I work for Kong<->API Provider Security but I hope it helps others! Also is Kong swagg still available for accepted PR's 😆 ? I will rock that t-shirt yo.

Comparison view: next...jeremyjpj0916:next

[jeremyjpj0916]

Update too on my findings, so

proxy_ssl_session_reuse on;

Is the default for nginx, so that means kong ships it on by default right now too, so for now i removed it:
http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_ssl_session_reuse

But the comments below also make me skeptical too:
"
Determines whether SSL sessions can be reused when working with the proxied server. If the errors “SSL3_GET_FINISHED:digest check failed” appear in the logs, try disabling session reuse.
"
Do we want a way in the kong conf to explicitly be able to enable/disable it, or just let it default on like it always has with kong(my guess is the above error means SSLv3 has a problem with the abbreviated handshake approach anyways so thats why there must be the potential for problems)? I will update my code accordingly. I personally feel SSLv3 is old and probably should not make a whole config value dedicated to helping them out(as kong doesn't do that now anyways, either knowingly or unknowingly 😋).

Another thing to consider would be this directive:
http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_ssl_protocols

Am I getting too picky here by thinking to include this as a new argument? Would we be overwhelming kong users? It does default to the most common types(TLSv1 TLSv1.1 TLSv1.2;) so maybe Kong continues to leave it off? Hopefully Nginx adds TLSv1.3 in the default list in near future as it is fully released now.

I also am not seeing the value in why kong decided to rename these from proxy_* nginx named directives to something like client_* (which makes me think of consumer-> kong calls as opposed to the action of proxying to the backend).

[sukrit007]

We have a use case of just doing end-to-end TLS without client cert and having some way to add trusted CA cert. Wondering, if we want to add this feature, irrespective of the fact that we have client certificate or not:

if proxy_ssl_trusted_cert then
         proxy_ssl_verify on;
         proxy_ssl_verify_depth ${{PROXY_SSL_VERIFY_DEPTH}};
         proxy_ssl_trusted_certificate ${{PROXY_SSL_TRUSTED_CERT}};
end

[jeremyjpj0916]

Closing this out of age. Can be revisited later if anyone wants mutual TLS support. .

[thibaultcha]

@jeremyjpj0916 Mutual TLS support is coming to Kong, more on this in the next few weeks as @james-callahan is finishing up his work on it. @james-callahan you may care to add something to this issue

[jeremyjpj0916]

Oh neat, I do not need it, but glad to see Kong stepping up on that functionality as I know others have mentioned it a few times I have seen. None of my links out to code in my fork work anymore in this issue as I deleted that fork at this point to clean it off for a different simpler PR I made on ACL, but essentially my changes mostly revolved around the nginx conf values needed to achieve mutual TLS as well as the kong.conf values. And some test cases and such as well. Will look at your code @james-callahan and see if it ended up being the same stuff I had put in a fork awhile back.

[james-callahan]

Will look at your code @james-callahan and see if it ended up being the same stuff I had put in a fork awhile back.

The code is quite different: it's being driven from the Lua side of things (rather than nginx configuration). We're still discussing which parts become an EE feature vs a CE feature (and hence the bulk of the code is being held in private).

[jeremyjpj0916]

@james-callahan Ahh that make a whole lot of sense as to do it with lua otherwise the capability would be fairly restricted and require lots of cert updates heh. Puts my older deleted patch to shame(but in my defense mine would have solved a simple small scale solution need :p ). Best of luck on the new EE feature!

[rafaelmagu]

@james-callahan does Kong now support the scenario described by @sukrit007 where end-to-end TLS is necessary but client certificates are not being used?

(edited because I got the wrong mentions 🤦‍♂️ )