Skip to content

Switch go-pkcs12 to use LegacyDES encoder#6515

Closed
erikgb wants to merge 1 commit intocert-manager:masterfrom
erikgb:pkcs12-modern
Closed

Switch go-pkcs12 to use LegacyDES encoder#6515
erikgb wants to merge 1 commit intocert-manager:masterfrom
erikgb:pkcs12-modern

Conversation

@erikgb
Copy link
Copy Markdown
Member

@erikgb erikgb commented Nov 24, 2023

Pull Request Motivation

Our dependency to build PKCS12 keystores, https://github.com/SSLMate/go-pkcs12, has released newer versions that deprecate our current use of the dependency. The recommendation is to switch to the more "modern" encoder, but I am pretty sure this would be a breaking change. For this reason, I suggest migrating to the less secure, but more compatible, LegacyDES encoder.

Related upstream PR: SSLMate/go-pkcs12#48.

Relates to #6511. I am unsure if this PR (without switching to the less compatible Modern encoder) solves the issue reported in the security audit, but I think compatibility is more important. The JKS and PKCS12 encodings are not secure by just the password anyway.

Kind

/kind cleanup

Release Note

Upgrade go-pkcs12 to latest version and switch to LegacyDES encoder

@jetstack-bot jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/testing Issues relating to testing labels Nov 24, 2023
@jetstack-bot
Copy link
Copy Markdown
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 maelvls for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 24, 2023
@erikgb erikgb changed the title Upgrade go-pkcs12 to latest version and switch to Modern encoder Upgrade go-pkcs12 to latest version and switch to LegacyDES encoder Nov 24, 2023
Copy link
Copy Markdown
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.

My initial reaction to this was that we should use LegacyRC2 to avoid the chance of a breaking change, but then I saw this line in the docs:

By default, OpenSSL 3 can't decode PKCS#12 files created using this encoder. For better compatibility, use LegacyDES. For better security, use Modern2023.

OpenSSL losing the ability to do RC2 makes me think that defaulting to DES might be a good shout (we definitely shouldn't care about the actual strength of the encryption - this is all irrelevant for security).

It's definitely a breaking change though; there could be tonnes of stuff out there that can do RC2 but not DES. I'd expect there probably isn't tonnes of stuff like that, but it's hard to tell.

I don't know how we could possibly evaluate which is actually going to be better for users, which makes me think that what we should really do is make this an option, which feels like a lot of work for not much benefit but might be the only option.

We might need to discuss this at a biweekly meeting to decide 🤔 I'll add it to next week's agenda.

@erikgb
Copy link
Copy Markdown
Member Author

erikgb commented Nov 26, 2023

My initial reaction to this was that we should use LegacyRC2

Yes, that seems to result in the same behavior as we have currently.

@inteon
Copy link
Copy Markdown
Member

inteon commented Nov 27, 2023

Let's start with using LegacyRC2.WithRand(rand).EncodeTrustStore(certs, password). Later we can switch to LegacyDES.
I'll create a PR for that first change.

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
@jetstack-bot jetstack-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 28, 2023
@erikgb erikgb changed the title Upgrade go-pkcs12 to latest version and switch to LegacyDES encoder Switch go-pkcs12 to use LegacyDES encoder Nov 28, 2023
@erikgb erikgb closed this Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Issues relating to testing dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants