-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Output warnings for cert within 6 months expiry, check intermediate cert expiry #802
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
84b60d2
to
e5957a4
Compare
…y in root Signed-off-by: Riyaz Faizullabhoy <[email protected]>
Signed-off-by: Riyaz Faizullabhoy <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, updated!
1c7d922
to
e146938
Compare
certs Signed-off-by: Riyaz Faizullabhoy <[email protected]>
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 [email protected]