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

Add support for generating ed25519 keys and certs #1061

Merged
merged 39 commits into from Jun 12, 2023

Conversation

izolight
Copy link
Contributor

It is now possible to generate ed25519 keys and certificates by specifying ed25519 as algo in the json.
This should fix #926

@codecov-io
Copy link

codecov-io commented Nov 24, 2019

Codecov Report

Merging #1061 into master will decrease coverage by 0.12%.
The diff coverage is 37.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
- Coverage   56.40%   56.28%   -0.13%     
==========================================
  Files          76       76              
  Lines        7256     7308      +52     
==========================================
+ Hits         4093     4113      +20     
- Misses       2697     2726      +29     
- Partials      466      469       +3     
Impacted Files Coverage Δ
bundler/bundle.go 80.23% <0.00%> (-4.96%) ⬇️
bundler/bundler.go 78.50% <0.00%> (-0.72%) ⬇️
helpers/derhelpers/derhelpers.go 0.00% <ø> (ø)
helpers/derhelpers/ed25519.go 53.84% <ø> (ø)
signer/signer.go 23.38% <0.00%> (-0.24%) ⬇️
transport/kp/key_provider.go 31.25% <0.00%> (-2.71%) ⬇️
initca/initca.go 61.19% <33.33%> (-0.64%) ⬇️
helpers/helpers.go 72.91% <75.00%> (+0.04%) ⬆️
csr/csr.go 85.47% <81.25%> (-0.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebe0199...6624bf1. Read the comment docs.

@claucece
Copy link
Contributor

Hi @lgarofalo ! I'll review this tomorrow ;)

Copy link
Contributor

@claucece claucece left a comment

Choose a reason for hiding this comment

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

It might need some changes, as well as adding some other tests. @izolight if you want we can do it together...

bundler/bundle.go Outdated Show resolved Hide resolved
bundler/bundle.go Outdated Show resolved Hide resolved
bundler/bundler.go Outdated Show resolved Hide resolved
csr/csr.go Outdated Show resolved Hide resolved
csr/csr.go Outdated Show resolved Hide resolved
initca/testdata/ca_csr_ed25519.json Show resolved Hide resolved
scan/crypto/tls/generate_cert.go Outdated Show resolved Hide resolved
transport/kp/key_provider.go Outdated Show resolved Hide resolved
transport/kp/key_provider.go Outdated Show resolved Hide resolved
transport/kp/key_provider.go Outdated Show resolved Hide resolved
@claucece
Copy link
Contributor

Hi @lgarofalo !

I'll go ahead and make these little changes so it can be merged ;)

@claucece
Copy link
Contributor

Hi, @izolight ! I've created a new pull request with your changes, to fix some things. It is found here: #1097. Let me know if it is ok ;)

@izolight
Copy link
Contributor Author

@claucece I've addressed some of your questions and updated the pr,
still a bit unsure about the type, maybe we should support PRIVATE KEY and Ed25519 PRIVATE KEY?

@underrun
Copy link

any hope for this pr moving forward?

@gitguardian
Copy link

gitguardian bot commented May 26, 2023

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Generic Private Key c639e67 initca/testdata/5min-ed25519-key.pem View secret
- Generic Private Key cf7fb43 initca/testdata/5min-ed25519-key.pem View secret
- Generic Private Key ad00b62 initca/testdata/5min-ed25519-key.pem View secret
- Generic Private Key bca3df9 initca/testdata/5min-ed25519-key.pem View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2023

Codecov Report

Merging #1061 (b43f9a2) into master (c21e85d) will increase coverage by 3.29%.
The diff coverage is 33.84%.

❗ Current head b43f9a2 differs from pull request most recent head ecc0574. Consider uploading reports for the commit ecc0574 to get more accurate results

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
+ Coverage   52.94%   56.23%   +3.29%     
==========================================
  Files          76       76              
  Lines        9095     7350    -1745     
==========================================
- Hits         4815     4133     -682     
+ Misses       3813     2745    -1068     
- Partials      467      472       +5     
Impacted Files Coverage Δ
bundler/bundle.go 80.23% <0.00%> (-4.15%) ⬇️
helpers/derhelpers/derhelpers.go 0.00% <ø> (ø)
helpers/derhelpers/ed25519.go 53.84% <ø> (+5.40%) ⬆️
signer/signer.go 22.81% <0.00%> (+2.66%) ⬆️
transport/kp/key_provider.go 30.89% <0.00%> (-0.90%) ⬇️
bundler/bundler.go 77.45% <8.33%> (+10.32%) ⬆️
initca/initca.go 61.19% <25.00%> (+3.16%) ⬆️
helpers/helpers.go 73.21% <75.00%> (+0.37%) ⬆️
csr/csr.go 84.83% <81.25%> (+8.44%) ⬆️
errors/error.go 73.07% <100.00%> (-0.30%) ⬇️

... and 64 files with indirect coverage changes

@izolight
Copy link
Contributor Author

merged changes from #1097

@izolight izolight requested a review from claucece May 26, 2023 20:27
Copy link
Contributor

@lukevalenta lukevalenta left a comment

Choose a reason for hiding this comment

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

Thanks for driving this PR forward! I left a few comments and requested changes, but overall this is looking good.

errors/error.go Outdated Show resolved Hide resolved
initca/initca.go Outdated Show resolved Hide resolved
initca/initca_test.go Outdated Show resolved Hide resolved
initca/initca_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@claucece claucece left a comment

Choose a reason for hiding this comment

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

Looks good. I agree with @lukevalenta .

I no longer work at Cloudflare, so I don't have time to check in depth these PRs, but happy to see this work move forward.

@lukevalenta
Copy link
Contributor

@nickysemenza this PR looks good to me but I lack permissions to approve. Would you be able to add this to your review queue?

@nickysemenza nickysemenza merged commit 9a0778d into cloudflare:master Jun 12, 2023
4 of 5 checks passed
@adamacosta
Copy link

Is this going to make a release sometime soon? It was confusing as hell to spend the last two hours wondering why I couldn't generate a ed25519 key when your csr.go file pretty clearly shows a case match on it, to see the original request for support was from five years ago, then to finally look at the date on this merge and the date of the last release and realize you haven't released since April.

I'm perfectly willing to build from main and give it a whirl, but man, maybe merge the website updates from three and a half years ago: #1095

Not a great experience to spend all this time digging through your source code because there's no documentation only to find out the source code doesn't actually match the latest release.

I know it's a free tool so honestly I apologize if this sounds harsh. Dealing with demanding jackasses on Github can be a pain, so honest to God, I've gotten a ton out of Cloudflare knowledge base material in general in the past and I really do appreciate it. This toolsuite is still a heck of a lot less inscrutable than openssl and at least the source code is very readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

how to generate ed25519 cert
8 participants