Add option to specify OCSP server #3497 by hugoboos · Pull Request #3505 · cert-manager/cert-manager

hugoboos · 2020-12-10T14:05:43Z

What this PR does / why we need it:
Adds option to specify OCSP server on the CA issuer.

Which issue this PR fixes *:
fixes #3497

Special notes for your reviewer:

Release note:

Added the option to specify the OCSP server for certificates issued by the CA issuer

jetstack-bot · 2020-12-10T14:05:59Z

Hi @hugoboos. Thanks for your PR.

I'm waiting for a jetstack or cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

hugoboos · 2020-12-10T16:38:01Z

/assign @jakexks

jakexks · 2020-12-12T02:38:05Z

/ok-to-test

hugoboos · 2020-12-15T09:46:12Z

/retest

wallrj

Thanks @hugoboos

You'll need to rebase and regenerate and update the copyright headers because we just merged a change to the boilerplate header.

If the OCSP value must be a URL, then we could add some API validation to avoid users setting a bad value here. See https://github.com/jetstack/cert-manager/blob/11353f2936c42f3b2bc91d62be969bf7efa9f50e/pkg/internal/apis/certmanager/validation/issuer_test.go

pkg/internal/apis/certmanager/v1alpha2/conversion.go

maelvls · 2021-02-02T09:17:00Z

Not sure what's the status of this PR, can we have it merged for v1.2 or should we bump it to v1.3?

/assign @JoshVanL

munnerz · 2021-02-03T09:37:48Z

pkg/apis/certmanager/v1/types_issuer.go

+	// The OCSP responder can be queried for the revocation status of an issued certificate.
+	// If not set the certificate wil be issued without the OCSP server set.
+	// +optional
+	OCSPServer []string `json:"ocspServer,omitempty"`


This name is singular, but it's a slice? Should this be ocspServers?

Aaah I know why it was using a singular name: the standard library's x509.go uses a singular name!

// RFC 5280, 4.2.2.1 (Authority Information Access) OCSPServer []string IssuingCertificateURL []string

The RFC 5280 describe the authority information access field (= analogous to our ocspServer array) as (ASN.1-like syntax):

id-pe-authorityInfoAccess OBJECT IDENTIFIER ::= { id-pe 1 } AuthorityInfoAccessSyntax ::= SEQUENCE SIZE (1..MAX) OF AccessDescription AccessDescription ::= SEQUENCE { accessMethod OBJECT IDENTIFIER, accessLocation GeneralName } id-ad OBJECT IDENTIFIER ::= { id-pkix 48 } id-ad-caIssuers OBJECT IDENTIFIER ::= { id-ad 2 } id-ad-ocsp OBJECT IDENTIFIER ::= { id-ad 1 }

→ I renamed OCSPServer to OCSPServers, is that still OK?

Yes, i have reused the naming from x509.go and the certificate.

We did this previously (reused the names from x509 for some fields) and eventually moved away from it (e.g. 'organization -> organizations').

I think OCSPServers makes sense to me! 😄

munnerz · 2021-02-03T09:39:53Z

Can we add a test case for this to ensure that it's being properly plumbed through?

maelvls · 2021-02-03T09:55:22Z

ensure that it's being properly plumbed

@munnerz Where do you think I could add a test? I'm not sure where 😅

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: James Munnelly <james@munnelly.eu>

maelvls · 2021-02-03T10:26:13Z

/retest

maelvls · 2021-02-03T12:35:05Z

/retest

Those pesky Venafi Cloud tests are still failing 😞

munnerz · 2021-02-03T12:46:22Z

@munnerz Where do you think I could add a test? I'm not sure where 😅

I think a unit test in the CA issuer code that ensures that given an issuer that configures ocspServers, a resulting certificate will contain those OCSP servers would suffice.

I'd expect to see this in pkg/issuer/ca: https://github.com/jetstack/cert-manager/tree/master/pkg/issuer/ca - we are missing explicit tests in here right now, so having this as the first example will pave the way for us to write tests for the existing CA-issuer specified fields (I think there's only one other, CRLDistributionPoints, and that was added recently)

maelvls · 2021-02-03T15:04:48Z

pkg/issuer/ca/setup_test.go

+			// TODO: How do I c.Issue()? The ca.CA struct only implements
+			// the Setup function, not the Issue function.


A unit test in the CA issuer code that ensures that given an issuer that configures ocspServers , a resulting certificate will contain those OCSP servers would suffice.

