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

Add support for Must-Staple / TLS-Feature #1224

Merged
merged 45 commits into from Feb 16, 2016
Merged
Changes from 35 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
6b5101c
Update CFSSL to latest
bifurcation Dec 4, 2015
02a98e8
Add support for the must-staple extension
bifurcation Nov 20, 2015
b4f202f
Add changes to CA tests
bifurcation Dec 15, 2015
580c79c
Merge branch 'master' into must-staple
bifurcation Dec 15, 2015
b48d3a3
Provide correct TLS-Feature extension value
bifurcation Dec 15, 2015
dfc7021
Add new test files
bifurcation Dec 15, 2015
54e336e
Merge branch 'master' into must-staple
bifurcation Dec 15, 2015
a439df4
Address review comments
bifurcation Dec 16, 2015
f95c5bb
Merge branch 'master' into must-staple
bifurcation Dec 16, 2015
b552d24
Remove stray variables
bifurcation Dec 16, 2015
0103dbb
Merge branch 'master' into must-staple
bifurcation Dec 16, 2015
d05e908
Merge branch 'master' into must-staple
bifurcation Dec 17, 2015
eb5a00f
Merge branch 'master' into must-staple
bifurcation Dec 18, 2015
02595e0
Merge branch 'master' into must-staple
bifurcation Jan 13, 2016
55d1795
Reset Godep changes
bifurcation Jan 13, 2016
e3daa6c
Factor out extension handling and require a specific value
bifurcation Jan 13, 2016
de8f1f5
Merge branch 'must-staple' of https://github.com/letsencrypt/boulder …
bifurcation Jan 13, 2016
2e74211
Merge branch 'master' into must-staple
bifurcation Jan 20, 2016
3411e9d
Merge branch 'master' into must-staple
bifurcation Jan 22, 2016
65ea6f6
Stricter error checking for extensions
bifurcation Jan 22, 2016
5020352
Merge branch 'master' into must-staple
bifurcation Jan 22, 2016
e588c52
Attempt to fix test failure
bifurcation Jan 22, 2016
d2f3a89
Merge branch 'must-staple' of https://github.com/letsencrypt/boulder …
bifurcation Jan 22, 2016
22bc89f
Merge branch 'master' into must-staple
bifurcation Jan 22, 2016
c1f1649
Back out RA change
bifurcation Jan 25, 2016
14fbf3b
Add stats and error only on a bad TLS Feature extension
bifurcation Jan 25, 2016
e243442
Merge branch 'master' into must-staple
bifurcation Jan 25, 2016
746fc88
Merge branch 'master' into must-staple
bifurcation Jan 26, 2016
4c3f14b
Merge branch 'master' into must-staple
bifurcation Jan 27, 2016
06ac584
Printf clean-up
bifurcation Jan 27, 2016
51c1a1c
Merge branch 'master' into must-staple
bifurcation Jan 27, 2016
0afa843
Merge branch 'master' into must-staple
bifurcation Jan 27, 2016
d9fdfac
Merge branch 'master' into must-staple
bifurcation Jan 28, 2016
4157112
Merge branch 'master' into must-staple
bifurcation Jan 28, 2016
1ec567e
Merge branch 'master' into must-staple
bifurcation Jan 29, 2016
5d35967
Ignore duplicate extensions
bifurcation Feb 16, 2016
4e31d80
Merge branch 'master' into must-staple
bifurcation Feb 16, 2016
42782be
Clean up metrics and don't use naked returns
bifurcation Feb 16, 2016
f1ccf2f
Merge branch 'must-staple' of https://github.com/letsencrypt/boulder …
bifurcation Feb 16, 2016
edd2471
Actually check for duplicate extensions
bifurcation Feb 16, 2016
4f81e8c
Test proper duplicate extension handling
bifurcation Feb 16, 2016
1d62ca8
Add duplicate-must-staple test CSR, and use it
bifurcation Feb 16, 2016
8a79e48
Merge branch 'master' into must-staple
rolandshoemaker Feb 16, 2016
901f651
Actually use the duplicate must staple CSR
bifurcation Feb 16, 2016
6fb913a
Merge branch 'must-staple' of https://github.com/letsencrypt/boulder …
bifurcation Feb 16, 2016
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -6,11 +6,13 @@
package ca

import (
"bytes"
"crypto"
"crypto/ecdsa"
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"encoding/asn1"
"encoding/base64"
"encoding/hex"
"encoding/json"
@@ -50,13 +52,67 @@ var badSignatureAlgorithms = map[x509.SignatureAlgorithm]bool{
x509.ECDSAWithSHA1: true,
}

// Miscellaneous PKIX OIDs that we need to refer to
var (
// X.509 Extensions
oidAuthorityInfoAccess = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 1}
oidAuthorityKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 35}
oidBasicConstraints = asn1.ObjectIdentifier{2, 5, 29, 19}
oidCertificatePolicies = asn1.ObjectIdentifier{2, 5, 29, 32}
oidCrlDistributionPoints = asn1.ObjectIdentifier{2, 5, 29, 31}
oidExtKeyUsage = asn1.ObjectIdentifier{2, 5, 29, 37}
oidKeyUsage = asn1.ObjectIdentifier{2, 5, 29, 15}
oidSubjectAltName = asn1.ObjectIdentifier{2, 5, 29, 17}
oidSubjectKeyIdentifier = asn1.ObjectIdentifier{2, 5, 29, 14}
oidTLSFeature = asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 24}

// CSR attribute requesting extensions
oidExtensionRequest = asn1.ObjectIdentifier{1, 2, 840, 113549, 1, 9, 14}
)

// OID and fixed value for the "must staple" variant of the TLS Feature
// extension:
//
// Features ::= SEQUENCE OF INTEGER [RFC7633]
// enum { ... status_request(5) ...} ExtensionType; [RFC6066]
//
// DER Encoding:
// 30 03 - SEQUENCE (3 octets)
// |-- 02 01 - INTEGER (1 octet)
// | |-- 05 - 5
var (
mustStapleFeatureValue = []byte{0x30, 0x03, 0x02, 0x01, 0x05}
mustStapleExtension = signer.Extension{
ID: cfsslConfig.OID(oidTLSFeature),
Critical: false,
Value: hex.EncodeToString(mustStapleFeatureValue),
}
)

// Metrics for CA statistics
const (
// Increments when CA observes an HSM fault
metricHSMFaultObserved = "CA.OCSP.HSMFault.Observed"

// Increments when CA rejects a request due to an HSM fault
metricHSMFaultRejected = "CA.OCSP.HSMFault.Rejected"

// Increments when CA handles a CSR requesting a "basic" extension:
// authorityInfoAccess, authorityKeyIdentifier, extKeyUsage, keyUsage,
// basicConstraints, certificatePolicies, crlDistributionPoints,
// subjectAlternativeName, subjectKeyIdentifier,
metricCSRExtensionBasic = "CA.OCSP.CSRExtensions.Basic"

// Increments when CA handles a CSR requesting a TLS Feature extension
metricCSRExtensionTLSFeature = "CA.OCSP.CSRExtensions.TLSFeature"

// Increments when CA handles a CSR requesting a TLS Feature extension with
// an invalid value
metricCSRExtensionTLSFeatureInvalid = "CA.OCSP.CSRExtensions.TLSFeatureInvalid"

// Increments when CA handles a CSR requesting an extension other than those
// listed above
metricCSRExtensionOther = "CA.OCSP.CSRExtensions.Other"
)

// CertificateAuthorityImpl represents a CA that signs certificates, CRLs, and
@@ -216,6 +272,57 @@ func (ca *CertificateAuthorityImpl) noteHSMFault(err error) {
return
}

// Extract supported extensions from a CSR. The following extensions are
// currently supported:
//
// * 1.3.6.1.5.5.7.1.24 - TLS Feature [RFC7633], with the "must staple" value.
// Any other value will result in an error.
//
// Other requested extensions are silently ignored.
func (ca *CertificateAuthorityImpl) extensionsFromCSR(csr *x509.CertificateRequest) (extensions []signer.Extension, err error) {
extensions = []signer.Extension{}
for _, attr := range csr.Attributes {
if !attr.Type.Equal(oidExtensionRequest) {
continue
}

for _, extList := range attr.Value {
for _, ext := range extList {
switch {
case ext.Type.Equal(oidTLSFeature):
ca.stats.Inc(metricCSRExtensionTLSFeature, 1, 1.0)
value, ok := ext.Value.([]byte)
if !ok {
msg := fmt.Sprintf("Mal-formed extension with OID %v", ext.Type)
err = core.CertificateIssuanceError(msg)
return

This comment has been minimized.

Copy link
@jsha

jsha Feb 1, 2016

Contributor

This method is too long for naked returns. Please use explicit return nil, core.CertificateIssuanceError(msg), and remove the named return types.

} else if !bytes.Equal(value, mustStapleFeatureValue) {
msg := fmt.Sprintf("Unsupported value for extension with OID %v", ext.Type)
ca.stats.Inc(metricCSRExtensionTLSFeatureInvalid, 1, 1.0)
err = core.CertificateIssuanceError(msg)
return
}

extensions = append(extensions, mustStapleExtension)

This comment has been minimized.

Copy link
@rolandshoemaker

rolandshoemaker Feb 1, 2016

Member

This seems to allow someone to request multiple copies of the extension which doesn't seem like something we should allow, maybe there are legitimate cases for that with other extensions though?

This comment has been minimized.

Copy link
@bifurcation

bifurcation Feb 1, 2016

Author Contributor

I don't think we need to support duplicate extensions, but I'm not that worried about forbidding it either. In practice, I don't expect this to be an issue. I don't even know how to create a CSR with duplicate extensions requested. Even if someone does figure out how to do it, having multiple copies of this one shouldn't be a problem. Maybe just add a comment for now, and if we support more extensions in the future, then we should address it?

This comment has been minimized.

Copy link
@jsha

jsha Feb 1, 2016

Contributor

I think Roland's right. We want to be conservative in the certificates we generate, and I think that includes refusing to generate a certificate with a repeated extension. For instance, someone could use that to generate arbitrarily large certificates, crudding up our database.

This comment has been minimized.

Copy link
@rolandshoemaker

rolandshoemaker Feb 1, 2016

Member

Mainly what I was worried about is that this would allow a user to create
arbitrarily sized certificates by just sending a massive CSR with a bunch
of duplicates. I haven't tested it but the x509.CertificateRequest type
seems to make this quite easy to create.

On Mon, Feb 1, 2016 at 1:50 PM bifurcation notifications@github.com wrote:

In ca/certificate-authority.go
#1224 (comment):

  •           switch {
    
  •           case ext.Type.Equal(oidTLSFeature):
    
  •               ca.stats.Inc(metricCSRExtensionTLSFeature, 1, 1.0)
    
  •               value, ok := ext.Value.([]byte)
    
  •               if !ok {
    
  •                   msg := fmt.Sprintf("Mal-formed extension with OID %v", ext.Type)
    
  •                   err = core.CertificateIssuanceError(msg)
    
  •                   return
    
  •               } else if !bytes.Equal(value, mustStapleFeatureValue) {
    
  •                   msg := fmt.Sprintf("Unsupported value for extension with OID %v", ext.Type)
    
  •                   ca.stats.Inc(metricCSRExtensionTLSFeatureInvalid, 1, 1.0)
    
  •                   err = core.CertificateIssuanceError(msg)
    
  •                   return
    
  •               }
    
  •               extensions = append(extensions, mustStapleExtension)
    

I don't think we need to support duplicate extensions, but I'm not that
worried about forbidding it either. In practice, I don't expect this to be
an issue. I don't even know how to create a CSR with duplicate extensions
requested. Even if someone does figure out how to do it, having multiple
copies of this one shouldn't be a problem. Maybe just add a comment for
now, and if we support more extensions in the future, then we should
address it?


Reply to this email directly or view it on GitHub
https://github.com/letsencrypt/boulder/pull/1224/files#r51486553.

This comment has been minimized.

Copy link
@rolandshoemaker
case ext.Type.Equal(oidAuthorityInfoAccess),
ext.Type.Equal(oidAuthorityKeyIdentifier),
ext.Type.Equal(oidBasicConstraints),
ext.Type.Equal(oidCertificatePolicies),
ext.Type.Equal(oidCrlDistributionPoints),
ext.Type.Equal(oidExtKeyUsage),
ext.Type.Equal(oidKeyUsage),
ext.Type.Equal(oidSubjectAltName),
ext.Type.Equal(oidSubjectKeyIdentifier):
ca.stats.Inc(metricCSRExtensionBasic, 1, 1.0)

This comment has been minimized.

Copy link
@jsha

jsha Feb 1, 2016

Contributor

This will increment once for each basic extension in the CSR, which doesn't quite match the comment "Increments when CA handles a CSR requesting a "basic" extension."

I'm not sure what value we get from this metric, either. I would probably just drop it.

default:
ca.stats.Inc(metricCSRExtensionOther, 1, 1.0)
}
}
}
}
return
}

// GenerateOCSP produces a new OCSP response and returns it
func (ca *CertificateAuthorityImpl) GenerateOCSP(xferObj core.OCSPSigningRequest) ([]byte, error) {
if err := ca.checkHSMFault(); err != nil {
@@ -315,6 +422,11 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
}
}

requestedExtensions, err := ca.extensionsFromCSR(&csr)
if err != nil {
return emptyCert, err
}

notAfter := ca.clk.Now().Add(ca.validityPeriod)

if ca.notAfter.Before(notAfter) {
@@ -366,7 +478,8 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest
Subject: &signer.Subject{
CN: commonName,
},
Serial: serialBigInt,
Serial: serialBigInt,
Extensions: requestedExtensions,
}

certPEM, err := ca.signer.Sign(req)
@@ -96,6 +96,24 @@ var (
// Edited signature to become invalid.
WrongSignatureCSR = mustRead("./testdata/invalid_signature.der.csr")

// CSR generated by Go:
// * Random public key
// * CN = not-example.com
// * Includes an extensionRequest attribute for a well-formed TLS Feature extension
MustStapleCSR = mustRead("./testdata/must_staple.der.csr")

// CSR generated by Go:
// * Random public key
// * CN = not-example.com
// * Includes an extensionRequest attribute for an empty TLS Feature extension
TLSFeatureUnknownCSR = mustRead("./testdata/tls_feature_unknown.der.csr")

This comment has been minimized.

Copy link
@jsha

jsha Jan 21, 2016

Contributor

There should be one more test case: TLS feature extension OID, but the value didn't match the must staple value.

It may be worthwhile to factor the test function out into a set of testCase structs, as we do elsewhere in the codebase. Up to you, though.

This comment has been minimized.

Copy link
@bifurcation

bifurcation Jan 22, 2016

Author Contributor

The test case you're asking for already exists -- that's what TLSFeatureUnknownCSR is.

This comment has been minimized.

Copy link
@jsha

jsha Jan 22, 2016

Contributor

Ah, got it. I was thinking there was a fourth case here, but I was wrong.


// CSR generated by Go:
// * Random public key
// * CN = not-example.com
// * Includes an extensionRequest attribute for the CT Poison extension (not supported)
UnsupportedExtensionCSR = mustRead("./testdata/unsupported_extension.der.csr")

// CSR generated by Go:
// * Random ECDSA public key.
// * CN = [none]
@@ -204,6 +222,9 @@ func setup(t *testing.T) *testCtx {
PublicKey: true,
SignatureAlgorithm: true,
},
AllowedExtensions: []cfsslConfig.OID{
cfsslConfig.OID(oidTLSFeature),
},
},
ecdsaProfileName: &cfsslConfig.SigningProfile{
Usage: []string{"digital signature", "server auth"},
@@ -591,3 +612,51 @@ func TestHSMFaultTimeout(t *testing.T) {
test.AssertEquals(t, ctx.stats.Counters[metricHSMFaultObserved], int64(2))
test.AssertEquals(t, ctx.stats.Counters[metricHSMFaultRejected], int64(4))
}

func TestExtensions(t *testing.T) {
ctx := setup(t)
defer ctx.cleanUp()
ctx.caConfig.MaxNames = 3
ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy)
ca.Publisher = &mocks.Publisher{}
ca.PA = ctx.pa
ca.SA = ctx.sa

// A TLS feature extension should put a must-staple extension into the cert
csr, _ := x509.ParseCertificateRequest(MustStapleCSR)
cert, err := ca.IssueCertificate(*csr, ctx.reg.ID)
test.AssertNotError(t, err, "Failed to gracefully handle a CSR with must_staple")
parsedCert1, err := x509.ParseCertificate(cert.DER)
test.AssertNotError(t, err, "Error parsing certificate produced by CA")

foundMustStaple := false
for _, ext := range parsedCert1.Extensions {
if ext.Id.Equal(oidTLSFeature) {
foundMustStaple = true
test.Assert(t, !ext.Critical, "Extension was marked critical")
test.AssertByteEquals(t, ext.Value, mustStapleFeatureValue)
}
}
test.Assert(t, foundMustStaple, "TLS Feature extension not found")
test.AssertEquals(t, ctx.stats.Counters[metricCSRExtensionTLSFeature], int64(1))

// ... but if it doesn't ask for stapling, there should be an error
csr, _ = x509.ParseCertificateRequest(TLSFeatureUnknownCSR)
cert, err = ca.IssueCertificate(*csr, ctx.reg.ID)
test.AssertError(t, err, "Allowed a CSR with an empty TLS feature extension")
test.AssertEquals(t, ctx.stats.Counters[metricCSRExtensionTLSFeature], int64(2))
test.AssertEquals(t, ctx.stats.Counters[metricCSRExtensionTLSFeatureInvalid], int64(1))

// Unsupported extensions should be silently ignored, having the same
// extensions as the TLS Feature cert above, minus the TLS Feature Extension
csr, _ = x509.ParseCertificateRequest(UnsupportedExtensionCSR)
cert, err = ca.IssueCertificate(*csr, ctx.reg.ID)
test.AssertNotError(t, err, "Failed to gracefully handle a CSR with an unknown extension")
parsedCert2, err := x509.ParseCertificate(cert.DER)
test.AssertNotError(t, err, "Error parsing certificate produced by CA")
test.AssertEquals(t, len(parsedCert2.Extensions), len(parsedCert1.Extensions)-1)
test.AssertEquals(t, ctx.stats.Counters[metricCSRExtensionOther], int64(1))

// None of the above CSRs have basic extensions
test.AssertEquals(t, ctx.stats.Counters[metricCSRExtensionBasic], int64(0))
}
Binary file not shown.
Binary file not shown.
Binary file not shown.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.