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

crypto/x509: CreateCertificateRequest cannot write extensions with critical #13739

Closed
nejisama opened this Issue Dec 27, 2015 · 17 comments

Comments

Projects
None yet
7 participants
@nejisama
Copy link

nejisama commented Dec 27, 2015

I found the newest code add a new function parseCSRExtension to fixed a bug that ParseCertificateRequest cannot read the certificate request with a critical flag in request extensions; but in CreateCertificateRequest, the critical flag will be lose; the reason is same as the parse bug.

@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented Dec 28, 2015

This bug report is incomplete (or at least quite confusing). It needs to answer:

  • What did you do?
  • What happened?
  • What did you expect to happen instead?

A test program would be great.

@nejisama

This comment has been minimized.

Copy link

nejisama commented Jan 19, 2016

i want to write a certificaterequest, which has a extension with a critical flag

extensions := []pkix.Extension{
pkix.Extension{Id: oidExtensionKeyUsage, Critical: true, Value: value} //value is an asn1 encode data
}
csr := x509.CertificateRequest {
//...
ExtraExtensions:extensions,
}
x509.CreateCertificateRequest(rand.Reader, csr, privkey)
//output

i want to get a csr has attriubutes like:

 Requested Extensions:
            X509v3 Key Usage: critical
                Certificate Sign, CRL Sign

but the output is :

 Requested Extensions:
X509v3 Key Usage: 
         Certificate Sign, CRL Sign

the critical flag is lost

@nejisama

This comment has been minimized.

Copy link

nejisama commented Jan 19, 2016

the issue cloudflare/cfssl#346 fixed a read bug in go 1.6, but i want to generate a csr

@bradfitz bradfitz changed the title crypto/x509 CreateCertificateRequest cannot write extensions with cirtical crypto/x509 CreateCertificateRequest cannot write extensions with critical Jan 21, 2016

@bradfitz bradfitz changed the title crypto/x509 CreateCertificateRequest cannot write extensions with critical crypto/x509: CreateCertificateRequest cannot write extensions with critical Jan 21, 2016

@bradfitz bradfitz added this to the Unplanned milestone Jan 21, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 21, 2016

Please post a complete program, not just a snippet.

I don't see why ExtraExtensions woudn't work. It's documented to include them:

    // ExtraExtensions contains extensions to be copied, raw, into any
    // marshaled CSR. Values override any extensions that would otherwise
    // be produced based on the other fields but are overridden by any
    // extensions specified in Attributes.
    //
    // The ExtraExtensions field is not populated when parsing CSRs, see
    // Extensions.
    ExtraExtensions []pkix.Extension

What do you get when you then Parse it? Again, show a complete program please.

@nejisama

This comment has been minimized.

Copy link

nejisama commented Jan 22, 2016

hello, ExtraExtensions works, but lose the critical flag.

package main

import (
    "bytes"
    "crypto/rand"
    "crypto/rsa"
    "crypto/x509"
    "crypto/x509/pkix"
    "encoding/asn1"
    "encoding/pem"
    "fmt"
)

func example() {
    subject := pkix.Name{
        Country:            []string{"CN"},
        Organization:       []string{"Test X509"},
        OrganizationalUnit: []string{"Test X509 Unit"},
        CommonName:         "testX509 Csr",
    }   
    type basicConstraints struct {
        IsCA       bool `asn1:"optional"`
        MaxPathLen int  `asn1:"optional,default:-1"`
    }   
    var extensions []pkix.Extension
    basicCon := basicConstraints{IsCA: true, MaxPathLen: -1} 
    bitstr, err := asn1.Marshal(basicCon)
    if err != nil {
        //  
    }   
    var oidExtensionBasicConstraints = []int{2, 5, 29, 19} //export from x509 package
    //notice: i set Critical flag here
    bcExt := pkix.Extension{Id: oidExtensionBasicConstraints, Critical: true, Value: bitstr}
    extensions = append(extensions, bcExt)
    csrTmpl := &x509.CertificateRequest{
        Subject:            subject,
        SignatureAlgorithm: x509.SHA1WithRSA,
        ExtraExtensions:    extensions,
    }   
    priv, err := rsa.GenerateKey(rand.Reader, 2048)
    if err != nil {
        //  
    }   
    csr, err := x509.CreateCertificateRequest(rand.Reader, csrTmpl, priv)
    if err != nil {
        //  
    }   
    //pem encode and output
    block := &pem.Block{
        Type:  "CERTIFICATE REQUEST", 
        Bytes: csr,
    }   
    var buf bytes.Buffer
    err = pem.Encode(&buf, block)
    if err != nil {
        //
    }
    fmt.Println(buf.String())
}

func main() {
    example()
}
test > myout.csr
openssl req -in myout.csr -text

I want to get the csr have the Attributes

Attributes:
Requested Extensions:
            X509v3 Basic Constraints: critical
                CA:TRUE

but what i get is

Attributes:
Requested Extensions:
            X509v3 Basic Constraints: 
                CA:TRUE

the critical flag is lost

@nejisama

This comment has been minimized.

Copy link

nejisama commented Jan 22, 2016

when i parse it