@munnerz How can I test that ca.CA can issue a certificate? ca.CA does not seem to implement Issue() :(

I tried writing a new unit test in pkg/controller/certificaterequests/ca but I had no way to check that the oscpServers has been properly set in the resulting certificate.

I started to write a new unit test that does check the created certificate; you can see it on another branch (take a look at ca_test.go).

I have not finished and I really wish we could add this ocspServers feature to v1.2-alpha.2 we are going to release this morning

@munnerz What do you think? I will create a new issue "add unit test for ocspServers and crlDistributionPoints" and point to the existing branch I started

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: James Munnelly <james@munnelly.eu>

The reason I had to withdraw from writing this unit test is that the ca.CA type in pkg/issuer/ca does not implement the Sign function, which means I cannot test the Sign feature. I then tried to implement the same unit test to pkg/controller/certificaterequests/ca, but the existing unit test do not check the fields inside the produced certificate, which means I cannot ensure that the ocspServers fields has properly been applied to the certificate. I will write a proper unit test... a bit later. Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: James Munnelly <james@munnelly.eu>

maelvls · 2021-02-05T10:55:53Z

~~After a discussion during the 1.2-alpha.2 release meeting, we will skip the 1.2-alpha.2 in order to get the PR properly tested 👍~~ I will personally make sure to get this unit tested! Tracked here: #3636

wallrj · 2021-02-05T11:00:20Z

/lgtm

wallrj

/lgtm

jakexks · 2021-02-05T11:05:34Z

/kind feature
/approve

jetstack-bot · 2021-02-05T11:05:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hugoboos, jakexks, wallrj

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~~ [jakexks,wallrj]

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

maelvls · 2021-02-05T12:32:30Z

Another task that I forgot to do as part of this PR: document the ocspServers feature on the website! The crlDistributionPoint feature is already in cert-manager/website#164, we should do the same

Still to be done:

Document ocspServers website#425 to document the new ocspServers field,
Add a unit test around the new ocspServers and crlDistributionPoints #3636 to unit test crlDistributionPoint and ocspServers.

jetstack-bot requested a review from wallrj December 10, 2020 14:06

jetstack-bot added area/api Indicates a PR directly modifies the 'pkg/apis' directory area/deploy Indicates a PR modifies deployment configuration labels Dec 10, 2020

hugoboos force-pushed the ocsp-server branch from 03a79c9 to ecb19a1 Compare December 10, 2020 14:07

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Dec 10, 2020

hugoboos mentioned this pull request Dec 10, 2020

Option to specify OCSP responder URL #3497

Closed

jetstack-bot assigned jakexks Dec 10, 2020

jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 12, 2020

hugoboos force-pushed the ocsp-server branch from ecb19a1 to d3f3ae0 Compare December 15, 2020 09:44

hugoboos force-pushed the ocsp-server branch from d3f3ae0 to 11353f2 Compare December 15, 2020 13:04

wallrj requested changes Dec 15, 2020

View reviewed changes

pkg/internal/apis/certmanager/v1alpha2/conversion.go Outdated Show resolved Hide resolved

hugoboos force-pushed the ocsp-server branch from 11353f2 to 334266c Compare December 15, 2020 15:54

meyskens added this to the v1.2 milestone Dec 22, 2020

hugoboos force-pushed the ocsp-server branch from 334266c to e3fc1a3 Compare January 11, 2021 14:04

hugoboos requested review from meyskens and wallrj January 11, 2021 14:36

JoshVanL added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 28, 2021

jetstack-bot assigned JoshVanL Feb 2, 2021

munnerz reviewed Feb 3, 2021

View reviewed changes

maelvls force-pushed the ocsp-server branch from e3fc1a3 to 59c6dfb Compare February 3, 2021 09:58

Rename ocspServer to oscpServers

ba22785

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: James Munnelly <james@munnelly.eu>

maelvls force-pushed the ocsp-server branch from 59c6dfb to ba22785 Compare February 3, 2021 10:13

jetstack-bot added the area/ca Indicates a PR directly modifies the CA Issuer code label Feb 3, 2021

maelvls reviewed Feb 3, 2021

View reviewed changes

maelvls and others added 2 commits February 5, 2021 10:20

ca issuer: add a unit test with the ocsp servers field

11b9d71

Signed-off-by: Maël Valais <mael@vls.dev> Co-authored-by: James Munnelly <james@munnelly.eu>

maelvls force-pushed the ocsp-server branch from 8fd29cf to 95f8410 Compare February 5, 2021 09:24

jetstack-bot assigned wallrj Feb 5, 2021

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2021

maelvls mentioned this pull request Feb 5, 2021

Add a unit test around the new ocspServers and crlDistributionPoints #3636

Closed

2 tasks

wallrj approved these changes Feb 5, 2021

View reviewed changes

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2021

jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Feb 5, 2021

jetstack-bot merged commit 35febb1 into cert-manager:master Feb 5, 2021

maelvls mentioned this pull request Feb 5, 2021

Document ocspServers cert-manager/website#425

Closed

maelvls mentioned this pull request Feb 11, 2021

Prepare backport of bug fixes for releasing 1.1.1 #3656

Closed

37 tasks

maelvls mentioned this pull request Mar 6, 2022

Retry on conflict for the end-to-end test "CA Injector for api services should update data when the certificate changes" #4925

Merged

		// TODO: How do I c.Issue()? The ca.CA struct only implements
		// the Setup function, not the Issue function.

Conversation

hugoboos commented Dec 10, 2020

Uh oh!

jetstack-bot commented Dec 10, 2020

Uh oh!

hugoboos commented Dec 10, 2020

Uh oh!

jakexks commented Dec 12, 2020

Uh oh!

hugoboos commented Dec 15, 2020

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maelvls commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

maelvls Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugoboos Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

munnerz Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

munnerz commented Feb 3, 2021

Uh oh!

maelvls commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls commented Feb 3, 2021

Uh oh!

maelvls commented Feb 3, 2021

Uh oh!

munnerz commented Feb 3, 2021

Uh oh!

maelvls Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

maelvls Feb 5, 2021

Choose a reason for hiding this comment

Uh oh!

maelvls commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wallrj commented Feb 5, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

jakexks commented Feb 5, 2021

Uh oh!

jetstack-bot commented Feb 5, 2021

Uh oh!

maelvls commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maelvls commented Feb 2, 2021 •

edited

Loading

maelvls Feb 3, 2021 •

edited

Loading

maelvls commented Feb 3, 2021 •

edited

Loading

maelvls commented Feb 5, 2021 •

edited

Loading

maelvls commented Feb 5, 2021 •

edited

Loading