Add cfssl issuer by badie · Pull Request #665 · cert-manager/cert-manager

badie · 2018-06-18T16:45:49Z

What this PR does / why we need it:

It adds support for a cfssl issuer to allow issuing certs from a cfssl based CA.

Release note:

Add support for CFSSL issuers

munnerz · 2018-06-18T17:03:05Z

/ok-to-test

munnerz · 2018-06-21T13:00:43Z

pkg/issuer/cfssl/fakes/lister.go

+)
+
+// Lister is a struct that implements kubernetes api secrets.SecretLister interface
+// It is used solely for testing to mock calls to list secrets


So client-go already has fairly extensive lister testing capabilities. I'd like to see us re-using these instead of creating our own 😄.

You can create a fake clientset using the fake sub-package of the client-go client (or our own clientset in pkg/client, if you need to test with cert-manager types).

Same goes for basically this whole package 😄

Makes sense. I'll reuse that package for the tests and delete this custom pkg

munnerz · 2018-06-21T13:02:34Z

pkg/apis/certmanager/v1alpha1/types.go

+
+type CFSSLCertificateKeyConfig struct {
+	Size int    `json:"size"`
+	Algo string `json:"algo"`


These fields should be removed in favour of generic fields on the Certificate itself that apply to all issuer types (and not just specific to individual issuers).

As we do not currently support all of these fields in the spec (although there is some pre-existing work to add support), we should simply omit them for now and hardcode values (similar to how the other issuers currently do it). I think the majority of the algorithm and size selection is now handled in the pkg/utils/pki package now, anyway 😄

@munnerz Thanks for the recommendation. It makes sense to have generic fields on the certificate thats not specific to any issuer.

I created a PR to add fields to Certificate spec and add helper functions to pki package for generating private keys #722

Once that PR #722 is reviewed, I'll change this PR to use it.

munnerz · 2018-06-21T13:06:21Z

pkg/issuer/cfssl/issue.go

+			CommonName: pki.CommonNameForCertificate(crt),
+		},
+		DNSNames: pki.DNSNamesForCertificate(crt),
+	}


There is a new function in the pki package to handle this now.

munnerz · 2018-06-21T13:07:22Z

pkg/issuer/cfssl/issue.go

+		return nil, fmt.Errorf("error building request body: %s", err)
+	}
+
+	issuerSpec := c.issuer.GetSpec().CFSSL


It should never be the case that this is nil, but worth double checking in case (and just returning an unexpected error or something if it is). Better weird errors than panics 😄

munnerz · 2018-06-21T13:09:38Z

pkg/issuer/cfssl/issue.go

+	return []byte(certificate.(string)), nil
+}
+
+func (c *CFSSL) buildRequestBody(csrPem []byte) ([]byte, error) {


Let's make csrPem type-safe and accept a *x509.CertificateRequest here - we can perform encoding at the time then.

munnerz · 2018-06-21T13:10:38Z

pkg/issuer/cfssl/issue.go

+	authKeySecret := c.issuer.GetSpec().CFSSL.AuthKey
+	if authKeySecret == nil {
+		return requestBytes, nil
+	}


Could you add a comment to clarify why we return early if authKeySecret is nil 😄

munnerz · 2018-06-21T13:11:39Z

pkg/issuer/cfssl/issue.go

+	}
+
+	h := hmac.New(sha256.New, []byte(hexAuthKey))
+	h.Write(requestBytes)


missing err check

munnerz · 2018-06-21T13:12:35Z

pkg/issuer/cfssl/issue_test.go

+			keyAlgo:          ecdsaKeyAlgo,
+			keySize:          ecdsaKeySize,
+			expectedCrt:      certStr,
+			expectedRespBody: fmt.Sprintf(`{"success":true,"result":{"certificate":"%s"}}`, certStr),


this test case says it should fail, but this response includes success? 🤔

munnerz · 2018-06-25T12:58:14Z

pkg/util/pki/generate.go

+
+	return ecdsa.GenerateKey(ecCurve, rand.Reader)
+}
+


The addition of EC keys is great, but can this be extracted out into a separate PR? It will take additional work to support this for all Issuer types.

Cool. I've extracted this work to PR #722

munnerz · 2018-06-25T12:59:22Z

test/e2e/issuer/issuer_cfssl.go

+	f := framework.NewDefaultFramework("create-cfssl-issuer")
+
+	issuerName := "test-cfssl-issuer"
+	serverURL := "http://local.cfssl.server"


How does this work? 🤔

jetstack-bot · 2018-07-20T15:38:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: munnerz

If they are not already assigned, you can assign the PR to them by writing /assign @munnerz in a comment when ready.

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

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-24T03:38:43Z

/ping @munnerz

Please review when you get a chance to. I'd like to get this going and hopefully accepted and merged by the next release 🙂

jetstack-bot · 2018-07-30T13:46:36Z

@badie: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
cert-manager-chart-verify	`163324c`	link	`/test chart-verify`
cert-manager-e2e-v1-8	`163324c`	link	`/test e2e v1.8`
cert-manager-e2e-v1-9	`163324c`	link	`/test e2e v1.9`
cert-manager-e2e-v1-7	`163324c`	link	`/test e2e v1.7`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jetstack-bot · 2018-08-08T19:12:36Z

@badie: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 18, 2018

jetstack-bot requested a review from wallrj June 18, 2018 16:46

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

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

munnerz reviewed Jun 25, 2018

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2018

badie mentioned this pull request Jul 19, 2018

Add keyAlgorithm and keySize fields to Certificates, and support ECDSA keys #722

Merged

badie force-pushed the master branch from 10ecc1b to 71d5a6b Compare July 20, 2018 15:38

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018

badie force-pushed the master branch 4 times, most recently from 4578002 to 8f27756 Compare July 22, 2018 02:22

badie force-pushed the master branch from 8f27756 to c9dab6c Compare July 30, 2018 01:44

Add cfssl issuer

3a8c972

badie force-pushed the master branch 3 times, most recently from 7a33cd8 to b836360 Compare July 30, 2018 03:26

Address PR comments

163324c

badie force-pushed the master branch from b836360 to 163324c Compare July 30, 2018 12:44

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2018

badie closed this Aug 21, 2018

Conversation

badie commented Jun 18, 2018

Uh oh!

munnerz commented Jun 18, 2018

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!

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!

jetstack-bot commented Jul 20, 2018

Uh oh!

badie commented Jul 24, 2018

Uh oh!

jetstack-bot commented Jul 30, 2018

Uh oh!

jetstack-bot commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants