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 EC private keys #997

Closed
wants to merge 2 commits into from

Conversation

farcaller
Copy link

This adds support for EC PRIVATE KEY.

Arguably this should be within ring but frankly I'm afraid to touch it as I couldn't even find the github tag for the current version. I suggest the support lands in rustls first so that its clients (e.g. kube-rs) can move on with the required support and then I'll see if this could go into ring.

@farcaller
Copy link
Author

The relevant pemfile change: rustls/pemfile#5

@Darkspirit
Copy link

A dependency to sec1 can't be added. Duplicate of #332 (comment).

@farcaller
Copy link
Author

Any idea where the source of ring 0.16.20 is? I'm not opposed to patching ring but I can't progress as ring from master won't work with rustls.

@farcaller
Copy link
Author

I've made a diff against ring's commit 9cc0d45f4d8521f467bb3a621e74b1535e118188, sent briansmith/ring#1456 and updated this PR to match.

@codecov-commenter
Copy link

Codecov Report

Merging #997 (47b6377) into main (23c5b69) will decrease coverage by 0.02%.
The diff coverage is 38.46%.

❗ Current head 47b6377 differs from pull request most recent head 1cfecb5. Consider uploading reports for the commit 1cfecb5 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   96.17%   96.14%   -0.03%     
==========================================
  Files          59       60       +1     
  Lines        9406     9759     +353     
==========================================
+ Hits         9046     9383     +337     
- Misses        360      376      +16     
Impacted Files Coverage Δ
rustls/src/sign.rs 87.80% <38.46%> (-4.96%) ⬇️
rustls/src/stream.rs 67.74% <0.00%> (-23.93%) ⬇️
rustls/src/conn.rs 93.72% <0.00%> (-2.60%) ⬇️
rustls/src/kx.rs 86.95% <0.00%> (-1.94%) ⬇️
rustls/src/msgs/deframer.rs 97.46% <0.00%> (-0.63%) ⬇️
rustls/src/server/tls12.rs 97.26% <0.00%> (-0.38%) ⬇️
rustls/src/client/hs.rs 97.73% <0.00%> (-0.25%) ⬇️
rustls/src/client/tls12.rs 97.94% <0.00%> (-0.22%) ⬇️
rustls/src/msgs/handshake_test.rs 99.43% <0.00%> (-0.01%) ⬇️
... and 21 more

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 23c5b69...1cfecb5. Read the comment docs.

@ctz
Copy link
Member

ctz commented Feb 6, 2022

Note #998 achieves this another (much less graceful) way. This PR can remain for doing it in the more reasonable way: so please add a git revert 5288fcf to the top.

@farcaller
Copy link
Author

Thanks for merging in a workaround!

@cpu
Copy link
Member

cpu commented Aug 11, 2023

This PR can remain for doing it in the more reasonable way: so please add a git revert 5288fcf to the top.

I think we should close this PR for now since the feature was accomplished with a workaround. The upstream ring project hasn't been active in some time, making it seem unlikely that a better approach will be provided there anytime soon. Even if it were, this branch has a number of conflicts that would need to be addressed. I suspect it would be easier to start a fresh branch inspired by this one when the time comes.

Thanks!

@cpu cpu closed this Aug 11, 2023
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.

None yet

5 participants