Output warnings for cert within 6 months expiry, check intermediate cert expiry #802
Conversation
riyazdf
force-pushed
the
cert-warnings-int-expiry
branch
2 times, most recently
from
0e019a5
to
7701deb
Compare
| @@ -233,6 +233,10 @@ func ValidateCertificate(c *x509.Certificate) error { | |||
| if (tomorrow).Before(c.NotBefore) || now.After(c.NotAfter) { | |||
| return fmt.Errorf("certificate is expired") | |||
| } | |||
| // If this certificate is expiring within 6 months, put out a warning | |||
| if (c.NotAfter).Before(time.Now().AddDate(0, 6, 0)) { | |||
| logrus.Warn("certificate is expiring within 6 months") | |||
There was a problem hiding this comment.
what other info can we include to make it clear which certificate is expiring?
|
Thanks for checking the intermediate certs too! If it isn't too much trouble, can we add a check for the SHA1 algorithms too for the Other than that, this LGTM! |
|
@cyli of course, great idea! Done. |
riyazdf
force-pushed
the
cert-warnings-int-expiry
branch
2 times, most recently
from
84b60d2
to
e5957a4
Compare
…y in root Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
riyazdf
force-pushed
the
cert-warnings-int-expiry
branch
from
e5957a4
to
2952229
Compare
| tomorrow := now.AddDate(0, 0, 1) | ||
| // Give one day leeway on creation "before" time, check "after" against today | ||
| if (tomorrow).Before(c.NotBefore) || now.After(c.NotAfter) { | ||
| return fmt.Errorf("certificate with CN %s is expired", c.Subject.CommonName) |
There was a problem hiding this comment.
The whole if checkExpiry block should probably be moved to the end of the function. This return here will cause us to accept delegation certs in tuf/tuf.go line 255 that may have serious flaws but have expired and we returned from this validation prematurely, c.f. comment in tuf/tuf.go line 249 Currently we do not reject expired delegation keys but warn if they might expire soon or have already
There was a problem hiding this comment.
good catch, updated!
riyazdf
force-pushed
the
cert-warnings-int-expiry
branch
2 times, most recently
from
1c7d922
to
e146938
Compare
certs Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
riyazdf
force-pushed
the
cert-warnings-int-expiry
branch
from
e146938
to
fb03348
Compare
|
LGTM |
Addresses #734 for root certificates, and cleans up some of the certificate validation logic. Also ensures we use non-expired and non-SHA1 certificates for intermediate certs. Though there is no way to include specific certs to the root at the moment, we will need this validation.
Last commit closes #860
Signed-off-by: Riyaz Faizullabhoy riyaz.faizullabhoy@docker.com