Use features to set key exchange preferences

[cjpatton]

Overwrite boringSSL's default key exchange preferences with safe
defaults using feature flags:

• "kx-client-pq-supported" enables support for PQ key exchange algorithms.
  Classical key exchange is still preferred, but will be upgraded to PQ
  if requested.

• "kx-client-pq-preferred" enables preference for PQ key exchange,
  with fallback to classical key exchange if requested.

• "kx-client-nist-required" disables non-FIPS-compliant algorithms on the client side.

If any of these "kx-*" features are enabled, then don't compile bindings
for SSL_CTX_set1_curves(). This is to prevent the feature flags from
silently overriding curve preferences chosen by the user.

Ideally we'd allow both: that is, use "kx-*" to set defaults, but still
allow the user to manually override them. However, this doesn't work
because by the time the SSL_CTX is constructed, we don't yet know
whether we're the client or server. (The "kx-*" features set different
preferences for each.)

[cjpatton]

I've updated the commit based on feedback on this PR and clarification of the semantics of "fips" and "fips-link-compiled". Please take another look!

[nox]

Cargo features are supposed to be strictly additive so I'm not sure that's the correct vehicle to use. That being said, I offer no alternative.

[cjpatton]

Cargo features are supposed to be strictly additive so I'm not sure that's the correct vehicle to use.

Are you referring to the fact that "kx-safe-default" disables SslContextBuilder::set_curves()?

Hmm, I definitely we agree we should try to follow best practice if we can. (I was not aware of this convention.) I guess one option is to put the set_curves API behind its own feature. The downside would be yet another feature flag for controlling key exchange preference.

Stepping back, the intent of this PR is to make it easy as possible for our Cloudflare-internal users to enable PQ key exchange without having to manually call set_curves. There might be other ways to do this, but based on my own assessment of the code (and my abilities with Rust), this is the lowest-complexity option with the fewest side-effects. See #145 (comment).

[inikulin]

@nox @cjpatton my understanding is that these features are mutually exclusive, so if we add a compile time check that ensures that features are not enabled simultaneously, won't it solve the conceptual problem?

[cjpatton]

@nox @cjpatton my understanding is that these features are mutually exclusive, so if we add a compile time check that ensures that features are not enabled simultaneously, won't it solve the conceptual problem?

Yes, they're mutually exclusive. What would we want to check at compile time? That if "kx-safe-defaults" is set, then set_curves() is missing? How would we do this?

[inikulin]

Yes, they're mutually exclusive. What would we want to check at compile time? That if "kx-safe-defaults" is set, then set_curves() is missing? How would we do this?

in build.rs we can do something like:

let enabled_count = [
    cfg!(feature = "kx-client-pq-supported"),
    cfg!(feature = "kx-client-pq-preferred") 
    cfg!(feature = "kx-client-nist-required")
]
.iter()
.filter(|f| f)
.count();

if enabled_count > 1 {
     panic!("...");
}

we can do similar checks for feature dependencies.
Btw, we should also document feature deps: either this features should implicitly enable PQ patch feature or we need to check if it's enabled in build.rs as shown above and error if not

[cjpatton]

in build.rs we can do something like:

let enabled_count = [
    cfg!(feature = "kx-client-pq-supported"),
    cfg!(feature = "kx-client-pq-preferred") 
    cfg!(feature = "kx-client-nist-required")
]
.iter()
.filter(|f| f)
.count();

if enabled_count > 1 {
     panic!("...");
}

But where would we do this? If I understand correctly what you're after, the goal is something like: if cfg!(feature = "kx-safe-defaults") and SslContextBuilder::set_curves() is defined, then fail to build. How do we express the second condition?

Also, note that it suffices to check if "kx-safe-defaults" is set because this is implied by any of the "kx-*" features.

we can do similar checks for feature dependencies. Btw, we should also document feature deps: either this features should implicitly enable PQ patch feature or we need to check if it's enabled in build.rs as shown above and error if not

Got, will do after rebasing this PR.

[cjpatton]

Rebased.

[cjpatton]

Squashed.

[cjpatton]

Rebased.

[cjpatton]

Changed "nist-required" to "fips-required". Eventually X25519 might be FIPS-validated.

[inikulin]

@cjpatton

But where would we do this?

I think there was a misunderstanding here, I thought that @nox complained that features are mutually exclusive, i.e. kx-client-pq-preferred and kx-client-nist-required can't be used together. Apparently it's not a case.

Changed "nist-required" to "fips-required". Eventually X25519 might be FIPS-validated.

not sure how do I feel about that, this seems to be relatively distant future thing and might be confusing considering currently FIPS-validated revision

[cjpatton]

@cjpatton

But where would we do this?

I think there was a misunderstanding here, I thought that @nox complained that features are mutually exclusive, i.e. kx-client-pq-preferred and kx-client-nist-required can't be used together. Apparently it's not a case.

Ah, OK! In fact, "kx-client-pq-preferred" and "kx-client-nist-required" can be used at the same time. The effect is that P256Kyber768Draft00 is the client's preferred key exchange algorithm.

Changed "nist-required" to "fips-required". Eventually X25519 might be FIPS-validated.

not sure how do I feel about that, this seems to be relatively distant future thing and might be confusing considering currently FIPS-validated revision

No problem, reverted.

[cjpatton]

Fixed up commit message.