adds option to specify CRL Distribution Point. #2612 by skra-space · Pull Request #2625 · cert-manager/cert-manager

skra-space · 2020-02-25T09:37:17Z

What this PR does / why we need it:
WIP adds option to specify CRL Distribution Point. Proposal

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

Special notes for your reviewer:
Here is only changes for API and self-signed. I also have plan for Vault but didn't implement it.
Release note:

Added 'CRL Distribution Points' fields to Self-signed and CA issuers

jetstack-bot · 2020-02-25T09:37:33Z

Hi @srbraun. 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.

Signed-off-by: srBraun <dev@skra.space>

munnerz · 2020-03-03T09:18:44Z

/ok-to-test

skra-space · 2020-03-10T12:07:18Z

/retest

munnerz · 2020-03-11T14:59:30Z

pkg/apis/certmanager/v1alpha2/types_issuer.go


-type SelfSignedIssuer struct{}
+type SelfSignedIssuer struct {
+	CRLDistributionPoints []string `json:"crlDistributionPoints,omitempty"`


Please add a comment on this field explaining:

What it does

What values are valid

What the default behaviour is if not specified

You should also have a // +optional line (similar to some other fields in this file) as the field is optional 😄

This text is displayed to users through both our docs and kubectl explain so it really helps to provide clearly information about fields and how they should be used 😄

Ah, we can also include // +kubebuilder:validation:UniqueItems here too to avoid users specifying duplicates.. not sure if it's strictly required, but it's easier to relax validation in future than it is to enforce additional validation 😄

pkg/apis/certmanager/v1alpha3/types_issuer.go

munnerz · 2020-03-11T15:06:20Z

This is looking good. Are there any restrictions/requirements around the format for these strings? Do they have to be URLs? If so, we should add validation to check that invalid values are not entered 😄

munnerz · 2020-03-11T15:06:52Z

Also: would it be possible to extend this PR to support the 'CA' issuer type in a similar way? The implementation of these two issuers are very similar, so it shouldn't require too much extra code to do it 😄

skra-space · 2020-03-11T16:46:11Z

Also: would it be possible to extend this PR to support the 'CA' issuer type in a similar way? The implementation of these two issuers are very similar, so it shouldn't require too much extra code to do it smile

Yes, I will add support for CDP to 'CA' issuer. I am currently working on it. I just wanted to show you what I have first. I will work through your comments and prepare a full PR.

Thank you for the review.

Signed-off-by: Sergey Braun <dev@skra.space>

munnerz

Only one more comment... it'd also be good if we could also add a // +kubebuilder:validation:Pattern here to restrict the types of values users can specify. I'm not 100% certain on what a good regex for this would be, as I imagine we'd have to support a variety of different URL formats (e.g. here you can see URLs starting with ldap:/// and containing commas: https://www.sysadmins.lv/blog-en/designing-crl-distribution-points-and-authority-information-access-locations.aspx).

munnerz · 2020-03-19T12:09:57Z

pkg/apis/certmanager/v1alpha2/types_issuer.go


-type SelfSignedIssuer struct{}
+type SelfSignedIssuer struct {
+	CRLDistributionPoints []string `json:"crlDistributionPoints,omitempty"`


Ah, we can also include // +kubebuilder:validation:UniqueItems here too to avoid users specifying duplicates.. not sure if it's strictly required, but it's easier to relax validation in future than it is to enforce additional validation 😄

skra-space · 2020-03-19T14:20:45Z

Only one more comment... it'd also be good if we could also add a // +kubebuilder:validation:Pattern here to restrict the types of values users can specify.

I am not sure how to validate it. According to RFC 5280, CDP name can be directory name or path relative to CRL issuer. In x509 go lib, CDP is an array of strings.

munnerz · 2020-03-20T09:51:14Z

This all looks good, we can look into tightening validation after this merges as I think it may require us to bump our controller-tools dependency 😄

Would you be able to open a PR against the docs repo to note this new option, in both the selfsigned and ca issuer documentation? If you open the PR against the release-next branch so that it is included as part of the 'next' release documentation 😄

https://github.com/cert-manager/website/blob/release-next/content/en/docs/configuration/selfsigned.md && https://github.com/cert-manager/website/blob/release-next/content/en/docs/configuration/ca.md

/lgtm
/approve
/hold
/milestone v0.15
/kind feature

jetstack-bot · 2020-03-20T09:51:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, srbraun

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

munnerz · 2020-03-23T12:07:01Z

/hold cancel

retest-bot · 2020-03-23T13:26:34Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

cedenilla · 2023-04-20T07:40:17Z

@srbraun Is there any possibility that you could add the feature to Vault, as you mentioned a few years ago? Thank you in advance!

skra-space · 2023-04-20T08:05:37Z

Hi! Thank you for reaching out to me. Unfortunately, I am not working on it anymore. I was planning to implement it for my employer but plans have changed there. I hope you'll find a solution. Good luck!

jetstack-bot requested review from JoshVanL and munnerz February 25, 2020 09:37

jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 25, 2020

jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 2, 2020

adds support for CDP to selfsigned issuer

04bfdde

Signed-off-by: srBraun <dev@skra.space>

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 Mar 2, 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 Mar 3, 2020

munnerz reviewed Mar 11, 2020

View reviewed changes

pkg/apis/certmanager/v1alpha3/types_issuer.go Show resolved Hide resolved

munnerz added this to the v0.15 milestone Mar 13, 2020

adds CDP field and description to CA issuer

9b76cdd

Signed-off-by: Sergey Braun <dev@skra.space>

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 Mar 19, 2020

skra-space changed the title ~~WIP adds option to specify CRL Distribution Point. #2612~~ adds option to specify CRL Distribution Point. #2612 Mar 19, 2020

skra-space requested a review from munnerz March 19, 2020 12:01

munnerz reviewed Mar 19, 2020

View reviewed changes

jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 20, 2020

jetstack-bot assigned munnerz Mar 20, 2020

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2020

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2020

skra-space mentioned this pull request Mar 23, 2020

Includes the CRL Distribution Points option cert-manager/website#164

Merged

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2020

jetstack-bot merged commit 35add56 into cert-manager:master Mar 23, 2020

skra-space deleted the crl branch March 23, 2020 14:16

Conversation

skra-space commented Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetstack-bot commented Feb 25, 2020

Uh oh!

munnerz commented Mar 3, 2020

Uh oh!

skra-space commented Mar 10, 2020

Uh oh!

munnerz Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

munnerz Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

munnerz Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

munnerz commented Mar 11, 2020

Uh oh!

munnerz commented Mar 11, 2020

Uh oh!

skra-space commented Mar 11, 2020 • 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!

munnerz Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

skra-space commented Mar 19, 2020

Uh oh!

munnerz commented Mar 20, 2020

Uh oh!

jetstack-bot commented Mar 20, 2020

Uh oh!

munnerz commented Mar 23, 2020

Uh oh!

retest-bot commented Mar 23, 2020

Uh oh!

cedenilla commented Apr 20, 2023

Uh oh!

skra-space commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skra-space commented Feb 25, 2020 •

edited

Loading

skra-space commented Mar 11, 2020 •

edited

Loading