Changing minimum certificate rotation period and backdating to 1h #1243
Conversation
Current coverage is 54.85% (diff: 100%)@@ master #1243 diff @@
==========================================
Files 78 77 -1
Lines 12402 12373 -29
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 6803 6787 -16
+ Misses 4665 4652 -13
Partials 934 934
|
| Usage: []string{"signing", "key encipherment", "server auth", "client auth"}, | ||
| Expiry: certExpiry, | ||
| NotBefore: notBefore, | ||
| NotAfter: notAfter, |
There was a problem hiding this comment.
Probably a dumb TLS question, but is it correct to specify both NotBefore / NotAfter and also Expiry? Does cfssl use both of them?
There was a problem hiding this comment.
CFSSL is silly and doesn't seem to allow me to not specify Expiry. Tells me this is an invalid profile. Didn't dig super far into it though, I might be missing something obvious.
In TLS there is not concept of Expiry, the only two things that matter are notBefore and notAfter.
This is the code that fills in the notBefore and notAfter: https://github.com/cloudflare/cfssl/blob/f8a8adaa9d48141125b78c23553a174845661522/signer/signer.go#L280
diogomonica
force-pushed
the
change-backdate
branch
from
46b9078
to
4fbf169
Compare
| @@ -124,10 +124,15 @@ func SigningPolicy(certExpiry time.Duration) *cfconfig.Signing { | |||
| certExpiry = DefaultNodeCertExpiration | |||
| } | |||
|
|
|||
| notBefore := time.Now().Round(time.Minute).Add(CertDefaultBackdate).UTC() | |||
| notAfter := time.Now().Round(time.Minute).Add(certExpiry).UTC() | |||
There was a problem hiding this comment.
Probably a good idea to only call time.Now() once to ensure consistency between these timestamps, in case something crazy happens to the clock between the two calls.
It will probably never issue in practice, but I'd rather have one less corner case to think about.
Signed-off-by: Diogo Monica <[email protected]>
diogomonica
force-pushed
the
change-backdate
branch
from
4fbf169
to
6486e7f
Compare
|
LGTM But I'm not sure we can merge this in between the last -rc and the 1.12 GA :( ping @aluzzardi @tiborvass for opinions |
|
Tagged as 1.12.1. We'll push it at the next release |
@aaronlehmann
Fixes #1175
Signed-off-by: Diogo Monica [email protected]