-
Notifications
You must be signed in to change notification settings - Fork 597
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
Default to require_ems
in FIPS mode
#1772
Conversation
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
@@ -280,7 +281,15 @@ impl ClientConfig { | |||
/// Return true if connections made with this `ClientConfig` will | |||
/// operate in FIPS mode. | |||
pub fn fips(&self) -> bool { | |||
self.provider.fips() | |||
#[cfg(feature = "tls12")] |
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.
If we're going to make the config-side fips()
depend on the config, should we make the CryptoProvider::fips()
pub(crate)
to make it harder to accidentally rely on the CryptoProvider::fips()
where that is not sufficient?
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 think -- I hope -- that there is not much confusion in semantics between CryptoProvider::fips()
and Client/ServerConfig::fips()
, even if they were functionally the same up until this commit. Will extend the docs though.
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.
Maybe the distinction needs to be explained in CryptoProvider::fips()
? That's where it's risky, if you use that fips()
but fail to check the config.
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.
That seems like a good idea to me
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 have added some clarifying text to CryptoProvider::fips()
-- please check.
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.
LGTM
c7800cc
to
d1bd423
Compare
Change default for `require_ems` based on `fips` crate feature, generalising the existing tests for `require_ems` to verify this too. Include `require_ems` in `fips()` determination.
d1bd423
to
e8958e3
Compare
Change default for
require_ems
based onfips
crate feature, generalising the existing tests forrequire_ems
to verify this too.Include
require_ems
infips()
determination.