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

crypto/x509: cannot generate version 1 or 2 certificates #21593

Closed
CRThaze opened this issue Aug 24, 2017 · 7 comments
Closed

crypto/x509: cannot generate version 1 or 2 certificates #21593

CRThaze opened this issue Aug 24, 2017 · 7 comments
Assignees
Milestone

Comments

@CRThaze
Copy link

@CRThaze CRThaze commented Aug 24, 2017

Questionarie

What version of Go are you using (go version)?

1.9rc2_cl165246139

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

package main

import (
	"crypto/rand"
	"crypto/rsa"
	"crypto/x509"
	"crypto/x509/pkix"
	"fmt"
	"io/ioutil"
	"math/big"
	"time"
)

func main() {
	privatekey, err := rsa.GenerateKey(rand.Reader, 4096)
	if err != nil {
		fmt.Println(err)
	}

	publickey := &privatekey.PublicKey

	caTemplate := &x509.Certificate{
		Version: 1,
		SerialNumber: big.NewInt(1),
		Subject: pkix.Name{
			Country:            []string{"US"},
			Province:           []string{"California"},
			Locality:           []string{"Mountain View"},
			Organization:       []string{"Acme"},
			OrganizationalUnit: []string{"Security"},
			CommonName:         "acme-dev",
		},
		NotBefore: time.Now(),
		NotAfter:  time.Now().AddDate(10, 0, 0),
	}
  
	cert, err := x509.CreateCertificate(rand.Reader, caTemplate, caTemplate, publickey, privatekey)
	if err != nil {
		fmt.Println(err)
	}
	ioutil.WriteFile("cert.pem", cert, 0777)
	fmt.Println("certificate saved to cert.pem")
}  

What did you expect to see?

The preceding Go program (shared here as source instead of a play.golang.org link since the crypto work takes too long and the tool times out) should generate a Version 1 x509 self singed certificate.

What did you see instead?

As can be seen by running openssl x509 -in cert.pem -inform der -text -noout on the resulting certificate file, this will instead generate a version 3 certificate.

Observations

x509 versions 2 & 3 merely support extra fields, on top of those provided by preceding versions. And so a version 3 certificate which does not use any version 2 or 3 specific fields would still be functionally identical to a version 1 certificate. However, I would expect that the crypto/x509 library should support applying the specified version to the certificate, as well as ensure that no fields which are not supported by a given version are being used.

Upon inspection of the x509.CreateCertificate() function's source code it appears that the version is simply hard-coded to v3

Proposed Fix

As near as I can tell, all that needs to be done here is to provide a check that only fields supported by the specified version are being used, and then pass the value of template.Version to the tbsCertificate struct.

Or rather, passing template.Version - 1 to the struct, so that we may use the more readable version values of 1 - 3 instead of the zero-index value used by the tbsCertificate. This also allows us to not break existing functionality and continue to use v3 if no value for Version is specified (and thus set to 0).

Something along the lines of:

if template.Version == 0 {
  template.Version = 3
}

if template.Version < 0 || template.Version > 3 {
	return nil, errors.New(fmt.Sprintf("x509: invalid version '%d'", template.Version))
} else if template.Version == 1 && (len(template.AuthorityKeyId) > 0 ||
	len(template.SubjectKeyId) > 0 ||
	len(template.ExtKeyUsage) > 0 ||
	template.KeyUsage != 0 ||
	template.BasicConstraintsValid == true) {
	return nil, errors.New("x509: version 1 does not support AuthorityKeyId, SubjectKeyId, BasicConstraints, KeyUsage, or ExtKeyUsage")
} else if template.Version == 2 && (len(template.ExtKeyUsage) > 0 ||
	template.KeyUsage != 0 ||
	template.BasicConstraintsValid == true) {
	return nil, errors.New("x509: version 2 does not support BasicConstraints, KeyUsage, or ExtKeyUsage")
}

within the x509.CreateCertificate() function and prior to the creation of the tbsCertificate struct.

I've tested this on my own branch and found that it does allow for non v3 labeled certificates to be created (providing they do not use fields unsupported by the speficied version). But I wanted to file an issue first and see what people thought, and make sure I'm not way off base here, before submitting my changes for review.

@ianlancetaylor ianlancetaylor changed the title crypto/x509 cannot generate version 1 or 2 certificates crypto/x509: cannot generate version 1 or 2 certificates Aug 24, 2017
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 24, 2017

CC @agl

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 24, 2017
@agl
Copy link
Contributor

@agl agl commented Aug 24, 2017

The Version field is not documented as being read by CreateCertificate, not am I aware of anything that doesn't support v3 certificates. Thus I think the current behaviour is reasonable. If you need to special certificates for testing, it's not unreasonable to have to patch the source for that and/or generate them in a custom way.

@CRThaze
Copy link
Author

@CRThaze CRThaze commented Aug 24, 2017

  1. What harm would there potentially be in adding this functionality? It would seem that supporting more of the full RFCs would be preferable. I'm fully willing to do the leg work, figure it'd be a good place to start contributing.
  2. Is there currently anything that actually uses the Version field, if CreateCertificate is not meant to?
  3. Since it is not part of the current documented behavior should this then be crafted as more of a proposal instead of an bug?
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 25, 2017

  1. With security and x509, usually the opposite test is applied: what benefit is there in adding this functionality?
  2. The same struct is filled by parsing routines
  3. Yes
@CRThaze
Copy link
Author

@CRThaze CRThaze commented Aug 25, 2017

OK cool, that makes sense. Thanks!
Upon consideration, if indeed there's no forseeable need for Version 1 or 2 certs to be generated, I think what may be more valueable here than adding more complexity would be just an update to the docs pointing out that only version 3 cert generation is supported.

Is there a formal process for making those sorts of doc updates? Or is it just a matter of submitting a change updating the function comment?

@agl
Copy link
Contributor

@agl agl commented Aug 29, 2017

It's just a CL changing the comment in the code. Everything flows from there.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 25, 2017

Change https://golang.org/cl/79876 mentions this issue: crypto/x509: document specifically that only v3 certificates are created.

@gopherbot gopherbot closed this in 7da2f82 Nov 25, 2017
@golang golang locked and limited conversation to collaborators Nov 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.