Skip to content
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

Merged
merged 3 commits into from
Aug 9, 2016

Conversation

riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Jun 24, 2016

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]

@@ -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")
Copy link
Contributor

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?

@cyli
Copy link
Contributor

cyli commented Jul 11, 2016

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 ValidateCertificate tests? If not, no worries.

Other than that, this LGTM!

@riyazdf
Copy link
Contributor Author

riyazdf commented Jul 11, 2016

@cyli of course, great idea! Done.

@riyazdf riyazdf force-pushed the cert-warnings-int-expiry branch 2 times, most recently from 84b60d2 to e5957a4 Compare July 25, 2016 20:46
@riyazdf riyazdf self-assigned this Jul 25, 2016
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)
Copy link
Contributor

@endophage endophage Aug 8, 2016

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, updated!

@riyazdf riyazdf force-pushed the cert-warnings-int-expiry branch 2 times, most recently from 1c7d922 to e146938 Compare August 9, 2016 00:14
@endophage
Copy link
Contributor

LGTM

@endophage endophage merged commit dba9c36 into master Aug 9, 2016
@riyazdf riyazdf deleted the cert-warnings-int-expiry branch August 9, 2016 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn if a delegation key certificate is expiring
4 participants