Added utility func to compute cert expiry. #339
Conversation
packager/util/util.go
Outdated
| // Returns -1 if cert is already expired. This will be used to periodically check if cert | ||
| // is still within validity range. | ||
| func GetDurationToExpiry(cert *x509.Certificate, currentTime time.Time) time.Duration { | ||
| if cert.NotBefore.After(currentTime) || cert.NotAfter.Before(currentTime) { |
There was a problem hiding this comment.
Depending on how this ends up being used, I'd suggest two changes:
- Drop the
NotAfter.Before(currentTime)bit; the return statement below will already return a negative value in that case. - Print a warning to the screen when
NotBefore.After(currentTime); this probably indicates some configuration error (either with the CA, or the local clock), and it would be good to have some knowledge if the server ends up spinning in a loop because of this.
If you choose to do #2 in the caller function instead of here (reasonable choice either way), then I'd prefer returning (time.Duration, error) instead of using a sentinel value.
There was a problem hiding this comment.
I decided to keep #1 because the logic seems cleaner if I can just return a valid duration plus nil (for the error).
I also decided to let the caller handle the error message.
There was a problem hiding this comment.
I think that it is no use to check if the cert is expired at the currentTime because the cert should be valid until the SXG signature is expired. It is too late to know its expiry at the currentTime.
The signature expiry time of Expires: now.Add(6 * 24 * time.Hour) needs to be considered and it is better to add it in this check.
There was a problem hiding this comment.
I agree, this utility is not meant to check it at current time (as in time.Now()), it was meant to check the cert against a specified deadline. Your point about the naming of the parameters is well taken, I will rename it to certExpiryDeadline.
There was a problem hiding this comment.
Maybe add a note, too, to the function doc that this should be the expected SXG expiration time.
@shigeki Also note that we discussed making a toml config parameter (something like renewalGracePeriod), with a default of say, 2 days, so that the process for requesting a new cert via ACME will begin 6+2=8 days before expiration, to allow for ACME server outages. Does this seem OK?
There was a problem hiding this comment.
c365be8 is fine for me. Thanks.
banaag
force-pushed
the
packager-cert-cache-mods
branch
from
8c5b955
to
c779aa8
Compare
packager/util/util.go
Outdated
| // is still within validity range. | ||
| func GetDurationToExpiry(cert *x509.Certificate, currentTime time.Time) (time.Duration, error) { | ||
| if cert.NotBefore.After(currentTime) || cert.NotAfter.Before(currentTime) { | ||
| return time.Duration(-1), errors.New("Certificate has invalid duration") |
There was a problem hiding this comment.
Go convention is to return the zero value on error. In this case, I think you can say return 0, errors...; the cast to Duration is implicit.
packager/util/util.go
Outdated
| return time.Duration(-1), errors.New("Certificate has invalid duration") | ||
| } | ||
|
|
||
| return cert.NotAfter.Sub(currentTime), nil; |
packager/util/util.go
Outdated
| // is still within validity range. | ||
| func GetDurationToExpiry(cert *x509.Certificate, currentTime time.Time) (time.Duration, error) { | ||
| if cert.NotBefore.After(currentTime) || cert.NotAfter.Before(currentTime) { | ||
| return time.Duration(-1), errors.New("Certificate has invalid duration") |
There was a problem hiding this comment.
"invalid duration" error message is wrong. Perhaps "Certificate is expired". (Maybe have a second error message for NotBefore like "Certificate is future-dated".)
packager/util/util_test.go
Outdated
| zeroDaysBeforeExpiry := time.Date(2019, time.August, 7, 5, 43, 32, 0, time.UTC) | ||
|
|
||
| d, err := util.GetDurationToExpiry(pkgt.B3Certs[0], beforeCert) | ||
| assert.Contains(t, errorFrom(err), "Certificate has invalid duration") |
There was a problem hiding this comment.
Maybe use https://godoc.org/github.com/stretchr/testify/assert#EqualError ? Unless an exact string match is prohibitive.
packager/util/util.go
Outdated
| // Returns -1 if cert is already expired. This will be used to periodically check if cert | ||
| // is still within validity range. | ||
| func GetDurationToExpiry(cert *x509.Certificate, currentTime time.Time) time.Duration { | ||
| if cert.NotBefore.After(currentTime) || cert.NotAfter.Before(currentTime) { |
There was a problem hiding this comment.
Maybe add a note, too, to the function doc that this should be the expected SXG expiration time.
@shigeki Also note that we discussed making a toml config parameter (something like renewalGracePeriod), with a default of say, 2 days, so that the process for requesting a new cert via ACME will begin 6+2=8 days before expiration, to allow for ACME server outages. Does this seem OK?
|
@twifkak 8 days are fine because OCSP next update is 7 days duration and we need not care about OCSP validation period against the cert expiry date. |
packager/util/util.go
Outdated
| @@ -78,14 +78,17 @@ func hasCanSignHttpExchangesExtension(cert *x509.Certificate) bool { | |||
|
|
|||
| // Returns the Duration of time before cert expires with given deadline. | |||
| // Note that the certExpiryDeadline should be the expected SXG expiration time. | |||
| // Returns -1 if cert is already expired. This will be used to periodically check if cert | |||
| // Returns 0 if cert is already expired. This will be used to periodically check if cert | |||
Addresses #93