Add keyAlgorithm and keySize fields to Certificates, and support ECDSA keys by badie · Pull Request #722 · cert-manager/cert-manager

badie · 2018-07-09T19:43:23Z

What this PR does / why we need it:

Adds two fields to CertificateSpec:
- keyAlgorithm, denotes which algorithm to use when generating
  a private key. Can be either rsa or ecdsa. When not set, the
  default algorithm used rsa.
- keySize, denotes the key size of the private key being generated.
  For rsa, minimum key size is 2048 and maximum is 8192.
  For ecdsa, sizes 224, 256, 384 & 521 are supported.
  See https://golang.org/pkg/crypto/elliptic
If both fields are not populated, rsa with key size 2048 is used
Adds pki.GeneratePrivateKeyForCertificate function that takes in a Certificate and generates a private key based on the keyAlgorithm & keySize fields in the CertificateSpec
Adds helper functions to the pki package to generate/parse rsa and ecdsa private keys

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Adds new fields: "keyAlgorithm", "keySize" onto CertificateSpec to allow specifying algorithm (rsa, ecdsa) and key size to use when generating TLS keys

munnerz

This looks great - thanks very much for putting it together!

In order to make this universally compatible between Issuers, I think we should try and remove all distinction of RSA vs EC within the pkg/issuers directory, instead opting to refer to crypto.PrivateKey throughout.

As this touches a pretty critical area of the codebase and we are close to v0.4 already (should be released today or tomorrow), I think this will land in the v0.5 release provided we can get all these bits cleared up 😄 (in approx. 1mth!)

munnerz · 2018-07-11T09:15:00Z

pkg/apis/certmanager/v1alpha1/types.go

+	KeySize int `json:"keySize,omitempty"`
+	// KeyAlgorithm is the private key algorithm of the corresponding private key
+	// for this certificate. Allowed values are either "rsa" or "ecdsa"
+	KeyAlgorithm string `json:"keyAlgorithm,omitempty"`


Can we create an 'enum' for these different possible values? It'd be worth also detailing as part of those that additional types may be added in future (so that anyone building API clients can handle unknown values properly).

ie.

type KeyAlgorithm string const ( RSAKeyAlgorithm KeyAlgorithm = "rsa" ECDSAKeyAlgorithm KeyAlgorithm = "ecdsa" )

munnerz · 2018-07-11T09:17:04Z

pkg/apis/certmanager/validation/certificate.go

+	switch strings.ToLower(crt.KeyAlgorithm) {
+	case "":
+		if crt.KeySize != 0 {
+			el = append(el, field.Invalid(fldPath.Child("keySize"), crt.KeySize, "should not be set if keyAlgorithm is not set"))


Why don't we allow specifying keySize without keyAlgorithm?

In future (i.e. once we add a proper Validating/MutatingWebhookConfiguration), we will apply defaulting to the keySize and keyAlgorithm fields which should allow us to handle this case better. But in the meantime it seems like we could allow a user to specify just keySize, and then we'll internally default the algorithm to RSA still (i.e. still validate that the specified size is a valid RSA keySize)

@munnerz It's mostly to reduce confusion and make it explicit that if you do not want the defaults, you'll need to specify both fields (keySize, keyAlgorithm).

I think a fair few people may just want to specify keySize without being explicit about the algorithm though. Despite us adding both of these fields in a single PR, they are not really linked (aside from how validation is performed).

I think we should allow setting just one or the other here (i.e. 'teach' validation about the defaults).

We can then handle proper defaulting with the mutating webhook for the next release, which should clean this bit of code up further 😄

munnerz · 2018-07-11T09:18:03Z

pkg/apis/certmanager/validation/certificate.go

+	default:
+		el = append(el, field.Invalid(fldPath.Child("keyAlgorithm"), crt.KeyAlgorithm, "must be either empty or one of rsa or ecdsa"))
+	}
+


Can we also add a generic check for keySize being >0? (I know this case is already handled, but having it explicitly would be great 😄)

munnerz · 2018-07-11T09:20:46Z

pkg/issuer/ca/issue.go

 	}

-	signerKey, err := kube.SecretTLSKey(c.secretsLister, c.issuerResourcesNamespace, c.issuer.GetSpec().CA.SecretName)
+	signerKey, err := kube.SecretRSAKey(c.secretsLister, c.issuerResourcesNamespace, c.issuer.GetSpec().CA.SecretName)


This key may not be a RSA key - we need to somehow handle this case. Can we use the generic crypto.PrivateKey (https://golang.org/pkg/crypto/#PrivateKey) throughout the codebase in order to handle this case better? It is more relevant now since #658 will allow users to begin building their own PKIs from scratch using cert-manager.

Makes sense. I did that to ensure that stays as RSA. In hindsight, it was to not change too much of the codebase
I'll update the codebase to use crypto.PrivateKey throughout

munnerz · 2018-07-11T09:23:12Z

pkg/issuer/vault/issue.go

+	signeeKey, err := kube.SecretRSAKey(v.secretsLister, crt.Namespace, crt.Spec.SecretName)
 	if k8sErrors.IsNotFound(err) || errors.IsInvalidData(err) {
-		signeeKey, err = pki.GenerateRSAPrivateKey(keyBitSize)
+		signeeKey, err = pki.GenerateRSAPrivateKey(pki.MinRSAKeySize)


Shouldn't this be using the new GeneratePrivateKeyForCertificate function?

munnerz · 2018-07-11T16:17:19Z

/ok-to-test

badie · 2018-07-12T03:59:26Z

@munnerz Thanks for the PR review. I've made the necessary changes.

munnerz · 2018-07-12T08:53:49Z

The chart-verify job (which is poorly named!) is currently failing as you need to re-generate the reference documentation due to changing the API types.

Can you try running hack/update-reference-docs-dockerized.sh on your local machine, and then committing those changes too? That should make it all go green 😄

badie · 2018-07-12T11:17:25Z

My bad, sorry I missed that 😐. I've run the script and pushed the doc updates

munnerz · 2018-07-12T14:44:33Z

Just re-surfacing this comment as github is hiding it from us!

I think a fair few people may just want to specify keySize without being explicit about the algorithm though. Despite us adding both of these fields in a single PR, they are not really linked (aside from how validation is performed).

I think we should allow setting just one or the other here (i.e. 'teach' validation about the defaults).

We can then handle proper defaulting with the mutating webhook for the next release, which should clean this bit of code up further 😄

badie · 2018-07-12T16:59:05Z

Here's my understanding of your comments to ensure we're on the same page:

allow setting keySize without being explicit about keyAlgorithm
- meaning, if keyAlgorithm is not provided, default to rsa and use the specified keySize
I think we should allow setting just one or the other here (i.e. 'teach' validation about the defaults).
- meaning, if keyAlgorithm is specified and keySize is not, apply sane defaults such as 256 bit key size for ecdsa & 2048 for rsa
also update docs accordingly to convey this message

Are we on the same page 💯 😊 ?

badie · 2018-07-16T18:28:07Z

@munnerz I've made the necessary changes

badie · 2018-07-16T19:14:50Z

/retest

munnerz

This is looking great 😄

Just a few more comments, but nothing major.

I need to actually pull and build a copy of this next, before merging as it changes a few important parts.

Thanks for all the work 🎉 looking forward to getting this in the next release.

munnerz · 2018-07-16T19:20:52Z

pkg/apis/certmanager/validation/certificate_test.go

+					KeySize:    2048,
+				},
+			},
+		},


Can we add a few more valid success cases? (e.g. for ecdsa with a valid size, explicit rsa with a valid size)

munnerz · 2018-07-16T19:21:11Z

pkg/issuer/acme/issue.go

+	key, err := kube.SecretRSAKey(a.secretsLister, crt.Namespace, crt.Spec.SecretName)
 	if k8sErrors.IsNotFound(err) || errors.IsInvalidData(err) {
-		key, err = pki.GenerateRSAPrivateKey(2048)
+		key, err = pki.GenerateRSAPrivateKey(pki.MinRSAKeySize)


Should this be GeneratePrivateKeyForCertificate?

& a change to use PublicKeyForPrivateKey and EncodePrivateKey

I actually left that there specifically as acme is rsa only at the moment. You're right though, it does make sense to use GeneratePrivateKeyForCertificate in that block so when acme protocol supports ecdsa, there'll be no change needed.

munnerz · 2018-07-16T19:23:15Z

pkg/util/kube/pki.go

+	return rsaPrivateKey, nil
+}
+
+func SecretRSAKey(secretLister corelisters.SecretLister, namespace, name string) (*rsa.PrivateKey, error) {


nit: comment on exported function

munnerz · 2018-07-16T19:26:32Z

pkg/util/pki/generate.go

+	224: elliptic.P224(),
+	256: elliptic.P256(),
+	384: elliptic.P384(),
+	521: elliptic.P521(),


Is it okay to reuse these (from a security perspective)
Also are they thread-safe? We run 5 Certificate workers, so there's a these could be used in parallel.

Is it okay to reuse these (from a security perspective)

From the go source https://golang.org/src/crypto/elliptic/elliptic.go?s=11343:11360#L367, this is calculated once and cached. Just to ensure I didn't miss anything, I've changed it from using a map to calculating the curve when needed.

Also are they thread-safe? We run 5 Certificate workers, so there's a these could be used in parallel.

Afaik, in go, its ok to have concurrent reads on a map with no writer. I've removed the map implementation so this shouldn't be a problem anymore

munnerz · 2018-07-16T19:29:33Z

pkg/util/pki/parse.go

+			return nil, errors.NewInvalidData("error parsing rsa private key: %s", err.Error())
+		}
+
+		err = key.Validate()


Is there a key.Validate equivalent for ecdsa? I see https://golang.org/pkg/crypto/ecdsa/#Verify, but not sure if that is for verifying a certificate against a public key?

Unfortunately, there's no equivalent for ecdsa.
You're right as thats for verifying a certificate against the public key

munnerz · 2018-07-16T19:36:19Z

Also don't worry too much about individual e2e job failures. There are a few known flakes in our e2e infrastructure that I am (gradually) working on fixing. The retest bot will take care of automatically retesting when the lgtm and approved label are applied 😄

- This PR adds two fields to CertificateSpec: - `keyAlgorithm`, denotes which algorithm to use when generating a private key. Can be either `rsa` or `ecdsa`. When not set, the default algorithm used `rsa`. - `keySize`, denotes the key size of the private key being generated. For `rsa`, minimum key size is 2048 and maximum is 8192. For `ecdsa`, sizes 224, 256, 384 & 521 are supported. See https://golang.org/pkg/crypto/elliptic - `keySize` can be set without being explicit about `keyAlgorithm`. - If `keySize` is specified and `keyAlgorithm` is not provided, `rsa` will be used as the key algorithm. - `keyAlgorithm` can be set without being explicit about `keySize`. - If `keyAlgorithm` is specified and `keySize` is not provided, key size key size of `256` will be used for `ecdsa` key algorithm and key size of `2048` will be used for `rsa` key algorithm. - helper functions in `pki` package now return crypto.PrivateKey

munnerz · 2018-07-18T08:58:29Z

Thanks very much for all the hard work here! 🎉

Let's get this into master so we can get feedback from end-users 😄I've opened #735 to follow up some of the pieces around validation (i.e. ensuring only RSA Certificates are processed for ACME Issuers).

/lgtm
/approve

jetstack-bot · 2018-07-18T08:58:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

badie · 2018-07-19T18:40:58Z

Thanks @munnerz
I'll update PR #665 to use the new features

jetstack-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 9, 2018

jetstack-bot requested review from kragniz and vdesjardins July 9, 2018 19:43

jetstack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 9, 2018

badie mentioned this pull request Jul 9, 2018

Add cfssl issuer #665

Closed

badie force-pushed the support-ec-keys branch from a2b7441 to 23deb7b Compare July 9, 2018 23:57

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 10, 2018

munnerz reviewed Jul 11, 2018

View reviewed changes

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2018

badie force-pushed the support-ec-keys branch from ee215aa to 7bb6f90 Compare July 16, 2018 18:23

munnerz reviewed Jul 16, 2018

View reviewed changes

munnerz changed the title ~~Add support for EC keys~~ Add keyAlgorithm and keySize fields to Certificates, and support ECDSA keys Jul 16, 2018

badie force-pushed the support-ec-keys branch from 7bb6f90 to 0797898 Compare July 17, 2018 16:40

badie force-pushed the support-ec-keys branch from 0797898 to 445e522 Compare July 17, 2018 16:42

munnerz mentioned this pull request Jul 18, 2018

'Runtime' validation support #735

Closed

jetstack-bot assigned munnerz Jul 18, 2018

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2018

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2018

jetstack-bot merged commit 6348c6f into cert-manager:master Jul 18, 2018

munnerz mentioned this pull request Jul 19, 2018

Feature/configurable keysize #634

Closed

avoidik mentioned this pull request Aug 6, 2018

Vault: RSA keyBitSize hardcoded to 2048 #636

Closed

Conversation

badie commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

munnerz commented Jul 11, 2018

Uh oh!

badie commented Jul 12, 2018

Uh oh!

munnerz commented Jul 12, 2018

Uh oh!

badie commented Jul 12, 2018

Uh oh!

munnerz commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

badie commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

badie commented Jul 16, 2018

Uh oh!

badie commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

munnerz commented Jul 16, 2018

Uh oh!

munnerz commented Jul 18, 2018

Uh oh!

jetstack-bot commented Jul 18, 2018

Uh oh!

badie commented Jul 19, 2018

Uh oh!

Reviewers

badie commented Jul 9, 2018 •

edited

Loading

munnerz commented Jul 12, 2018 •

edited

Loading

badie commented Jul 12, 2018 •

edited

Loading

badie commented Jul 16, 2018 •

edited

Loading