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 pkcs12 password feature into Bundle resource #235

Closed
wants to merge 2 commits into from
Closed

Add pkcs12 password feature into Bundle resource #235

wants to merge 2 commits into from

Conversation

arsenalzp
Copy link
Contributor

It is another PR related to #199. It adds PKCS12 arbitrary password feature into Bundle resource.

@dependabot @arsenalzp
add Bundle arbitrary password
f5e3d62 
Bumps the all group with 3 updates: [github.com/spf13/cobra](https://github.com/spf13/cobra), [k8s.io/klog/v2](https://github.com/kubernetes/klog) and software.sslmate.com/src/go-pkcs12.

Updates `github.com/spf13/cobra` from 1.7.0 to 1.8.0
- [Release notes](https://github.com/spf13/cobra/releases)
- [Commits](spf13/cobra@v1.7.0...v1.8.0)

Updates `k8s.io/klog/v2` from 2.100.1 to 2.110.1
- [Release notes](https://github.com/kubernetes/klog/releases)
- [Changelog](https://github.com/kubernetes/klog/blob/main/RELEASE.md)
- [Commits](kubernetes/klog@v2.100.1...v2.110.1)

Updates `software.sslmate.com/src/go-pkcs12` from 0.3.0 to 0.4.0

---
updated-dependencies:
- dependency-name: github.com/spf13/cobra
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: all
- dependency-name: k8s.io/klog/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: all
- dependency-name: software.sslmate.com/src/go-pkcs12
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: all
...

Signed-off-by: dependabot[bot] <support@github.com>

Allow permissions to put the leases in the trust-manager namespace, not the trust namespace

Signed-off-by: Thomas Spear <tspear@conquestcyber.com>

Give the new role a name consistent with the other cert-manager components

Signed-off-by: Thomas Spear <tspear@conquestcyber.com>

Don't use the trust namespace for leases

Signed-off-by: Thomas Spear <tspear@conquestcyber.com>

Update deploy/charts/trust-manager/templates/role.yaml

Co-authored-by: Erik Godding Boye <egboye@gmail.com>
Signed-off-by: tspearconquest <81998567+tspearconquest@users.noreply.github.com>

add pkcs12 password feature

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

add test for PKCS12 with arbitrary password

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

add PKCS12 arbitrary password tests

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

remove unnecessary labels from ConfigMap resource

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

update helm docs

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix some remarks from the PR

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix JKS tests

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

reflect PKCS12 and JKS encoders modification to integration test

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

update Helm docs

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix typo in the Values part

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

update Helm docs

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

update Helm values and doc files

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

Add pkcs12 password feature in the Bundle resource

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix password issue in helm chart

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix conflicts

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

tweak readme to encourage use

Signed-off-by: Ashley Davis <ashley.davis@venafi.com>

chore: generate applyconfigurations for custom resources

Signed-off-by: Erik Godding Boye <egboye@gmail.com>

Bump the all group with 3 updates

Bumps the all group with 3 updates: [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo), [github.com/onsi/gomega](https://github.com/onsi/gomega) and [sigs.k8s.io/structured-merge-diff/v4](https://github.com/kubernetes-sigs/structured-merge-diff).

Updates `github.com/onsi/ginkgo/v2` from 2.13.0 to 2.13.1
- [Release notes](https://github.com/onsi/ginkgo/releases)
- [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md)
- [Commits](onsi/ginkgo@v2.13.0...v2.13.1)

Updates `github.com/onsi/gomega` from 1.29.0 to 1.30.0
- [Release notes](https://github.com/onsi/gomega/releases)
- [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md)
- [Commits](onsi/gomega@v1.29.0...v1.30.0)

Updates `sigs.k8s.io/structured-merge-diff/v4` from 4.2.3 to 4.4.1
- [Release notes](https://github.com/kubernetes-sigs/structured-merge-diff/releases)
- [Changelog](https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/RELEASE.md)
- [Commits](kubernetes-sigs/structured-merge-diff@v4.2.3...v4.4.1)

---
updated-dependencies:
- dependency-name: github.com/onsi/ginkgo/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: all
- dependency-name: github.com/onsi/gomega
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: all
- dependency-name: sigs.k8s.io/structured-merge-diff/v4
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: all
...

Signed-off-by: dependabot[bot] <support@github.com>

fix conflicts

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix bundle test

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

fix trust.cert-manager.io_bundles file

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

remove old cluster-wide password feature

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sgtcodfish for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 16, 2023
@jetstack-bot
Copy link
Contributor

Hi @arsenalzp. Thanks for your PR.

I'm waiting for a 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.

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 jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 16, 2023
@arsenalzp
Copy link
Contributor Author

All conflicts were fixed, all work was re-based.

erikgb
erikgb requested changes Nov 16, 2023
View reviewed changes
deploy/charts/trust-manager/templates/trust.cert-manager.io_bundles.yaml
properties:
key:
description: Key is the key of the entry in the object's `data` field to be used.
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this section should be added.

pkg/apis/trust/v1alpha1/types_bundle.go
@@ -122,6 +122,8 @@ type AdditionalFormats struct {
// PKCS12 requests a PKCS12-formatted binary trust bundle to be written to the target.
// The bundle is created without a password.
Copy link
Contributor

Choose a reason for hiding this comment

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

Must be updated.

pkg/apis/trust/v1alpha1/types_bundle.go
@@ -122,6 +122,8 @@ type AdditionalFormats struct {
// PKCS12 requests a PKCS12-formatted binary trust bundle to be written to the target.
// The bundle is created without a password.
PKCS12 *KeySelector `json:"pkcs12,omitempty"`
// Arbitrary password for bundle stores
Password string `json:"password,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we forcing the same password on JKS and PKCS12? I am not sure it's the most flexible solution.

Copy link
Contributor Author

@arsenalzp arsenalzp Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, exactly, because pksc12 and jks are two formats of the same bundle, I don't expect customer wants to have different password for each format of the same bundle.
Does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense?

It really depends on how you look at truststore passwords: If you think the password is a sensitive secret that needs to be protected , it could make sense. But those will not be satisfied with what we are suggesting in this PR anyway. 😄 As commented in #199 (comment) they think we need to put the password/passwords in secrets.

Then there are others, like me, who just consider these binary truststore formats as troublesome and want to use the more standardized PEM format. But are forced to use JKS or PKCS12 by tools that don't want to support PEM. We want it to be as simple as possible to use. And for Java, the situation is like this:

  • Older Java Runtime Environments are delivered with a default JKS truststore, where the password is changeit. I would like to replace this default JKS truststore with as little effort as possible, which means that I want the password to be changeit - to avoid reconfiguration of all applications running on the JRE.
  • Newer Java Runtime Environments have migrated to password-less PKCS12 truststore, ref. https://bugs.openjdk.org/browse/JDK-8275252. In this case I want the truststore to be password-less, for the same reasons as described for the JKS format.

So forcing the same password on both truststore formats will block users like me, who just want this to be as simple as possible and don't think the truststore password is a sensible thing. A workaround is to create two bundle resources, but that will waste a lot of space in etcd - since there is currently no way to turn off the PEM format in bundles.

Does this makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@arsenalzp If you have time, please join our daily stand-up today at 11:30 CET. I thought this would be a "good first issue", but it has turned out to be a lot more challenging than anticipated. Thanks for the stamina!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello,
Yes, it does make sense for me. I implemented a new structures with password parameter for each one, as was proposed by you and your colleague, it wasn't so complicated from the technical perspective, because code is quite clear, however some tests are not so clear for me, so they were falling, but I fixed all issues. I need more time to trim the code.
Thank you for the invitation, I would like to join, but today is Public holidays in Czech Republic, so I'm far away from my laptop and reliable Internet.

This was referenced Nov 23, 2023
Merged
Open
Closed
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Nov 25, 2023
@jetstack-bot
Copy link
Contributor

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

  • f58cdb2 Merge branch 'cert-manager:main' into main

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.

@arsenalzp arsenalzp closed this by deleting the head repository Nov 25, 2023
@erikgb
Copy link
Contributor

erikgb commented Nov 25, 2023

@arsenalzp A tip: Try to use rebase instead of merge. The merge commits creates a lot of issues with DCO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgb erikgb erikgb requested changes

Assignees
No one assigned
Labels
dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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.
3 participants
@arsenalzp @jetstack-bot @erikgb