libroach: disable core file when encryption is requested.

[mberhault]

Set core size soft/max limits to 0 when encryption-at-rest is enabled.

This spits out a loud warning on stdout (similar to warning about lack
of AES instruction set support) when the (set|get)rlimit calls fail,
or when running on Windows.

Release note (enterprise change): disable core dumps when enabling encryption

[cockroach-teamcity]

This change is 

[mberhault]

Tweaking recommendations in docs for swap and core files: #3375

[bdarnell]

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

---

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

rocksdb::Status DisableCoreFile() {
#ifdef _WIN32
  return rocksdb::Status::NotSupported("preventing crash reports is not supported on Windows");

Does windows even have something analogous to core files? Maybe this should pass silently instead of erroring.

---

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

  }

  std::cerr << "changed maximum core size limit to soft=" << new_lim.rlim_cur

I think successfully disabling core dumps should just be a log message (or silent, if we can't easily get to the logs from c++), not stderr.

---

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

              << "*** WARNING ***" << std::endl
              << "Encryption requested, but could not disable core dumps" << std::endl
              << "Keys will be leaks in code dumps!" << std::endl

s/will be leaks/may be leaked/

I'd put this on stdout instead of stderr. (Why? Because if someone is doing cockroach debug blah | something, you wouldn't want messages like this mixed in with the output)

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

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

Previously, bdarnell (Ben Darnell) wrote…

Does windows even have something analogous to core files? Maybe this should pass silently instead of erroring.

It has at least full memory dumps and WER (windows error reporting), the latter sends mini-dumps to MS.
I honestly don't know how they are triggered or how to prevent them (through an API or otherwise). At some point, you're running a cockroach node with encryption on Windows, which means you're either doing development/testing and the extra output is a good warning to see, or you're in production and we really should be yelling at you.

---

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

Previously, bdarnell (Ben Darnell) wrote…

I think successfully disabling core dumps should just be a log message (or silent, if we can't easily get to the logs from c++), not stderr.

We can't yet as logging from C++ is still conditional on vmodule=rocksdb=3 (I plan on adding a non-conditional logger for rare logging but it's not available yet). Removed. The error should be sufficient.

---

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

Previously, bdarnell (Ben Darnell) wrote…

s/will be leaks/may be leaked/

I'd put this on stdout instead of stderr. (Why? Because if someone is doing cockroach debug blah | something, you wouldn't want messages like this mixed in with the output)

Changed language.

I assume you mean stderr instead of stdout"? As mentioned before, I already have the AES warning on stdout. I'm happy to do both on stderr and move them over to proper logging once hooked up.

[bdarnell]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

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

Previously, mberhault (marc) wrote…

Changed language.

I assume you mean stderr instead of stdout"? As mentioned before, I already have the AES warning on stdout. I'm happy to do both on stderr and move them over to proper logging once hooked up.

D'oh, yes I meant "stderr instead of stdout". I think that the only time cockroach start should use stdout is for the block of addresses and paths that it prints at startup.

[mberhault]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

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

Previously, bdarnell (Ben Darnell) wrote…

D'oh, yes I meant "stderr instead of stdout". I think that the only time cockroach start should use stdout is for the block of addresses and paths that it prints at startup.

Ok. Switched both to std::cerr. I'll move (or add them) to the logging hook once I add one not behind v=3.

[mberhault]

bincheck is happy with linux/darwin but failing to run the windows binary with:

C:/projects/bincheck/mnt/cockroach.exe: error while loading shared libraries: libwinpthread-1.dll: cannot open shared object file: No such file or directory

This doesn't seem related to my change, I'll try other binaries to check.

[mberhault]

Windows build issue fixed in #27438. Will rebase/re-test-build/re-bincheck.

[mberhault]

The Windows binary is returning the error generated when setrlimit fails, as opposed to the status created in the #ifdef _WIN32 block. While this is a suprising effect and isn't a big issue as long as it builds and runs, should we worry about the _WIN32 handling for FlushWall in https://github.com/cockroachdb/cockroach/blob/master/c-deps/libroach/db.cc#L259?

@benesch: do we have a reliable define for windows builds?

[bdarnell]

That's very odd. Could we be missing a header in this file that's present somewhere in rocksdb? Although from my very brief research it looks like _WIN32 is supposed to be defined by the compiler, not any header.

[benesch]

I'm not sure what's going on. After applying this diff

$ git diff
diff --git a/c-deps/libroach/ccl/crypto_utils.cc b/c-deps/libroach/ccl/crypto_utils.cc
index 5193db2..e05f1f8 100644
--- a/c-deps/libroach/ccl/crypto_utils.cc
+++ b/c-deps/libroach/ccl/crypto_utils.cc
@@ -70,6 +70,12 @@ rocksdb_utils::BlockCipher* NewAESEncryptCipher(const enginepbccl::SecretKey* ke
 bool UsesAESNI() { return CryptoPP::UsesAESNI(); }
 
 rocksdb::Status DisableCoreFile() {
+
+#ifdef _WIN32
+#error "win32 is defined"
+#else
+#error "win32 is not defined"
+#endif
 #ifdef _WIN32
   return rocksdb::Status::NotSupported("preventing crash reports is not supported on Windows");
 #else

build/builder.sh make build fails with "win32 is not defined" while build/builder.sh mkrelease windows fails with "win32 is defined." Seems to be working OK?

[mberhault]

uh. Please ignore everything I said. I forgot to log the actual contents of status, so I'm just getting the generic warning from db.cc rather than the specific blah blah Windows blah blah status.

I've added the status to the warning message and will try again. I expect it will work perfectly. Sorry about wasting everyone's time.

[mberhault]

Yeah, that was it: bincheck result

*** WARNING ***
Encryption requested, but could not disable core dumps: preventing crash reports is not supported on Windows
Keys may be leaked in core dumps!

Sorry about that.

[mberhault]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)