Extract traits under crypto use (part 1) #1350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ctz
commented
Jul 10, 2023
cpu
approved these changes
Jul 10, 2023
ctz
commented
Jul 11, 2023
Codecov Report
@@ Coverage Diff @@
## main #1350 +/- ##
==========================================
- Coverage 96.39% 96.35% -0.05%
==========================================
Files 61 62 +1
Lines 14472 14554 +82
==========================================
+ Hits 13950 14023 +73
- Misses 522 531 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ctz
force-pushed
the
jbp-cpu-djc-crypto++-pt1
branch
from
c8861b8
to
809c7d1
Compare
cpu
approved these changes
Jul 12, 2023
ctz
force-pushed
the
jbp-cpu-djc-crypto++-pt1
branch
2 times, most recently
from
76d598d
to
1d89c17
Compare
The only consumers of the `pub(crate)` visible `pubkey` field of the `KeyExchange` struct were using it to get at a `&[u8]` of public key bytes. This commit: 1. Unexports the `pubkey` field of the `KeyExchange` struct. 2. Adds a `pub(crate)` visible `pub_key()` method to return the public key as a `&[u8]`. 3. Adjusts the tls12 client `emit_clientkx` function to use `&[u8]` for its pub key argument. 4. Adjusts all callers to use the new `pub_key` accessor in place of the field. The name is changed from `pubkey` to `pub_key` to match Rust naming conventions[0]. [0]: https://rust-lang.github.io/api-guidelines/naming.html
Following up on the previous commit, this commit updates the `KeyExchange` struct's private `skxg`, `pubkey` and `privkey` fields to be named `group`, `pub_key` and `priv_key`. This better matches the Rust naming convention for struct members and makes for easier to understand code.
For better code organization this commit moves the generic crypto interface code from `src/crypto.rs` to `src/crypto/lib.rs`. The *ring* specific code implementing the generic interfaces is moved to `src/crypto/ring.rs` as a sub-module of `crypto. All imports are adjusted accordingly. This has the advantage of leaving `src/crypto/lib.rs` small, and without any *ring* specific imports. In the future we may choose to feature-gate the ring sub-module to allow building the crate without a dependency on ring.
The `KeyExchangeError` type is generic enough to live in the `crypto` module. This will allow it to be shared with non-ring implementations in the future.
This commit moves the existing Ring-based key exchange mechanisms from `rustls/src/kx.rs` to `rustls/src/crypto/ring.rs` in anticipation of adapting the codebase to a more general keyex trait that these types will implement. No changes are made to the implementation except to update import paths to reference the new location.
This commit adds a trait for referring to supported key exchanges over named groups in a general fashion. The *ring* specific `SupportedKxGroup` type is then made to implement this trait.
This commit adds a `KeyExchange` associated type to the `CryptoProvider` trait. The `KeyExchange` type is constrained with its own `KeyExchange` trait that has an associated type for the `SupportedGroup`. In the `crypto::ring` package we adapt the existing *ring* specific `KeyExchange` and `SupportedKxGroup` types to these new traits. Throughout the codebase we tighten generic bounds where required to ensure we have a `CryptoProvider` bound that allows accessing the associated `KeyExchange` and `SupportedGroup`. We also make the `CryptoProvider` an associated type on the `Side` config.
The `KeyExchange` trait's methods were ordered constructors -> complex functions -> less complex functions. The original *ring* specific `KeyExchange` didn't match this ordering. This commit synchronizes the two.
Instead, the ring-based `rustls::Ticketer` is exported directly, as is the `TicketSwitcher` which is a useful building block for downstream users.
This replaces the one use of `start()` (in TLS1.2 server) with `choose()`, and then calls the result `start()` which I think is slightly clearer.
ctz
force-pushed
the
jbp-cpu-djc-crypto++-pt1
branch
from
1d89c17
to
caa89e6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a first third of #1319 that I think we can land now. There was some previous review of the final commit on #1319 -- this PR includes addressing those issues.