Supporting ECC Certificates #190
Conversation
There was a problem hiding this comment.
This looks great - thanks very much!
A couple of separate concerns I have with this:
-
What happens if a user wants some certificates to be ECC, and some RSA (for browser compatibility etc.). Right now we don't have the concept of a 'kube-lego class' - the closest we have is limiting kube-lego to only watch a single namespace
-
What happens if a user switches the key type after already having requested some certificates?
Can come up with an answer for what should happen in these cases? It'd also be great to have some test coverage for the second case.
pkg/kubelego/kubelego.go
Outdated
| default: | ||
| kl.legoKeySize = kubelego.EccKeySize | ||
| break | ||
| } |
There was a problem hiding this comment.
Can we do something here to detect invalid key sizes? Silently assuming defaults despite a user specifying otherwise could be dangerous.
pkg/kubelego/kubelego.go
Outdated
| case "521": | ||
| ks, err := strconv.ParseInt(keySize, 10, 0) | ||
| if err != nil { | ||
| kl.legoKeySize = kubelego.EccKeySize |
There was a problem hiding this comment.
Can we return an error here instead of assuming defaults? Silently ignoring options can lead to confusion when debugging.
pkg/kubelego/kubelego.go
Outdated
| } else if keyType == kubelego.KeyTypeRsa { | ||
| ks, err := strconv.ParseInt(keySize, 10, 0) | ||
| if err != nil { | ||
| kl.legoKeySize = kubelego.RsaKeySize |
There was a problem hiding this comment.
Again, can we return an error here instead of assuming defaults silently?
pkg/kubelego/kubelego.go
Outdated
| kl.legoKeyType = kubelego.KeyTypeEcc | ||
| break | ||
| default: | ||
| kl.legoKeyType = kubelego.KeyTypeEcc |
There was a problem hiding this comment.
I'd prefer not to change the default here, as I'm unsure how this would play with existing certificates.
README.md
Outdated
| @@ -115,6 +115,8 @@ Please note: | |||
| | `LEGO_LOG_LEVEL` | n | `info` | Set log level (`debug`, `info`, `warn` or `error`) | | |||
| | `LEGO_KUBE_ANNOTATION` | n | `kubernetes.io/tls-acme` | Set the ingress annotation used by this instance of kube-lego to get certificate for from Let's Encrypt. Allows you to run kube-lego against staging and production LE | | |||
| | `LEGO_WATCH_NAMESPACE` | n | `` | Namespace that kube-lego should watch for ingresses and services | | |||
| | `LEGO_KEY_TYPE` | n | `ecc` | Set your preferred private key type (`ecc`, `rsa`) to generate certificates with. | | |||
There was a problem hiding this comment.
Can we keep rsa as the default here please
|
To answer the second case, the certificate will only be renewed when (1) the cert does not cover all domains, (2) it is expired or is going to expire sooner than the minimum validity setting. Certificates provisioned before changing the key types will not trigger a certificate request. See https://github.com/harborfront/kube-lego/blob/master/pkg/ingress/tls.go |
|
Anything going on with this? The functionality is quite useful. |
|
@frusdelion still have interest in this? |
This pull request allows for the generation of both RSA and ECC CSRs, ECC being the new default.
This resolves #144