-
Notifications
You must be signed in to change notification settings - Fork 110
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 example of generating a new CA and a new server cert signed by it #99
Conversation
1651a13
to
9316961
Compare
1c6f6d6
to
c5afc0b
Compare
Build failure related to parsing a revoked certificate might be fixed by #142. |
0c67396
to
1d8dbfe
Compare
@@ -33,7 +33,9 @@ default = ["pem"] | |||
features = ["x509-parser"] | |||
|
|||
[dev-dependencies] | |||
native-tls = "0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this with rustls we can don't even need the pipe crate, and we can avoid relying on C libraries.
(Honestly, I'm not sure it makes sense to include a whole TLS setup in the example -- that seems to be more like a testing thing?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked having a complete end-to-end example because it shows how everything ties together, leaving less to research/guess. Some users of the crate may be coming to it with a significant amount of TLS knowledge, and others may be coming into it looking for a solution to some problem, but only basic TLS knowledge. A larger example will help people in the latter group (personally I'm somewhere between the two).
I'll take a look into using rustls
instead of nativetls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my case the 'solution to a problem' was 'how can i create temporary servers for testing with temporary TLS/SSL certificates, and connect clients without suppressing TLS/SSL validations' - a common testing scenario.
I've added a commit to replace usage of
I'm sure I'm missing something obvious, lol. |
502f04c
to
5a4a92c
Compare
Here's the current output:
Client gets stuck at I'm sure this is a lack of understanding on my part of one of the crates I'm trying to use. Help would be appreciated. |
5a4a92c
to
b0f24f3
Compare
It's getting stuck in the handshake step ( |
Current output after additional commits:
Gets stuck in same places. |
I haven't had a chance to review the code you have in progress, but you might be able to crib from the |
Thanks for the suggestion. I've looked thru that code (as well as It was pretty easy to get it working with |
This isn't a problem specific to the I added Thanks for your help, I'll keep digging. |
Late to the discussion but +1 to using the |
Agree. |
35b27d1
to
671fe5f
Compare
@est31 / @cpu / @djc there's a difference in behavior between Let's assume the following behavior (implemented in all three
With With
If If Is there a bug in rustls - should it be able to run on a buffered bidirectional pipe with a rusttls client hooked up to one end of the pipe and a rusttls server hooked up to the other end? |
Also, while I have included |
671fe5f
to
e2cbb8b
Compare
// Does not work with either buffered or unbuffered bipipe, even when we use the same message | ||
// pattern as with the nativetls example, where client sends then reads, and server reads and | ||
// then sends. Client blocks sending a message; server blocks reading a message. | ||
let (mut p_client, mut p_server) = pipe::bipipe_buffered(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's really no need to use a pipe thing here. You can just directly use a Vec<u8>
buffer (or two) that one side writes into and the other side then reads from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vectors couldn't be moved into the server thread and client threads, because it would require moving each vector twice.
e2cbb8b
to
b3d2ef1
Compare
Codecov Report
@@ Coverage Diff @@
## main #99 +/- ##
=======================================
Coverage 71.60% 71.60%
=======================================
Files 7 7
Lines 1729 1729
=======================================
Hits 1238 1238
Misses 491 491 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Is the status of this branch still accurately reflected in the "Do not merge" PR title? I appreciate the effort you've put into this work, and I do agree that better examples are needed. However, I think the approach taken here is too heavy-weight. It's more of an example of using the downstream TLS crates vs using I would feel a lot better about examples that focused exclusively on certificate generation, the domain of rcgen. If it's true that there are still gaps that make it challenging for a new user to use the generated certificates with Rustls then I think the best course of action would be to try and extend the existing Rustls examples to better resolve those challenges. Apologies for not voicing these opinions earlier. |
No worries. I was actually starting to get that same feeling myself - like the scope of the proposed code changes had changed, and that at least some portion of what it became might belong in another crate. It was here originally to show how to use rcgen to solve a problem, and provide an end-to-end solution. I am a little concerned about the different behavior of the tls implementations I documented in a comment above, but that may belong elsewhere also: "there's a difference in behavior between rustls and nativetls when the client and server are communicating over a bidirectional pipe from the pipe crate." Let me think about it a bit and I may proactively refactor or close the PR. |
Resolves #79