Conversation
Codecov Report
@@ Coverage Diff @@
## main #1492 +/- ##
==========================================
- Coverage 96.49% 96.46% -0.03%
==========================================
Files 74 74
Lines 15110 15110
==========================================
- Hits 14580 14576 -4
- Misses 530 534 +4
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@ctz The CI results seem promising. Is this something that could be reviewed if it were rebased on main or are there still aspects in flux? |
|
Yes I plan to rebase & prepare this for review. |
ctz
force-pushed
the
jbp-zeroize
branch
3 times, most recently
from
8da4904 to
e05fc67
Compare
|
#1544 should mean I can drop the last couple of commits on this PR. Then I think it will be ready. |
This is not always a secret quantity, but treating it as such covers zeroisation on `OkmBlock`, and hence tls13::key_schedule. It also covers some intermediate values in hkdf computations.
So we can wrap it in `zeroize::Zeroizing`
This avoids having them as 'loose' unzeroized type on the way to being moved to their final home. This is sufficient, because: - tls12: the secret comes from `tls12::ConnectionSecrets::master_secret()` which borrows from its internal storage; `tls12::ConnectionSecrets::drop` zeroes this storage. - tls13: the secret comes from `resumption_master_secret_and_derive_ticket_psk`, of type `hkdf::OkmBlock`, which we borrow from. Only once the borrow finishes will that be zeroized.
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
LGTM.
I notice the icount benchmarks show a small bump, but it doesn't seem very significant. Perhaps worth running the local benchmarks to see if the instruction delta affects wallclock perf?
FYI @aochagavia this PR might be a good case study for the new method of performance measurement -- instruction counts show a small (and expected) bump, but might cause an outsized impact on real-world performance. |
Having types from dependent crates appear in rcgen's public API makes taking semver incompatible updates to those dependencies tricky: we need to treat it as a semver incompatible change to rcgen itself. To make sure whenever this type leak happens that it was done because of a deliberate choice this branch adds [cargo-check-external-types](https://github.com/awslabs/cargo-check-external-types) to CI. Three existing types that leaked into the API are fixed: `ring::error::KeyRejected`, `ring::error::Unspecified`, and `pem::PemError`. In each case it's simple to translate the specific error types from the dependency into our own `Error` variants that don't expose the type. Two types are allow-listed: `time::offset_date_time::OffsetDateTime` and `zeroize::Zeroize`. The first, `OffsetDateTime` is used throughout the public API. It isn't clear to me if we want to keep that as part of the public API and treat `time` updates carefully, or if we should pursue using a more generic representation. I've left this out for now so it can be discussed. The second, `Zeroize`, could probably be avoided by implementing `Drop` and calling `zeroize` on the sensitive fields manually. That's the approach Rustls implemented (rustls/rustls#1492). I've left this out for in case there was a reason to prefer implementing the trait on the public types.
3 participants
This is for #265. After the Microsoft incident publicised a couple of months ago, I think I changed my mind about the relative importance of this. It also goes hand-in-hand with having more types containing secrets, introduced in the recent crypto provider work.
closes #265