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.

Sign up for GitHub

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

Changing minimum certificate rotation period and backdating to 1h #1243

Merged
merged 1 commit into from
Aug 1, 2016
Merged

Changing minimum certificate rotation period and backdating to 1h #1243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions ca/certificates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,17 @@ const (
RootCAExpiration = "630720000s"
// DefaultNodeCertExpiration represents the default expiration for node certificates (3 months)
DefaultNodeCertExpiration = 2160 * time.Hour
// CertBackdate represents the amount of time each certificate is backdated to try to avoid
// clock drift issues.
CertBackdate = -1 * time.Hour
// CertLowerRotationRange represents the minimum fraction of time that we will wait when randomly
// choosing our next certificate rotation
CertLowerRotationRange = 0.5
// CertUpperRotationRange represents the maximum fraction of time that we will wait when randomly
// choosing our next certificate rotation
CertUpperRotationRange = 0.8
// MinNodeCertExpiration represents the minimum expiration for node certificates (25 + 5 minutes)
// X - 5 > CertUpperRotationRange * X <=> X < 5/(1 - CertUpperRotationRange)
// Since we're issuing certificates 5 minutes in the past to get around clock drifts, and
// we're selecting a random rotation distribution range from CertLowerRotationRange to
// CertUpperRotationRange, we need to ensure that we don't accept an expiration time that will
// make a node able to randomly choose the next rotation after the expiration of the certificate.
MinNodeCertExpiration = 30 * time.Minute
// MinNodeCertExpiration represents the minimum expiration for node certificates
MinNodeCertExpiration = 1 * time.Hour
)

// ErrNoLocalRootCA is an error type used to indicate that the local root CA
Expand Down
8 changes: 4 additions & 4 deletions ca/certificates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,12 +534,12 @@ func TestNewRootCANonDefaultExpiry(t *testing.T) {
parsedCerts, err := helpers.ParseCertificatesPEM(cert)
assert.NoError(t, err)
assert.Len(t, parsedCerts, 2)
assert.True(t, time.Now().Add(time.Minute*50).Before(parsedCerts[0].NotAfter))
assert.True(t, time.Now().Add(time.Hour).After(parsedCerts[0].NotAfter))
assert.True(t, time.Now().Add(time.Minute*59).Before(parsedCerts[0].NotAfter))
assert.True(t, time.Now().Add(time.Hour).Add(time.Minute).After(parsedCerts[0].NotAfter))

// Sign the same CSR again, this time with a 29 Minute expiration RootCA (under the 30 minute minimum).
// Sign the same CSR again, this time with a 59 Minute expiration RootCA (under the 60 minute minimum).
// This should use the default of 3 months
newRootCA, err = ca.NewRootCA(rootCA.Cert, rootCA.Key, 29*time.Minute)
newRootCA, err = ca.NewRootCA(rootCA.Cert, rootCA.Key, 59*time.Minute)
assert.NoError(t, err)

cert, err = newRootCA.ParseValidateAndSignCSR(csr, "CN", ca.ManagerRole, "ORG")
Expand Down
10 changes: 8 additions & 2 deletions ca/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,16 @@ func SigningPolicy(certExpiry time.Duration) *cfconfig.Signing {
certExpiry = DefaultNodeCertExpiration
}

now := time.Now()
notBefore := now.Round(time.Minute).Add(CertBackdate).UTC()
notAfter := now.Round(time.Minute).Add(certExpiry).UTC()

return &cfconfig.Signing{
Default: &cfconfig.SigningProfile{
Usage: []string{"signing", "key encipherment", "server auth", "client auth"},
Expiry: certExpiry,
Usage: []string{"signing", "key encipherment", "server auth", "client auth"},
Expiry: certExpiry,
NotBefore: notBefore,
NotAfter: notAfter,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably a dumb TLS question, but is it correct to specify both NotBefore / NotAfter and also Expiry? Does cfssl use both of them?

Copy link
Contributor Author

@diogomonica diogomonica Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

// Only trust the key components from the CSR. Everything else should
// come directly from API call params.
CSRWhitelist: &cfconfig.CSRWhitelist{
Expand Down