Add Flag to override TLS certificate location

[ChaosInTheCRD]

Summary

A command flag in server.go that would allow the user to set a custom location in the pod filesystem (e.g. /tls/tls.crt / /tls/tls.key) that the server looks to for a TLS certificate .

Use Cases

This would allow users to mount their own certificates (e.g. from vault / k8s secret), and also allow for external controllers (like cert-manager) to manage the certificates rotation.

Restricted environments/organisations also may require Fully validated TLS Encryption from a trusted Root CA.

Notes

Another thing to note is that it seems as though the makefile generates a single set of key/cert each time the image is built.

My concern is that this means (especially if TLS is now default) the certificate will become invalid 365 days after the image was built, and the image itself will no longer function correctly, with no way of easily mounting your own certificate to replace it.

I would like to make my first contribution!

As I have a good idea about how this enhancement can be delivered, I would like the opportunity to create the PR myself and complete my first contribution. Therefore if I could have this issue assigned to me I would very much appreciate it 😃

---

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

[alexec]

I think the 365 day issue is a bug, and should be fixed.

[alexec]

We can address the 365 day issue by generating self-signed certs whenever the server starts. This is more secure, but it does not allow you to use your own certificates. We could have a conventional path for them, and only create them if they are not found.

[davidcollom]

I agree on the generation on startup - this would without doubt increase the security posture (I.E: not the same TLS Key/Cert Across all argo instances using that specific release/version).

Allowing the end user to be flexible and provide the location of their certificates is a relatively common practise across a large portion of the ecosystem.

I would discourage/conventional hard coded paths, as the end user may not have much/any power over the naming or location convention and therefore cause more confusion and inability to provide a secure installation.

If said certificate flags are passed and the files are not present, I would expect argo server to crash/exit so that you cannot start up in secure mode without said certificates (without explicitly saying insecure mode on).

[alexec]

@davidcollom I'll roll the changes out to Argo Dataflow, would you be willing to look at porting them to workflows?

[davidcollom]

@alexec - There is already a PR open @ https://github.com/argoproj/argo-workflows/pull/6460 which would be good to get some feedback on this specific area.

[alexec]

So this is what I'm thinking:

https://github.com/argoproj-labs/argo-dataflow/pull/196

The runner generates a self-signed certificate when it starts. This same code would work for argo-server, but with one tweak, if /tmp/argo/argo-server.crt exists, then we use that instead. That would mean that this is configuration-by-convention.

[davidcollom]

Yea, but this issue still stands:

I would discourage/conventional hard coded paths, as the end user may not have much/any power over the naming or location convention and therefore cause more confusion and inability to provide a secure installation.

If I'm a user of a cluster and don't have the ability to override where my certificates are presented to argo, therefore this doesn't solve the issue.

Being able to provide/override these values for those environments would a great step forward, something in which @ChaosInTheCRD began implementing earlier.

Something that this also doesn't cover is if you need to inject or validate all certificates/communications and require the loading of a Root/Intermediate CA Certificate.

Bear in mind that if you're storing said certificates as a kubernetes secret they will follow the convention of tls.crt and key.crt along with ca.crt, these can be overridden via subpath. HOWEVER has a drawback that the secret is NOT updated to the container and therefore a restart of any pod would be required to pick up any renewed certificates.

[alexec]

So you're suggesting put the certificates into a secret and have argo-server load them from there?

[ChaosInTheCRD]

If this style was followed, it would work really well. You get kubernetes to rely on creating the certificate for you, but leave the opportunity for the user to completely decide on which certificates they want to mount (certificates.k8s.io, vault, cert-manager etc.).

It also works well with the PR that I raised 😊.

Regardless, I'm really happy to work on this. Myself and @davidcollom could also pair on it if there is more to be completed from the current PR

[alexec]

If Kubernetes can create the secrets automatically (and is supported in all recent versions and by all providers) then I think that is the correct solution.

Can I just create a certficates.k8s.io resource and Kubenetes fills out the details for me?

[egeland]

Would be great if we just specify the TLS location, like

spec:
  tls:
    secretName: my-awesome-cert

Then you can have cert-manager create and maintain with config like:

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: my-awesome-cert
spec:
  dnsNames:
  - argo.example.com
  issuerRef:
    kind: ClusterIssuer
    name: letsencrypt
  secretName: my-awesome-cert

[agilgur5]

Another thing to note is that it seems as though the makefile generates a single set of key/cert each time the image is built.

My concern is that this means (especially if TLS is now default) the certificate will become invalid 365 days after the image was built, and the image itself will no longer function correctly, with no way of easily mounting your own certificate to replace it.

We can address the 365 day issue by generating self-signed certs whenever the server starts.

I agree on the generation on startup - this would without doubt increase the security posture (I.E: not the same TLS Key/Cert Across all argo instances using that specific release/version).

For reference, this specific portion of the issue was resolved in #6540

Would be great if we just specify the TLS location, like

Something similar to this was added in #5582 / #7621 / #9789 via the --tls-certificate-secret-name flag.

That being said, I think using file paths is a better option IMO, as it does not require Argo to have access to the Secret.

Instead, a user can mount the Secret as a volume, and that way Argo does not need to have RBAC to get that Secret.