Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add/update docs for new trust-manager features #1351

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Add/update docs for new trust-manager features #1351

merged 1 commit into from
Nov 30, 2023

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Nov 26, 2023 •
edited
Loading

I noticed that trust-manager docs are not updated for new features available since trust-manager v0.7.0:

  • PKCS#12 as additional target format
  • Secret targets

It also seems like the trust-manager API docs are outdated. Do we have CI to update it, or is it done manually? If we don't have CI, it would probably make sense to include an update in this PR? Update: API docs for trust-manager is now updated to v0.7.0.

\cc @inteon @SgtCoDFish

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 26, 2023
@netlify Netlify
Copy link

netlify bot commented Nov 26, 2023 •
edited
Loading

✅ Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit e564f83
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/65685e53e249ff0008e988bd
😎 Deploy Preview https://deploy-preview-1351--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@erikgb erikgb force-pushed the new-features-trust-manager branch 2 times, most recently from 9f38d9a to f53b43d Compare November 26, 2023 15:51
inteon
inteon reviewed Nov 26, 2023
View reviewed changes
Copy link
Member

@inteon inteon left a comment

Choose a reason for hiding this comment

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

Thanks @erikgb this is an awesome contribution! The changes look good, there is one paragraph that I will reread and add some comments.

SgtCoDFish
SgtCoDFish requested changes Nov 27, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

A few suggestions on this; what do you think?

content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
@erikgb erikgb force-pushed the new-features-trust-manager branch from 1d832da to 6d4d6d3 Compare November 27, 2023 09:52
@erikgb erikgb force-pushed the new-features-trust-manager branch from 6d4d6d3 to 7a0e065 Compare November 27, 2023 09:56
inteon
inteon reviewed Nov 27, 2023
View reviewed changes
content/docs/trust/trust-manager/README.md Outdated
Comment on lines 109 to 110
secrets. Both JKS and PKCS#12 uses weak encryption primitives, so a trust store (or keystore) will NOT
be protected by a password alone, and needs to be protected by additional measures.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
secrets. Both JKS and PKCS#12 uses weak encryption primitives, so a trust store (or keystore) will NOT
be protected by a password alone, and needs to be protected by additional measures.
secrets. For that reason, these passwords do not provide any security value and don't have to remain secret.

Copy link
Contributor Author

@erikgb erikgb Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if I agree with this suggestion. Users want to use passwords on trust stores to protect the integrity of the trust store. We all can agree that a trust store doesn't contain anything secret, but some think it's important to use a secret password - even in Kubernetes. I am open to rewording here, it's just that I find the suggestion not logic in the context. 😉

Copy link
Member

Choose a reason for hiding this comment

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

This section is so tricky and it's screaming out for me to write that FAQ I'd been talking about on why these passwords aren't useful. I think the PR as-written is not quite correct, but Tim's suggestion isn't the wording I'd go for either. I'll make an alternative suggestion which tries to stay neutral here.

Copy link
Contributor Author

@erikgb erikgb Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Ash! Your suggestion was so good that I included you as a co-author on this PR. ❤️ I hope Tim also thinks it's ok now.

@erikgb erikgb force-pushed the new-features-trust-manager branch from 7a0e065 to 3b2b566 Compare November 27, 2023 20:17
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 27, 2023
@erikgb erikgb requested review from SgtCoDFish and inteon November 30, 2023 08:25
SgtCoDFish
SgtCoDFish reviewed Nov 30, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

Few little bits - what do you think?

content/docs/trust/trust-manager/README.md Outdated
Comment on lines 109 to 110
secrets. Both JKS and PKCS#12 uses weak encryption primitives, so a trust store (or keystore) will NOT
be protected by a password alone, and needs to be protected by additional measures.
Copy link
Member

Choose a reason for hiding this comment

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

This section is so tricky and it's screaming out for me to write that FAQ I'd been talking about on why these passwords aren't useful. I think the PR as-written is not quite correct, but Tim's suggestion isn't the wording I'd go for either. I'll make an alternative suggestion which tries to stay neutral here.

content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
content/docs/trust/trust-manager/README.md Outdated Show resolved Hide resolved
@erikgb erikgb force-pushed the new-features-trust-manager branch from 44fdf54 to a3dac03 Compare November 30, 2023 09:59
@erikgb @SgtCoDFish
Add/update docs for new trust-manager features
e564f83 
Co-authored-by: Ashley Davis <SgtCoDFish@users.noreply.github.com>
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
@erikgb erikgb force-pushed the new-features-trust-manager branch from a3dac03 to e564f83 Compare November 30, 2023 10:05
@erikgb erikgb requested a review from SgtCoDFish November 30, 2023 10:07
SgtCoDFish
SgtCoDFish approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

This is definitely a big improvement. Thanks!

(not sure if this'll go through if you added me as a co-author - we'll see 😁 )

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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

The pull request process is described here

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023
@jetstack-bot jetstack-bot merged commit 2efcce5 into cert-manager:master Nov 30, 2023
1 check passed
@jetstack-bot jetstack-bot mentioned this pull request Dec 7, 2023
Merged
@erikgb erikgb mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SgtCoDFish SgtCoDFish SgtCoDFish approved these changes

@inteon inteon Awaiting requested review from inteon

Assignees
No one assigned
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erikgb @jetstack-bot @SgtCoDFish @inteon