Add PKCS#11 certificate generation tool #3729

rolandshoemaker · 2018-05-21T18:51:34Z

Tested against relevant hardware for generating both RSA and ECDSA roots and intermediates with keys generated using gen-key.

Also this makes a few changes to the gen-key tool after further experience with the HSM and more reading of the PCKS#11 specification. Main change is the removal of compatMode, which was intended to provide support for two naming schemes for EC used in subsequent PKCS#11 drafts. It turns out these schemes were changes in name only and the underlying structs/ints were the exact same (i.e. CKA_ECDSA_PARAMS == CKA_EC_PARAMS and CKM_ECDSA_KEY_PAIR_GEN == CKM_EC_KEY_PAIR_GEN) and just allowed using one of the two names based on preference. This meant with compatMode enabled or disabled the tool did the exact same thing.

Fixes #3697.

cpu

I think cmd/gen-ca/main.go would benefit from some godoc comments on the types & functions defined within.

Overall this branch looks pretty good to me. I'd like to do another 🔍 after I've refreshed my memory on some of the PKCS11 details in use here. I flagged a few small nits from a first pass.

cpu · 2018-05-23T20:29:11Z

.gitignore

@@ -36,3 +36,4 @@ tags
 # IDE support files
 .idea

+.vscode/*


😲 Are you using vscode for Boulder & friends regularly?

...yes, visual code studio is great ❤️.

cpu · 2018-05-23T20:32:42Z

cmd/gen-ca/main.go

+		return 0, err
+	}
+	if len(handles) == 0 {
+		return 0, errors.New("no objects found matching label and ID")


Would it be clearer if this said "matching provided template"?

cpu · 2018-05-23T20:36:07Z

cmd/gen-ca/main.go

+}
+
+const dateLayout = "2006-01-02 15:04:05"
+const serialLength = 16


I think this const serialLength and the const dateLayout above could be put inside of the constructCert function scope instead of being global.

cpu · 2018-05-23T20:37:24Z

cmd/gen-ca/main.go

+}
+
+func main() {
+	module := flag.String("module", "", "PKCS#11 module to use")


Is there a default for this that we can set for the Linux distro we expect to use this on?

The PKCS#11 module you use is dependent on the hardware/software you are communicating with. I think putting any defaults here would leak more information about things we use than we are probably comfortable with.

cpu · 2018-05-23T20:40:01Z

cmd/gen-ca/main.go

+	slot := flag.Uint("slot", 0, "Slot signing key is in")
+	pin := flag.String("pin", "", "PIN for slot if not using PED to login")
+	label := flag.String("label", "", "Signing key label")
+	id := flag.String("id", "", "Signing key ID hex")


Is there an expected format for the hex input? 0xAAAAAA vs AAAAAA?

This led me down a rabbit hole of trying to figure out if there are canonical names/specs for the different textual encoding formats for hexadecimal. Apparently there aren't really any.

cpu · 2018-05-23T20:40:31Z

cmd/gen-ca/main.go

+	id := flag.String("id", "", "Signing key ID hex")
+	profilePath := flag.String("profile", "", "Path to certificate profile")
+	pubKeyPath := flag.String("publicKey", "", "Path to public key for certificate") // this could also be in the profile
+	issuerPath := flag.String("issuer", "", "Path to issuer cert if generating a intermediate")


typo: "a intermediate" -> "an intermediate"

cpu · 2018-05-23T20:43:18Z

cmd/gen-ca/main.go

+	if err := cert.CheckSignatureFrom(issuer); err != nil {
+		log.Fatalf("Failed to verify certificate signature: %s", err)
+	}
+	log.Println("Verifed certificate signature")


typo: "Verifed" -> "Verified"

cpu · 2018-05-23T20:46:47Z

cmd/gen-ca/main.go

+	"github.com/letsencrypt/boulder/pkcs11helpers"
+)
+
+type x509Signer struct {


Can you add some Godoc comments for this type & its functions?

cpu · 2018-05-23T20:47:08Z

cmd/gen-ca/main.go

+	return p.pub
+}
+
+func findObject(ctx pkcs11helpers.PKCtx, session pkcs11.SessionHandle, tmpl []*pkcs11.Attribute) (pkcs11.ObjectHandle, error) {


I think this function deserves a short comment describing its use/purpose.

jsha · 2018-05-25T21:33:05Z

cmd/gen-ca/main.go

+	return handles[0], nil
+}
+
+// getKey retrieves the private key handle and constructs the public key defined


Can you rewrite this comment? It doesn't really make sense to me. I think something like "constructs an x509Signer based on the PKCS#11 private key object with the provided label and ID."

Also, why do we take both label and ID? It seems like it would make more sense to just take label, and find the ID based on that label.

This is mainly a spec thing, CKA_LABEL is not meant to be unique and CKA_ID is intended to differentiate objects in that case (as well as to identify key pairs).

jsha · 2018-05-25T21:39:27Z

cmd/gen-ca/main.go

+
+// constructCert generates the certificate template for use in x509.CreateCertificate
+func constructCert(ctx pkcs11helpers.PKCtx, profile *certProfile, pubKey []byte, session pkcs11.SessionHandle) (*x509.Certificate, error) {
+	dateLayout := "2006-01-02 15:04:05"


Any reason not to use time.RFC3339 for this? It's a bit annoying since it requires us to write out seconds and time zone, but probably better to be explicit and formal than to not be. Also, the format of these date fields should be documented somewhere. Maybe on the certProfile struct, and make that an exported type so it becomes part of the godoc?

I think using RFC3339 is a bit of a footgun given it allows input of timezone, which will always need to be ignored/normalized to UTC to conform with 5280. We could use a truncated form of 3339 that just omits the timezone, but at that point we're just adding a T between the date and time for no real reason.

Given any format will need to be documented somewhere for people to know how to use it anyway I think going with this is simplest solution (and easier for humans to read in general).

Alright, works for me so long as it is well documented.

jsha · 2018-05-25T21:45:46Z

cmd/gen-ca/main.go

+	return oid, nil
+}
+
+// constructCert generates the certificate template for use in x509.CreateCertificate


This should probably called makeTemplate or something since it doesn't actually do the signing.

jsha · 2018-05-25T21:47:01Z

cmd/gen-ca/main.go

+		SignatureAlgorithm:    sigAlg,
+		SerialNumber:          big.NewInt(0).SetBytes(serial),
+		BasicConstraintsValid: true,
+		IsCA: true,


The indentation here is weird. It doesn't line up with the line above. How does this past our gofmt test?

golang/go#7819

Wow, that is weird. /shrug

jsha · 2018-05-25T21:49:06Z

cmd/gen-ca/main.go

+		NotAfter:              notAfter,
+		OCSPServer:            []string{profile.OCSPURL},
+		CRLDistributionPoints: []string{profile.CRLURL},
+		IssuingCertificateURL: []string{profile.IssuerURL},


I think IssuerURL will be absent for a self-signed root, right? So we should specially handle the case where it's empty.

Relatedly, for each value we interpolate here we should ensure it's non-empty / non-default.

jsha · 2018-05-25T22:12:20Z

cmd/gen-ca/main.go

+		log.Fatalf("Failed to parse public key: %s", err)
+	}
+
+	certTmpl, err := constructCert(ctx, &profile, pubPEM.Bytes, session)


*certTemplate

jsha · 2018-05-25T22:13:39Z

cmd/gen-key/ecdsa.go

-// mechanism and attribute types should be used, for devices that implement
-// a pre-2.11 version of the PKCS#11 specification compatMode should be true.
-func ecArgs(label string, curve *elliptic.CurveParams, compatMode bool, keyID []byte) generateArgs {
+// type of key should be generated.


Can you add to the PR description explaining why compatMode is removed?

jsha · 2018-05-25T22:14:24Z

cmd/gen-key/ecdsa.go

-		pkcs11.NewAttribute(pkcs11.CKA_EC_POINT, nil),
-	})
+// the correct curve type.
+func ecPub(ctx pkcs11helpers.PKCtx, session pkcs11.SessionHandle, object pkcs11.ObjectHandle, expectedCurve *elliptic.CurveParams) (*ecdsa.PublicKey, error) {


As long as you're touching this line, let's split it into one line per argument.

jsha · 2018-05-25T22:21:07Z

cmd/gen-key/ecdsa.go

 	}
+	log.Printf("\tX: %X\n", pubKey.X.Bytes())


Did you mean to keep this log line?

jsha · 2018-05-25T22:33:26Z

pkcs11helpers/helpers.go

+	"github.com/miekg/pkcs11"
+)
+
+type PKCtx interface {


The exported types in this file need documentation.

Seeing this factored out, it really does feel like it duplicates a lot of pkcs11key. Are you sure you wouldn't rather just use the library? It has the benefit that it's used somewhat more, so people are more likely to find bugs.

I went back and forth about whether to use pkcs11key or not and landed here. I think there are a handful of reasons not to, the largest among them being that it makes testing a bit more painful and and requires either a number of changes to how the ceremony process works or changes to pkcs11key that would only be used in this specific context (which means we're back to it not getting touched by (many) other people in order to find bugs).

I think on the whole I eventually came down on the side of just writing the code here. It allows the gen-ca and gen-key packages to be somewhat more easy to audit, in terms of how they work, and have restricted interfaces in terms of what they can do and what signature formats etc that they support and what kinds of objects they can extract.

cpu

Only a few tiny cosmetic changes to request.

cpu · 2018-06-06T17:21:41Z

cmd/gen-ca/main.go

+
+	// CommonName should contain the requested subject common name
+	CommonName string
+	// CommonName should contain the requested subject organization


s/CommonName/Organization/

cpu · 2018-06-06T17:21:51Z

cmd/gen-ca/main.go

+	CommonName string
+	// CommonName should contain the requested subject organization
+	Organization string
+	// CommonName should contain the requested subject country code


s/CommonName/Country/

cpu · 2018-06-06T17:27:23Z

cmd/gen-ca/main.go

+	// OCSPURL should contain the URL at which a OCSP responder that
+	// can respond to OCSP requests for this certificate operates
+	OCSPURL string
+	// OCSPURL should contain the URL at which CRLs for this certificate


s/OCSPURL/CRLURL/

cpu

🎉

rolandshoemaker added 11 commits May 15, 2018 15:47

Initial work

c54d419

Merge branch 'master' into pkcs11-gen-cert

226a78f

Use better name

bf77215

Fix gen key

f4ddf19

Some cleanups and factoring out duped code

35de145

Improve some comments and various other things

865c6f1

Merge branch 'master' into pkcs11-gen-cert

1827140

Parse issuer as PEM

bfc1a21

Partial tests for gen-ca

a19129f

Almost all of tests done

825851a

Remove debug println

9049c32

rolandshoemaker requested a review from a team as a code owner May 21, 2018 18:51

cpu suggested changes May 23, 2018

View reviewed changes

Review fixes

c670996

jsha requested changes May 25, 2018

View reviewed changes

rolandshoemaker added 3 commits May 30, 2018 12:59

Review fixes

0287422

Document CertProfile

2cf11d3

Typo

bbbb915

cpu suggested changes Jun 6, 2018

View reviewed changes

Fix doc comments

b491007

jsha approved these changes Jun 12, 2018

View reviewed changes

cpu approved these changes Jun 12, 2018

View reviewed changes

rolandshoemaker merged commit 6fe950b into master Jun 12, 2018

cpu deleted the pkcs11-gen-cert branch June 12, 2018 19:15

Add PKCS#11 certificate generation tool #3729

Add PKCS#11 certificate generation tool #3729

Conversation

rolandshoemaker commented May 21, 2018 • edited Loading

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolandshoemaker May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu left a comment

Choose a reason for hiding this comment

rolandshoemaker commented May 21, 2018 •

edited

Loading

rolandshoemaker May 29, 2018 •

edited

Loading