-
Notifications
You must be signed in to change notification settings - Fork 114
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 RPK and PQ crypto features #126
Conversation
660217b
to
b076e42
Compare
a4bf734
to
bcb4b4e
Compare
63ce28e
to
3a4d712
Compare
boring-sys/Cargo.toml
Outdated
bindgen = { version = "0.65.1", default-features = false, features = ["runtime"] } | ||
cmake = "0.1" | ||
[package.metadata.docs.rs] | ||
features = ["rpk", "post-quantum"] |
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 wouldn't call it post-quantum
: BoringSSL already supports PQ. More importantly, BoringSSL already support the key agreement that we want everyone to use.
This just adds extra key agreements we don't want others to use. (We need it temporarily for compliance/testing.)
What about internal-pq
and a note that people shouldn't use it?
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.
would like to avoid stuff that is "internal", could it be "experimental-pq" and could you provide a short summary, so we can explain what's "in the package" and why it's experimental?
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.
Upstream BoringSSL support the post-quantum hybrid key agreement X25519Kyber768Draft00. Most users should stick to that one. Enabling this feature, adds a few other post-quantum key agreements:
- X25519Kyber768Draft00Old is the same as X25519Kyber768Draft00, but under its old codepoint.
- X25519Kyber512Draft00. Similar to X25519Kyber768Draft00, but uses level 1 parameter set for Kyber. Not recommended. It's useful to test whether the shorter ClientHello upsets fewer middle boxes.
- P256Kyber768Draft00. Similar again to X25519Kyber768Draft00, but uses P256 as classical part. It uses a non-standard codepoint. Not recommended.
Presently all these key agreements are deployed by Cloudflare, but we do not guarantee continued support for them.
boring/src/ssl/mod.rs
Outdated
pub const P256_KYBER768_DRAFT00: SslCurve = SslCurve(ffi::NID_P256Kyber768Draft00); | ||
|
||
#[cfg(feature = "post-quantum")] | ||
pub const X25519_KYBER768_DRAFT00: SslCurve = SslCurve(ffi::NID_X25519Kyber768Draft00); |
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.
There is also NID_X25519Kyber768Draft00Old
, the old codepoint for X25519Kyber768Draft00.
boring/src/ssl/mod.rs
Outdated
pub const P256_KYBER768_DRAFT00: SslCurve = SslCurve(ffi::NID_P256Kyber768Draft00); | ||
|
||
#[cfg(feature = "post-quantum")] | ||
pub const X25519_KYBER768_DRAFT00: SslCurve = SslCurve(ffi::NID_X25519Kyber768Draft00); |
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.
This one doesn't need the feature flag: it's in upstream BoringSSL.
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.
SGTM but I don't like very much specifying a single version number in the workspace.
There is no need to always re-release hyper-boring when boring is changed and vice versa, keeping version numbers in each Cargo manifest is better to keep track of what is getting released imo.
@nox it's easier this way to keep track of breaking changes (which is, for example, is every boringssl revision bump). |
No description provided.