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

don't sign X.509 certs #34896

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Conversation

lijunwangs
Copy link
Contributor

@lijunwangs lijunwangs commented Jan 23, 2024

Problem

This is to resurrect PR from #34202 to address merge conflicts and some comments.

Summary of Changes

This get rid of 3rd party components rcgen in the path of private key access to make the code more secure.

Fixes #

riptl and others added 3 commits January 22, 2024 14:31
Nodes currently don't verify X.509 self-signed certificates
because peer authentication is done via TLS 1.3 CertificateVerify.
Thus, encodes an invalid signature in the X.509 certificate instead.
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (a5c470d) 81.7% compared to head (5766457) 81.7%.
Report is 72 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34896     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         826      826             
  Lines      223413   223357     -56     
=========================================
- Hits       182614   182553     -61     
- Misses      40799    40804      +5     

@lijunwangs lijunwangs changed the title Better pkcs8 don't sign X.509 certs Jan 23, 2024
@behzadnouri
Copy link
Contributor

Can you push your commits directly to #34202 so we have all the context and discussions in one place?
These are some guides on how to do that:
https://tighten.com/insights/adding-commits-to-a-pull-request/
https://stackoverflow.com/questions/22237609/

@lijunwangs
Copy link
Contributor Author

lijunwangs commented Jan 25, 2024

Can you push your commits directly to #34202 so we have all the context and discussions in one place? These are some guides on how to do that: https://tighten.com/insights/adding-commits-to-a-pull-request/ https://stackoverflow.com/questions/22237609/

Unfortunately, I do not have permission to push to the firedancer repo. So the technique would not work.

The real change is c1bf6c0 -- rest is just address merge conflicts.

Which is just restore the issuer common name back to "Solana node" from "Solana"

behzadnouri
behzadnouri previously approved these changes Jan 27, 2024
Copy link
Contributor

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

lgtm,
Can you also plz comment on #34202 pointing to this one for reference and close that as well?

@@ -253,7 +251,7 @@ mod tests {
#[test]
fn test_connection_with_specified_client_endpoint() {
// Start a response receiver:
let (response_recv_socket, response_recv_exit, keypair2, response_recv_ip) = server_args();
let (response_recv_socket, response_recv_exit, keypair2, _) = server_args();
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not used anywhere, should server_args() be updated not to return response_recv_ip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

Copy link
Contributor

@behzadnouri behzadnouri left a comment

Choose a reason for hiding this comment

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

lgtm

@lijunwangs lijunwangs merged commit 8fde8d2 into solana-labs:master Jan 29, 2024
46 checks passed
@lijunwangs
Copy link
Contributor Author

Just for record keeping, the CERT looks like the following:

openssl x509 -in dummy.cer -inform DER -text -noout
Certificate:
Data:
Version: 3 (0x2)
Serial Number: 72340172838076673 (0x101010101010101)
Signature Algorithm: ED25519
Issuer: CN = Solana node
Validity
Not Before: Jan 1 00:00:00 1970 GMT
Not After : Jan 1 00:00:00 4096 GMT
Subject:
Subject Public Key Info:
Public Key Algorithm: ED25519
ED25519 Public-Key:
pub:
06:ab:db:0e:56:94:64:04:cc:ae:ab:56:87:55:da:
32:46:23:16:40:e7:ff:21:66:b8:23:94:7c:65:c1:
2f:1f
X509v3 extensions:
X509v3 Subject Alternative Name: critical
DNS:localhost
X509v3 Basic Constraints: critical
CA:FALSE
Signature Algorithm: ED25519
ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:
ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:
ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:ff:
ff:ff:ff:ff:ff:ff:ff:ff:ff:ff

@ripatel-fd
Copy link
Contributor

Thank you @lijunwangs. So happy to see this go through. 🎉
I might do a follow up PR adding more tests today, just to be safe.

ripatel-fd pushed a commit to firedancer-io/firedancer that referenced this pull request Feb 19, 2024
In solana-labs/solana#34896,
Solana Labs chose a slightly different mock cert layout.

This PR updates Firedancer and Solana Labs to use the same.
ripatel-fd pushed a commit to firedancer-io/firedancer that referenced this pull request Feb 19, 2024
In solana-labs/solana#34896,
Solana Labs chose a slightly different mock cert layout.

This PR updates Firedancer and Solana Labs to use the same.
github-merge-queue bot pushed a commit to firedancer-io/firedancer that referenced this pull request Feb 19, 2024
In solana-labs/solana#34896,
Solana Labs chose a slightly different mock cert layout.

This PR updates Firedancer and Solana Labs to use the same.
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

4 participants