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.

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

Rewrite x509 modules using cryptography (v2 with breaking changes) #63099

Merged
merged 33 commits into from
Dec 22, 2022

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Nov 22, 2022

What does this PR do?

  • Rewrites the x509 modules using cryptography instead of M2Crypto
  • There are some breaking changes, so it has to be explicitly enabled in the minion config
  • Addresses current bugs, especially the repeated signing of certificates during state checks
  • Adds Elliptic Curve/Edwards Curve keys support
  • Adds DER/PKCS7/PKCS12 serialization support (PKCS12/PKCS7 requires cryptography release 36/37)

Open questions for the reviewer

  • Do I mark issues related to v1 as fixed and include those in the changelog? How do I submit feature requests for what is essentially a new module?
  • Would it make more sense to not XOR v1 and v2, but simply deprecate v1? That would help justify the list of breaking changes (see the execution module docstring).
  • Is it sensible to directly require cryptography [and increase the required version]? As far as I can tell, it is always part of Salt's indirect requirements (through pyopenssl).

What issues does this PR fix or reference?

Not sure if a breaking v2 counts as a fix, so not marking some of those as fixed for the time.

References:
Fixes: #59169
Fixes: #52167
Fixes: #58165
Fixes: #59315
Fixes: #63103
Fixes: #57535 (tested manually, works)
Fixes: #63248
Fixes: #63249
#63066 (would contribute to eventual fix)
#63085 (would contribute to eventual fix)

Previous Behavior

  • repeatedly signs certificates only to check for changes, aggravated by [BUG] Test mode does not propagate to __states__ when using prereq #62590
  • only RSA support
  • no support for PKCS12
  • generates invalid CSR by default (v3 does not exist for CSR, only certificates)
  • reversed ordering of NameAttributes in encoded Distinguished Names (no spec, but consensus is wide to small scope, e.g. C>ST>L>O>OU>CN. This encoded ordering is reversed when output as a RFC4514 string [this is spec'd]).

New Behavior

  • certificates are only signed when necessary, includes a workaround for prereq
  • additionally support for ECDSA with SECP(256|384|521)R curve keys, EdDSA with ED25519 and ED448 schemes
  • manages PKCS12-embedded keys and certificates
  • generates valid CSR only (only v1 allowed)
  • ordering of encoded NameAttributes from wide to small scope by default (can be overridden). Note: this will cause changes when a certificate/CSR from the previous module is introduced to this one

Merge requirements satisfied?

Commits signed with GPG?

Yes

@lkubb lkubb marked this pull request as ready for review November 25, 2022 18:30
@lkubb lkubb requested a review from a team as a code owner November 25, 2022 18:30
@lkubb lkubb requested review from garethgreenaway and removed request for a team November 25, 2022 18:30
@whytewolf
Copy link
Collaborator

I like this. however it needs a bunch of changelogs. to reflect the changes this is bringing in. also might need to start working through decommissioning the old x509 in favor of this. not just the flag change but calling out that people should start moving over in the logs with a decommission version.

@lkubb
Copy link
Contributor Author

lkubb commented Dec 6, 2022

From what you wrote, I deduce that I should treat this as the x509 module and mark the issues as fixed as well as write feature requests for the big features. Will update this PR soonish with changelog and deprecation.

@lkubb lkubb requested review from Ch3LL and removed request for s0undt3ch December 15, 2022 23:03
Ch3LL
Ch3LL previously approved these changes Dec 16, 2022
@Ch3LL Ch3LL requested a review from s0undt3ch December 16, 2022 20:06
@Ch3LL Ch3LL added the Sulfur v3006.0 release code name and version label Dec 16, 2022
whytewolf
whytewolf previously approved these changes Dec 16, 2022
salt/utils/x509.py Outdated Show resolved Hide resolved
salt/utils/x509.py Outdated Show resolved Hide resolved
salt/utils/x509.py Outdated Show resolved Hide resolved
tests/pytests/integration/modules/test_x509_v2.py Outdated Show resolved Hide resolved
tests/pytests/integration/modules/test_x509_v2.py Outdated Show resolved Hide resolved
tests/pytests/integration/states/test_x509_v2.py Outdated Show resolved Hide resolved
tests/pytests/integration/states/test_x509_v2.py Outdated Show resolved Hide resolved
@Ch3LL Ch3LL requested a review from s0undt3ch December 21, 2022 14:39
@Ch3LL Ch3LL merged commit c258121 into saltstack:master Dec 22, 2022
@Ch3LL
Copy link
Contributor

Ch3LL commented Dec 22, 2022

This will indeed get into 3006.0 :) Thanks for your quick follow up on this PR to get it over the line

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