Skip to content

Conversation

@mberhault
Copy link
Contributor

@mberhault mberhault commented Jun 12, 2018

Fixes #26383

Set appropriate -m flags for cryptopp to enable AES runtime detection
checks (without those, it does not even try).

Add a UsesAESNI() function in CryptoPP which returns true iff:

  • AES-NI runtime detection is compiled in
  • AES-NI instruction is available

Add a warning to stdout (normal logging requires vmodule=rocksdb=3) if
encryption is requested but AES-NI is not available.

Add a test to make sure our builds always have AES-NI enabled.

Release note (core): build release binaries with runtime AES detection.

@mberhault mberhault requested review from a team and benesch June 12, 2018 18:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mberhault
Copy link
Contributor Author

I probably won't leave that test in. I'm going to check it with bincheck first.

@bdarnell
Copy link
Contributor

:lgtm: as long as bincheck passes.


Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


c-deps/libroach/ccl/crypto_utils_test.cc, line 12 at r1 (raw file):

#include "crypto_utils.h"

TEST(CryptoUtils, HasAESNI) { EXPECT_TRUE(UsesAESNI()); }

So we expect this test to fail if run on a CPU that doesn't have AESNI? Or is this just checking whether AESNI was compiled in?


Comments from Reviewable

@mberhault
Copy link
Contributor Author

I'm also not sure whether TC passes. Ultimately, that one function is more about information when encryption is enabled (show a big warning to the user, maybe even ask for a special flag to allow encryption without AES-NI).

@mberhault
Copy link
Contributor Author

Added a loud warning on stdout (normal logging requires vmodule=rocksdb=3 which is as good a invisible) when encryption is enabled and we the AES instruction set is not available.

This also passes on bincheck's test-linux. It triggers the warning about lack of AES-NI but proceeds correctly. I'll tweak bincheck to 1) add encryption for appropriate versions and probably 2) support running against edge binaries.

@mberhault
Copy link
Contributor Author

Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


c-deps/libroach/ccl/crypto_utils_test.cc, line 12 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

So we expect this test to fail if run on a CPU that doesn't have AESNI? Or is this just checking whether AESNI was compiled in?

This checks for both. The function I added to cryptopp returns the value of HasAESNI() (which performs the runtime cpu check) but inside a #if CRYPTOPP_BOOL_AESNI_INTRINSICS_AVAILABLE which is the big compile-time wrapper for all of these. This is the same thing the encrypt/decrypt steps do.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

I've tested this on a variety of qemu processor features including with/without sse4.2, aes, pclmul, ssse3. All are happy and the warning only shows up if aes is disabled (pclmul and ssse3 are only used for GCM and are still guarded by runtime checks).

@mberhault
Copy link
Contributor Author

I've run bincheck against a manual test release build (using cockroachdb/bincheck#50). Surprisingly, it even works on windows. I'm almost shocked.

@mberhault mberhault force-pushed the marc/cryptopp_runtime_aes_detection branch from 7691b23 to 6882860 Compare June 13, 2018 16:31
Fixes cockroachdb#26383

Set appropriate `-m` flags for cryptopp to enable AES runtime detection
checks (without those, it does not even try).

Add a `UsesAESNI()` function in CryptoPP which returns true iff:
* AES-NI runtime detection is compiled in
* AES-NI instruction is available

Add a warning to stdout (normal logging requires `vmodule=rocksdb=3`) if
encryption is requested but AES-NI is not available.

Add a test to make sure our builds always have AES-NI enabled.

Release note (core): build release binaries with runtime AES detection.
@mberhault mberhault force-pushed the marc/cryptopp_runtime_aes_detection branch from 6882860 to 6881f30 Compare June 13, 2018 16:33
@benesch
Copy link
Contributor

benesch commented Jun 14, 2018

:lgtm:


Reviewed 7 of 7 files at r1, 1 of 1 files at r2.
Review status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)


c-deps/libroach/ccl/crypto_utils_test.cc, line 12 at r1 (raw file):

Previously, mberhault (marc) wrote…

This checks for both. The function I added to cryptopp returns the value of HasAESNI() (which performs the runtime cpu check) but inside a #if CRYPTOPP_BOOL_AESNI_INTRINSICS_AVAILABLE which is the big compile-time wrapper for all of these. This is the same thing the encrypt/decrypt steps do.

Yeah, this seems like a weird thing to put into a test. Doesn't seem like tests should fail if hardware support isn't available. Maybe just test HasAESNI instead? Don't feel strongly though.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

Review status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)


c-deps/libroach/ccl/crypto_utils_test.cc, line 12 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, this seems like a weird thing to put into a test. Doesn't seem like tests should fail if hardware support isn't available. Maybe just test HasAESNI instead? Don't feel strongly though.

Ok, I've changed it to check HasAESNI vs UsesAESNI along with a longish comment explaining the success/failure conditions. Basically, this will now only fail if the build disables hardware-accelerated AES and the instruction set is available.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Jun 14, 2018
26649: libroach: make CryptoPP build with runtime AES-NI detection. r=mberhault a=mberhault

Fixes #26383

Set appropriate `-m` flags for cryptopp to enable AES runtime detection
checks (without those, it does not even try).

Add a `UsesAESNI()` function in CryptoPP which returns true iff:
* AES-NI runtime detection is compiled in
* AES-NI instruction is available

Add a warning to stdout (normal logging requires `vmodule=rocksdb=3`) if
encryption is requested but AES-NI is not available.

Add a test to make sure our builds always have AES-NI enabled.

Release note (core): build release binaries with runtime AES detection.

Co-authored-by: marc <marc@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jun 14, 2018

Build succeeded

@craig craig bot merged commit 6881f30 into cockroachdb:master Jun 14, 2018
@mberhault mberhault deleted the marc/cryptopp_runtime_aes_detection branch June 14, 2018 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants