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

Conversation

@bifurcation
Copy link
Contributor

commented Dec 4, 2015

This PR updates the vendorized copy of CFSSL to the latest version, which allows calling code to request extensions. It then inspects submitted CSRs to see if they request the TLS-Feature extension and if so, adds that extension to the request that is passed to CFSSL.

Obviously, we don't want to accept arbitrary extensions from clients. CFSSL has a configuration option that whitelists extensions, so I think we should rely on that for filtering. As it is now, extensions that boulder doesn't support are silently ignored; extensions supported by boulder but not turned on in CFSSL will result in an error. For now, hard-coding TLS-Feature seems like an OK compromise, but we might want to revisit the config story here.

@jmhodges

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2015

FYI these unit test failures are real! Got some code to fix up

@jmhodges

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2015

Barnes, are you going to work on this or...?

@jmhodges

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2015

Baaaaaaaaaaarnes

@osirisinferi

This comment has been minimized.

Copy link

commented Dec 14, 2015

You sure the DER encoding of "0303020105" is correct?

I've compiled the master branch of OpenSSL today (with Must Staple support since a few days) and someone from the community gave me this option for openssl.cnf:

1.3.6.1.5.5.7.1.24=DER:3003020105

This correctly resulted in:

        TLS Feature: 
            status_request

But the value of "0303020105" gave:

        TLS Feature: 
            .....

Can't be good, right?

I'm pretty sure it should be 30..., a Constructed content with length of 3 with a content of an Integer with a length of 1 and value of 5. Not a Primitive bit string which doesn't add up 😛

@selecadm

This comment has been minimized.

Copy link

commented Dec 14, 2015

If only I knew this is where the problem is, I would at least have looked at this.

02a98e8#diff-128aca6dfc1169ea56f9e24336a39104R62

It should be:

var mustStapleFeatureValue = "3003020105"

@bifurcation

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2015

@osirisinferi @selecadm Thanks a lot. Looks like I got it right in the comments, then failed to transcribe it properly to code. Should be fixed in b48d3a3

@bifurcation

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2015

Alright, lights are green, go for re-r?

@bifurcation bifurcation added r? and removed needs-revision labels Dec 15, 2015
//
// Other requested extensions are silently ignored.
//
// XXX(rlb): It might be good to add a generic mechanism for extensions, but

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

We don't use XXX comments anymore: https://github.com/letsencrypt/boulder/wiki/Boulder-Development#patch-guidelines (or rather, we use them to mark something we intend to change before merging). Please turn this into a TODO with a link to an issue or email address, or just remove.

// on the other hand, we probably *don't* want to copy arbitrary client-
// provided data into extensions.
extensions := core.ExtensionsFromCSR(&csr)
requestedExtensions := []signer.Extension{}

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

Why not default this to nil instead, and turn the append below into an assignment?

This comment has been minimized.

Copy link
@bifurcation

bifurcation Dec 16, 2015

Author Contributor

If we want to support other extensions in the future, append() will be more natural, and it's not expensive now.

// provided data into extensions.
extensions := core.ExtensionsFromCSR(&csr)
requestedExtensions := []signer.Extension{}
if _, present := extensions[oidTLSFeature.String()]; present {

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

Rather than calling .String() repeatedly, call it in the var setting above.

// XXX(rlb): It might be good to add a generic mechanism for extensions, but
// on the other hand, we probably *don't* want to copy arbitrary client-
// provided data into extensions.
extensions := core.ExtensionsFromCSR(&csr)

This comment has been minimized.

Copy link
@jmhodges

jmhodges Dec 15, 2015

Contributor

This is only used here, so it doesn't need to go in core. Drop a file in the ca package that has this func def in it.

This comment has been minimized.

Copy link
@bifurcation

bifurcation Dec 16, 2015

Author Contributor

After looking at how x509.ParseCertificateRequest handles requested extensions, I just folded this method into ca.IssueCertificate.

)

// ExtensionsFromCSR extracts the set of requested extensions from a CSR
func ExtensionsFromCSR(csr *x509.CertificateRequest) map[string][]byte {

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

Don't put this in core, put it in CA where it is used, and make it unexported. If we later find we need to share it across packages, we can abstract it out into a new package.

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

Also, please add a test for this function.

// ExtensionsFromCSR extracts the set of requested extensions from a CSR
func ExtensionsFromCSR(csr *x509.CertificateRequest) map[string][]byte {
extensionMap := map[string][]byte{}
for _, attr := range csr.Attributes {

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

Why are we using Attributes here? The docs say:

// Attributes is a collection of attributes providing
// additional information about the subject of the certificate.
// See RFC 2986 section 4.1.

I think we want the Extensions field.

This comment has been minimized.

Copy link
@bifurcation

bifurcation Dec 16, 2015

Author Contributor

Nope, those are CSR extensions, not certificate extensions. The way you request certificate extensions in a CSR is with the extensionRequest attribute.

attributes is a collection of attributes providing additional
information about the subject of the certificate. ... Another example is information to
appear in X.509 certificate extensions (e.g. the
extensionRequest attribute from PKCS #9).

You can see crypto/x509 builds the DomainNames list in the same way.

func ExtensionsFromCSR(csr *x509.CertificateRequest) map[string][]byte {
extensionMap := map[string][]byte{}
for _, attr := range csr.Attributes {
if !attr.Type.Equal(oidExtensionRequest) || len(attr.Value) != 1 {

This comment has been minimized.

Copy link
@jsha

jsha Dec 15, 2015

Contributor

Modulo the above comment, I think it makes sense to skip over things that are not relevant, but if we find a relevant thing and it has the wrong length, that should be an error.

@jmhodges jmhodges added needs-revision and removed r? labels Dec 15, 2015
@osirisinferi

This comment has been minimized.

Copy link

commented on core/util.go in a439df4 Dec 16, 2015

Forgive me if I'm saying something really stupid, as I'm not a Go programmer, but should these two variables be left here in core/util, as oidExtensionRequest is again declared in ca/certificate-authority in the latest commit?

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
@rolandshoemaker

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

Other than one comment LGTM, extensions are confusing.

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.

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.

@jsha jsha added the needs-revision label Feb 1, 2016
@jsha jsha added r=jsha and removed needs-revision labels Feb 16, 2016

for _, extList := range attr.Value {
for _, ext := range extList {
if extensionSeen[ext.Type.String()] {

This comment has been minimized.

Copy link
@rolandshoemaker

rolandshoemaker Feb 16, 2016

Member

This map is never added to so this check will always fail.

This comment has been minimized.

Copy link
@jsha

jsha Feb 16, 2016

Contributor

Nice catch. We should have an additional test case for that situation.

@jsha jsha added r=jsha and removed r=jsha labels Feb 16, 2016
rolandshoemaker added a commit that referenced this pull request Feb 16, 2016
Add support for Must-Staple / TLS-Feature
@rolandshoemaker rolandshoemaker merged commit 1089371 into master Feb 16, 2016
6 checks passed
6 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fmt
Details
integration
Details
lint
Details
migrations
Details
vet
Details
jsha added a commit to letsencrypt/cfssl that referenced this pull request Feb 18, 2016
Previously, we would hard fail on any CSR extensions that weren't whitelisted.
However, as Hugo pointed out in
letsencrypt/boulder#1224 (comment),
this will break a large number of clients, which include extensions in the CSR
that we don't explicitly process. The new behavior is to copy only the
whitelisted extensions into the certificate, and ignore the rest.
jsha added a commit to letsencrypt/cfssl that referenced this pull request Feb 18, 2016
Previously, we would hard fail on any CSR extensions that weren't whitelisted.
However, as Hugo pointed out in
letsencrypt/boulder#1224 (comment),
this will break a large number of clients, which include extensions in the CSR
that we don't explicitly process. The new behavior is to copy only the
whitelisted extensions into the certificate, and ignore the rest.
jsha added a commit to letsencrypt/cfssl that referenced this pull request Feb 19, 2016
Previously, we would hard fail on any CSR extensions that weren't whitelisted.
However, as Hugo pointed out in
letsencrypt/boulder#1224 (comment),
this will break a large number of clients, which include extensions in the CSR
that we don't explicitly process. The new behavior is to copy only the
whitelisted extensions into the certificate, and ignore the rest.
@riking riking deleted the must-staple branch Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.