rqlite / helm-charts

Helm charts for rqlite
MIT License
9 stars 0 forks source link

extraArgs is ignored #7

Closed roeldev closed 6 months ago

roeldev commented 6 months ago

I have certificates with my own root ca that I would like to use. I tried using extraArgs. The secret wich is referenced by config.tls.node.secretName is generted from cert-manager and contains a ca.crt value.

extraArgs:
  - "-fk=true"
  - "-http-ca-cert=/config/client-tls/ca.crt"
  - "-node-ca-cert=/config/node-tls/ca.crt"

Unfortunately it seems this is currently not possible as the extraArgs helm value is ignored. This results in an error: failed to verify certificate: x509: certificate signed by unknown authority

Before I used this helm chart I'd add the certs via args, using the same secret. This worked.

...
          args:
            # https://rqlite.io/docs/guides/security/
            - "-http-ca-cert=/certs/ca.crt"
            - "-http-cert=/certs/tls.crt"
            - "-http-key=/certs/tls.key"
#            - "-http-verify-client"
            - "-node-ca-cert=/certs/ca.crt"
            - "-node-cert=/certs/tls.crt"
            - "-node-key=/certs/tls.key"

Am I missing something or is this currently not supported via the helm charts?

jtackaberry commented 6 months ago

So first of all, yeah, extraArgs is ignored and that's definitely bug. No idea how I missed that one. (It got inadvertently dropped after some refactoring needed to support readonly nodes.) Will fix it later today.

That said ...

I had expected custom root CAs to be used by setting config.tls.node.ca and/or config.tls.client.ca to the PEM contents of the root certificate(s), because ca.crt isn't officially part of a kubernetes.io/tls Secret. That seems to be a fairly idiomatic approach, at least with the charts I've used.

I suppose you must be using cert-manager's in-tree CA issuer?

I think it's a bit of a hack to use the CA that issued the server cert as the same one to validate client certs. Of course it makes perfect sense in the context of inter-node communication where rqlite doesn't support using a different client cert for mTLS, but it seems less correct for client (application) connections over HTTPS.

CAs are pretty long-lived -- is it a problem to put the CA cert in the config.tls.node.ca and/or config.tls.client.ca chart values?

Otherwise, your extraArgs approach will work (once I fix the bug) but I can't claim that's an official/supported method. That is to say, I don't consider the mount point of the secret to be "public API", so to speak.

Not that I have any plans to change it, to be sure, but if for some reason I did, I wouldn't consider it a breaking change requiring a major version rev.

jtackaberry commented 6 months ago

Should be fixed in version 1.2.0 of the chart, just released. Thanks for the bug report!

roeldev commented 6 months ago

So first of all, yeah, extraArgs is ignored and that's definitely bug. No idea how I missed that one. (It got inadvertently dropped after some refactoring needed to support readonly nodes.) Will fix it later today.

Thanks for that!

That said ...

I had expected custom root CAs to be used by setting config.tls.node.ca and/or config.tls.client.ca to the PEM contents of the root certificate(s), because ca.crt isn't officially part of a kubernetes.io/tls Secret. That seems to be a fairly idiomatic approach, at least with the charts I've used.

I suppose you must be using cert-manager's in-tree CA issuer?

I'm using cert-manager with step-ca issuer.

I think it's a bit of a hack to use the CA that issued the server cert as the same one to validate client certs. Of course it makes perfect sense in the context of inter-node communication where rqlite doesn't support using a different client cert for mTLS, but it seems less correct for client (application) connections over HTTPS.

You're right that using the same cert for http and inter-node traffic feels a bit wrong. That's something I probably need the change, but does not seem like a problem (for now).

CAs are pretty long-lived -- is it a problem to put the CA cert in the config.tls.node.ca and/or config.tls.client.ca chart values?

Otherwise, your extraArgs approach will work (once I fix the bug) but I can't claim that's an official/supported method. That is to say, I don't consider the mount point of the secret to be "public API", so to speak.

Not that I have any plans to change it, to be sure, but if for some reason I did, I wouldn't consider it a breaking change requiring a major version rev.

The ca cert I'm using is automatically generated and stored in a sealed secret. I prefer to keep it there and not to copy it to helm values all over the place.

Would it be possible to introduce an extra helm value config.tls.node.caSecretName (and for client config.tls.client.caSecretName) which expects a ca.crt value, which then mounts the ca secret and sets the -node-ca-cert arg?

I'd be happy to make a pull request for it.

jtackaberry commented 6 months ago

Would it be possible to introduce an extra helm value config.tls.node.caSecretName (and for client config.tls.client.caSecretName) which expects a ca.crt value, which then mounts the ca secret and sets the -node-ca-cert arg?

I actually had the same thought when typing my last reply. I don't think I've come across that in my travels, but I understand the use case (and your reluctance to manually duplicate generated values). Considering that cert-manager explicitly does this, and cert-manager is the incumbent when it comes to cert generation, your idea sounds cromulent to me. I'd never noticed this before because I use ACME for client-facing certificates (so Secrets won't have ca.crt), and I hadn't gotten around to playing with cert-manager for a private CA.

No need for a PR, I've already started working something up.

But now that I think of it, with your previous deployment before this Helm chart, were you actually using mutual TLS for the HTTP port? If so, how were you dealing with readiness probes? Currently config.tls.client.mutual is ignored pending a solution to https://github.com/rqlite/rqlite/issues/1508 -- did you have workable solution to this one?

roeldev commented 6 months ago

Would it be possible to introduce an extra helm value config.tls.node.caSecretName (and for client config.tls.client.caSecretName) which expects a ca.crt value, which then mounts the ca secret and sets the -node-ca-cert arg?

I actually had the same thought when typing my last reply. I don't think I've come across that in my travels, but I understand the use case (and your reluctance to manually duplicate generated values). Considering that cert-manager explicitly does this, and cert-manager is the incumbent when it comes to cert generation, your idea sounds cromulent to me. I'd never noticed this before because I use ACME for client-facing certificates (so Secrets won't have ca.crt), and I hadn't gotten around to playing with cert-manager for a private CA.

I've used this when setting up a Postgres cluster using cloudnative-pg: https://cloudnative-pg.io/documentation/1.16/certificates/#user-provided-certificates-mode

spec:
  ...
  certificates:
    serverCASecret: postgres-tls
    serverTLSSecret: postgres-tls

But now that I think of it, with your previous deployment before this Helm chart, were you actually using mutual TLS for the HTTP port? If so, how were you dealing with readiness probes? Currently config.tls.client.mutual is ignored pending a solution to rqlite/rqlite#1508 -- did you have workable solution to this one?

Mutual TLS for client was indeed not working with the readiness prober. I also didn't look further into that as I'm still in a proof of concept stage of building my app and setting up the required Kubernetes services.