csrStruct, err := x509.ParseCertificateRequest(csrBytes)
fmt.Println(csrStruct.Extensions[0])
output:
{2.5.29.19 false [48 3 1 1 255]}

this output do not match what I input in the template (critical should be true,but get fasle)

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 22, 2016

Here is a functional & much more minimal version of what you supplied above:

package main

import (
        "crypto/rand"
        "crypto/rsa"
        "crypto/x509"
        "crypto/x509/pkix"
        "encoding/asn1"
        "fmt"
        "log"
)

func main() {
        type basicConstraints struct {
                IsCA       bool `asn1:"optional"`
                MaxPathLen int  `asn1:"optional,default:-1"`
        }
        basicCon := basicConstraints{IsCA: true, MaxPathLen: -1}
        bitstr, err := asn1.Marshal(basicCon)
        if err != nil {
                log.Fatalf("Marshal: %v", err)
        }

        var oidExtensionBasicConstraints = []int{2, 5, 29, 19} // from x509 package
        bcExt := pkix.Extension{Id: oidExtensionBasicConstraints, Critical: true, Value: bitstr}
        fmt.Printf("Writing extension: %#v\n", bcExt)

        csrTmpl := &x509.CertificateRequest{
                SignatureAlgorithm: x509.SHA1WithRSA,
                ExtraExtensions:    []pkix.Extension{bcExt},
        }
        priv, err := rsa.GenerateKey(rand.Reader, 2048)
        if err != nil {
                log.Fatalf("GenerateKey: %v", err)
        }
        csr, err := x509.CreateCertificateRequest(rand.Reader, csrTmpl, priv)
        if err != nil {
                log.Fatalf("CreateCertificateRequest: %v", err)
        }

        back, err := x509.ParseCertificateRequest(csr)
        if err != nil {
                log.Fatalf("ParseCertificateRequest: %v", err)
        }
        fmt.Printf(" Parsed extension: %#v\n", back.Extensions[0])
}

It outputs:

$ go run csr.go
Writing extension: pkix.Extension{Id:asn1.ObjectIdentifier{2, 5, 29, 19}, Critical:true, Value:[]uint8{0x30, 0x3, 0x1, 0x1, 0xff}}
 Parsed extension: pkix.Extension{Id:asn1.ObjectIdentifier{2, 5, 29, 19}, Critical:false, Value:[]uint8{0x30, 0x3, 0x1, 0x1, 0xff}}
@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jan 22, 2016

From the x509.go code:

    atvs = append(atvs, pkix.AttributeTypeAndValue{
       // There is no place for the critical flag in a CSR.                                                                                                             
       Type:  e.Id,
       Value: e.Value,
    })

Maybe CreateCertificateRequest should return an error if you're trying to set critical, rather than discard it?

And maybe it should be documented.

@agl?

@agl agl self-assigned this Mar 11, 2016

@evantill

This comment has been minimized.

Copy link

evantill commented Oct 13, 2017

I all, I think I am impacted by this problem. How can I help on this issue ?

@evantill

This comment has been minimized.

Copy link

evantill commented Oct 13, 2017

there is a comment // There is no place for the critical flag in a CSR. at #L2164
But in my case I need to create a certificate with the Critical flag on the timestampingextended usage. How can I proceed ?

@bradfitz I have posted on the forum my question with more details

@agl

This comment has been minimized.

Copy link
Contributor

agl commented Oct 13, 2017

What caught my eye here is that OpenSSL's debug output can show a CSR extension as critical. From looking at the source, they are stuffing PKIX Extension objects into CSR attributes.

I don't know if I got lost in the old PKCS documents, or whether this is a case where reality diverges from the spec, but doing what OpenSSL does seems valid here.

I'll upload something in a sec.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Oct 13, 2017

Change https://golang.org/cl/70851 mentions this issue: crypto/x509: follow OpenSSL and emit Extension structures directly in CSRs.

@evantill

This comment has been minimized.

Copy link

evantill commented Oct 14, 2017

Thanks @agl.

If I understand your fix, we will be able to set the critical flag in the ExtraExtensions correct ?
but at line #2173 you're skiping the extra extension if the attribute already exist...

In my case I need to set the Critical flag on the extended key usage stamping (which I actually pass as a ExtKeyUsage. How would I proceed ?

@evantill

This comment has been minimized.

Copy link

evantill commented Oct 14, 2017

Creating a certificate with an extended key usage as critical seems to work :

go version go1.9.1 darwin/amd64

https://gist.github.com/evantill/ebeb9535458c108e35207e0dbf6fe351

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Oct 14, 2017

@evantill would you mind commenting on the CL instead? That way @agl doesn't have context switching between different platforms, as he receives a lot of notifications on Github, plus any comments with the CL will be easier to follow along for anyone :) Thank you.

@evantill

This comment has been minimized.

Copy link

evantill commented Oct 14, 2017

@odeke-em sure I will swith on the CL.

@agl

This comment has been minimized.

Copy link
Contributor

agl commented Dec 31, 2017

@evantill You need to not include extensions in the Attributes is all.

@gopherbot gopherbot closed this in 0b37f05 Mar 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment