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.

Sign up for GitHub

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

Jump to bottom

Create generic API for crypto protocol data #644

Merged
merged 7 commits into from
Mar 7, 2020
Merged

Create generic API for crypto protocol data #644

merged 7 commits into from
Mar 7, 2020

Conversation

djc
Copy link
Member

@djc djc commented Feb 26, 2020 •
edited
Loading

As discussed in #638. @kim, what do you think? @demimarie-parity, you might also be interested.

Is AuthenticationData still the best name? It might as well be called Data in crypto::Session, could also be CryptoData to be even more generic.

@djc djc force-pushed the ad branch 2 times, most recently from e5c42c7 to a80e57e Compare February 26, 2020 10:25
@codecov
Copy link

codecov bot commented Feb 26, 2020 •
edited
Loading

Codecov Report

Merging #644 into master will increase coverage by 0.09%.
The diff coverage is 69.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #644      +/-   ##
==========================================
+ Coverage   70.87%   70.97%   +0.09%     
==========================================
  Files          69       69              
  Lines       12072    12072              
==========================================
+ Hits         8556     8568      +12     
+ Misses       3516     3504      -12
Impacted Files Coverage Δ
quinn-proto/src/lib.rs 83.33% <ø> (ø) ⬆️
quinn-proto/src/connection.rs 79.09% <ø> (+0.22%) ⬆️
quinn/tests/many_connections.rs 0% <0%> (ø) ⬆️
quinn/src/tests.rs 94.63% <100%> (ø) ⬆️
quinn-proto/src/tests/mod.rs 97.02% <100%> (-0.01%) ⬇️
quinn-proto/src/shared.rs 42.48% <100%> (+2.48%) ⬆️
quinn-proto/src/crypto/types.rs 36.66% <50%> (ø)
quinn/src/endpoint.rs 78.06% <50%> (-1.22%) ⬇️
quinn-proto/src/crypto/rustls.rs 81.98% <57.14%> (-0.55%) ⬇️
quinn/src/streams.rs 56.06% <66.66%> (ø) ⬆️
... and 5 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 752693e...e77b8c2. Read the comment docs.

@kim
Copy link
Contributor

kim commented Feb 26, 2020

Looks great!

I think Data works, as it’s pronounced “session data”.

@djc
Copy link
Member Author

djc commented Feb 26, 2020

I figured I'd actually try to make quinn generic, and it turned out to be fairly straightforward, mostly adding lots of generic parameters and adding some extra bounds.

Demi-Marie
Demi-Marie reviewed Feb 26, 2020
View reviewed changes
quinn-proto/src/crypto/rustls.rs Outdated Show resolved Hide resolved
quinn-proto/src/endpoint.rs Outdated Show resolved Hide resolved
quinn-proto/src/shared.rs Outdated Show resolved Hide resolved
@Demi-Marie
Copy link
Contributor

Would it help if I wrote tests for this feature?

@djc
Copy link
Member Author

djc commented Feb 26, 2020

Yes, I would love if you could add some tests!

Ralith
Ralith reviewed Feb 27, 2020
View reviewed changes
quinn-proto/src/crypto/rustls.rs Outdated Show resolved Hide resolved
quinn-proto/src/crypto/rustls.rs Outdated Show resolved Hide resolved
quinn-proto/src/crypto.rs Show resolved Hide resolved
@djc djc mentioned this pull request Mar 2, 2020
Closed
Ralith
Ralith reviewed Mar 6, 2020
View reviewed changes
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Only minor issues. Should rustls become an optional feature at the quinn level?

quinn-proto/src/crypto.rs Outdated Show resolved Hide resolved
quinn-proto/src/crypto.rs Outdated Show resolved Hide resolved
quinn/src/connection.rs Outdated Show resolved Hide resolved
quinn/Cargo.toml Outdated Show resolved Hide resolved
quinn/Cargo.toml Show resolved Hide resolved
quinn-proto/Cargo.toml Show resolved Hide resolved
quinn-proto/src/crypto.rs Outdated Show resolved Hide resolved
djc added 3 commits March 6, 2020 22:53
@djc
Move crypto wrapper types back into quinn-proto
33b4915 
This is the second time we flip-flop on this. Previously in
4fd4868 (#351) we moved these types from
quinn into quinn-proto so that they could be used as part of the low-level
configuration API.

Then in 9522b44 (#511) we moved them back
into quinn after some discussion, making the point that the configuration
API at the quinn-proto was not in a great place.

Here, we keep the existing quinn-proto configuration API and merely offer
an abstracted API relying on the wrapper types for use in quinn internals.
@djc
Add AuthenticationData type to confer TLS crypto bits
90cbe2e
@djc
Remove old authentication data APIs
52ec6ea
@djc
Copy link
Member Author

djc commented Mar 6, 2020

Thanks for the review, hope it wasn't too onerous. I did an initial attempt at introducing a rustls feature flag to quinn, but I have not yet figured out how to handle the examples (which all require rustls). I tried to replicate tokio's setup for this without any luck.

Ralith
Ralith reviewed Mar 6, 2020
View reviewed changes
quinn-proto/Cargo.toml Outdated Show resolved Hide resolved
quinn/Cargo.toml Outdated Show resolved Hide resolved
@Ralith
Copy link
Collaborator

Ralith commented Mar 6, 2020

but I have not yet figured out how to handle the examples (which all require rustls). I tried to replicate tokio's setup for this without any luck.

Enumerating the examples explicitly on Cargo.toml with a suitable required-features clause should do the trick, I'd think.

djc added 4 commits March 7, 2020 20:07
@djc
Move TLS client config defaults into quinn-proto
424d62b 
The default client config setup contains low-level rustls code that is
better off in quinn-proto. This also makes the OS store roots and
certificate transparency easily available to quinn-proto users, and
makes it easy to use them by using Cargo-level features.
@djc
Make quinn crate types generic over proto::crypto::Session
277713b
@djc
Remove accidentally added wireshark log
b99bd51
@djc
Make rustls an optional dependency for quinn
e77b8c2
@djc
Copy link
Member Author

djc commented Mar 7, 2020

Alright, I've added a commit that makes rustls/webpki optional for quinn.

Ralith
Ralith approved these changes Mar 7, 2020
View reviewed changes
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Nice work!

@Ralith Ralith merged commit 6cca808 into master Mar 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the ad branch March 7, 2020 20:12
@kim
Copy link
Contributor

kim commented Mar 8, 2020

Great work!

@kim kim mentioned this pull request Mar 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Demi-Marie Demi-Marie Demi-Marie left review comments

@Ralith Ralith Ralith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@djc @kim @Demi-Marie @Ralith