fix: Generate TLS Certificates on startup and only keep in memory

[davidcollom]

This is a first pass resolution to issues raised within #6355

the tls util was taken from argo-cd's utilities and an additional tlsConfig helper added to simplify the initilisation process of the server.

[alexec]

I have one comment on this. tls.go is very large file. I wonder if it is better just to choose a single good set-up, e.g. TLS v1.3 RSA 4096 so that this code does not sit un-used and un-maintained.

[davidcollom]

That's a fair point, we did consider this but thought first pass to take a copy from argoCD as is.

[alexec]

Now taken a deeper-dive, I'd like to ask for a more frugal solution. This is 400 new lines of complex code with the attendant risks or many lines of code.

• Do we want 4096 on 2048 RSA?
• I don't think we should keep any code for algos we do not use. Everything ECC can be removed. It will lead to the assumption that
• We don't need any support for CA.
• Creation of tls.Config should be in a different file.

If it is acceptable to have files on disk, e.g. because argo-server.[crt|key] has been mounted, then presumably it is acceptable to write the certificates to a temporary directory? But am I wrong about this?

This was my take on this:

https://github.com/argoproj-labs/argo-dataflow/blob/main/runner/sidecar/generate_cert.go

Please scrutinise it. It doesn't have the tls.Config code.

[alexec]

What ECSDA rather that RSA?

[davidcollom]

ECSDA is faster, and uses smaller keys to maintain the same/better level of security than that of RSA. Which would require a much larger key and comes with a performance hit.

[yaronf]

I totally agree with @davidcollom. But please make the particular algorithm configurable and not hard-coded. To test your stuff, I suggest you support both P256 and P521. P256 is OK as the default.

[yaronf]

General question (I know my way around TLS but not Argo): now that the cert is no longer a static file and is stored in memory, should there be an API or command-line option to pull it out of the container?

[davidcollom]

General question (I know my way around TLS but not Argo): now that the cert is no longer a static file and is stored in memory, should there be an API or command-line option to pull it out of the container?

The certificate is not designed to be taken out of the container in any manor, this is the intention of this change, there are further PR's to address the bring your own certificates.

The intention of the Self Signed Certificate is to ensure encryption from an ingress/loadbalancer to the argo-server, and nothing more.

[yaronf]

I totally agree with @davidcollom. But please make the particular algorithm configurable and not hard-coded. To test your stuff, I suggest you support both P256 and P521. P256 is OK as the default.

[yaronf]

The cert duration should be configurable, too.

[davidcollom]

This is no different to the current master/main duration, of which is NOT configurable.

if you wish to bring your own certificate, then please see other open PR's which look to use kubernetes TLS certificates.

[yaronf]

The intention of the Self Signed Certificate is to ensure encryption from an ingress/loadbalancer to the argo-server, and nothing more.

But (as you stated in the mail thread) if the user can fetch the cert, they can put it in their trust store, which is sometimes a good thing - instead of using an unauthenticated connection.

Also I am still hoping AWS will see the light and will eventually start verifying TLS connections from the LB.

[davidcollom]

if the user can fetch the cert, they can put it in their trust store, which is sometimes a good thing - instead of using an unauthenticated connection.

Sorry, I don't understand you here? Downloading a key from a public docker image of which EVERYONE on that release is using is by no means secure and as you would also find mentioned is a large security issue.

By NOT allowing users to override this, prevents the user from inadvertently and unknowingly open them up to this issue.

Also I am still hoping AWS will see the light and will eventually start verifying TLS connections from the LB.

Likewise, but this wouldn't be resolved here, I would argue, that this should not be self signed certificates from Argo and should be a bring your own certificate situation from a Corporate or Trusted CA.

[yaronf]

if the user can fetch the cert, they can put it in their trust store, which is sometimes a good thing - instead of using an unauthenticated connection.

Sorry, I don't understand you here? Downloading a key from a public docker image of which EVERYONE on that release is using is by no means secure and as you would also find mentioned is a large security issue.

By NOT allowing users to override this, prevents the user from inadvertently and unknowingly open them up to this issue.

I assume you mean "downloading the cert", not "the key".

And I understand your point, but the flip side is that you're educating the user to work with a broken TLS connection ("broken lock"). I don't understand the use case well enough to make a call here.

[yaronf]

Sorry if I'm repeating myself, I think GH lost one of my comments.

I'm not sure where in the cert these identifiers (IP address, DNS name) are going. Where they should be going is the SubjectAltName extension, see https://datatracker.ietf.org/doc/html/draft-rsalz-use-san. (The draft is expired but what it says is still relevant).

[SgtCoDFish]

IPAddresses and DNSNames is how one tells Go to fill in the SAN extension, so this bit looks correct as far as filling in the SAN extension

[alexec]

I’d actually want to avoid configurability. It’s extra settings for users to get wrong, and is to have to support. If over time ECC is wrong, then we can release a new version to address that.

Users who want to accept certs can use Cert Manager.

[SgtCoDFish]

@ChaosInTheCRD pointed me at this; I took a look and it looks pretty good to me.

Without the ability to export the cert and establish trust out-of-band (i.e. before the first connection is made), it's not possible to use the generated certs safely and it'll always be vulnerable to a man-in-the-middle attack on at least the first connection. I think that's what @yaronf is getting at.

Without the ability to export the cert, there's always that risk. That said, with the cert being regenerated on startup, and a different one being generated for each pod that could be running in a deployment (as far as I can tell?), trying to establish trust ahead-of-time isn't going to work in practice.

Honestly, if I were implementing something like this from scratch, I wouldn't touch this TLS generation stuff and would only do bring-your-own-cert, like in the other PR. If my app was running in k8s, I'd probably have it generate a cert-manger CertificateRequest and wait for cert-manager to do its thing (but then, as a cert-manager maintainer, of course I would say that 😁)

Obviously this isn't starting from scratch though, and this PR is definitely a big improvement over the current situation - the cert generated by this PR isn't a CA, and crucially isn't the same for every argo-workflows deployment.