Skip to content

venafi: service-generated CSRs must not be locked#767

Merged
jetstack-bot merged 2 commits intocert-manager:masterfrom
maelvls:say-more-about-service-generated-csr
Dec 3, 2021
Merged

venafi: service-generated CSRs must not be locked#767
jetstack-bot merged 2 commits intocert-manager:masterfrom
maelvls:say-more-about-service-generated-csr

Conversation

@maelvls
Copy link
Member

@maelvls maelvls commented Dec 2, 2021

It seems our documentation isn't precise enough and does not mention the possibility for "Service Generated CSRs" to be a valid configuration as long as it is not locked; for example the following works with cert-manager:

Screenshot 2021-12-02 at 16 34 13

Signed-off-by: Maël Valais mael@vls.dev

cc @SpectralHiss

Signed-off-by: Maël Valais <mael@vls.dev>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 2, 2021
@netlify
Copy link

netlify bot commented Dec 2, 2021

✔️ Deploy Preview for cert-manager-website ready!

🔨 Explore the source changes: 3745695

🔍 Inspect the deploy log: https://app.netlify.com/sites/cert-manager-website/deploys/61a9e2172df7d30007227621

😎 Browse the preview: https://deploy-preview-767--cert-manager-website.netlify.app/docs/configuration/venafi

@SpectralHiss
Copy link
Contributor

if we just add that cert-manager can work with the policy set to server provided csr without it being locked it would be good.
In that case vcert (and by extension, cert-manager) will set organisation fields etc. will be set to the policy value.

I don't think the CSR itself is modified but this is a leaky abstraction by TPP, it's just that in that mode the certificate will have different properties derived from the policy than what the CSR requested.

@jetstack-bot jetstack-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 3, 2021
@maelvls
Copy link
Member Author

maelvls commented Dec 3, 2021

@SpectralHiss Got it. I added an explanation of what will happen if you use "Service Generated CSR" unlocked. What do you think?

Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls maelvls force-pushed the say-more-about-service-generated-csr branch from 9c8c529 to 3745695 Compare December 3, 2021 09:23
@SpectralHiss
Copy link
Contributor

SpectralHiss commented Dec 3, 2021

this is a good addition to the Venafi issuer docs 👍

@SpectralHiss
Copy link
Contributor

/lgtm

@jetstack-bot
Copy link
Contributor

@SpectralHiss: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

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.

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both of you for figuring this out.

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2021
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls, 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:

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

@jetstack-bot jetstack-bot merged commit d26c2a0 into cert-manager:master Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants