libroach: add key manager #20670

mberhault · 2017-12-13T03:28:17Z

Release Note: none.

Add the FileKeyManager which loads active/old keys from files.
This will be used for the store keys.

A bunch of build/test things here:

finally use CryptoPP. Only linked against CCL code
add testutils for easier testing
change existing tests to testutils
add FormatString

cockroach-teamcity · 2017-12-13T03:28:22Z

This change is

mberhault · 2017-12-13T03:28:46Z

@benesch for build changes. Nothing shocking here. The beginnings of testutils though!

mberhault · 2017-12-13T03:29:49Z

c-deps/libroach/ccl/key.cc

+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+#include "key.h"
+#include <filters.h>  // CryptoPP


I'm putting comments here because it's not otherwise clear what these are for. cryptopp doesn't have an include subdir. Any ideas? I don't particularly want to re-organize everything.

I'd change the include path to be .. instead of ../cryptopp and then refer to everything as cryptopp/FOO.

mberhault · 2017-12-13T03:32:32Z

c-deps/libroach/testutils.h

+#define EXPECT_OK(status) EXPECT_TRUE(status.ok()) << "got: " << status.getState();
+#define ASSERT_OK(status) ASSERT_TRUE(status.ok()) << "got: " << status.getState();
+
+#define EXPECT_ERR(status, err_msg)\


I'm not a fan of macros either, but I like the in-line error reporting and I'm kind of just following gtest here. The compareErrorMessage is a function though.

bdarnell · 2017-12-13T21:11:58Z

Reviewed 12 of 19 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

c-deps/libroach/CMakeLists.txt, line 44 at r2 (raw file):

  PUBLIC  ./include
  PRIVATE ../protobuf/src
  PRIVAGE ../rocksdb/include

PRIVAGE?

c-deps/libroach/testutils.h, line 22 at r2 (raw file):

#include "util.h"

namespace testutils {

Will this be a problem as soon as this header is included from multiple source files? I think the function definitions should go in a .cc file.

c-deps/libroach/testutils.h, line 53 at r2 (raw file):

// clang-format off

#define EXPECT_OK(status) EXPECT_TRUE(status.ok()) << "got: " << status.getState();

This evaluates the (textual) expansion of status twice, which is not what you want. You need to assign it to a variable.

c-deps/libroach/testutils.h, line 56 at r2 (raw file):

#define ASSERT_OK(status) ASSERT_TRUE(status.ok()) << "got: " << status.getState();

#define EXPECT_ERR(status, err_msg)\

Add some braces around these two lines so it doesn't break if there's already an s in the local namespace.

c-deps/libroach/util.cc, line 18 at r2 (raw file):

#include <string>

std::string FormatString(const char* fmt, ...) {

Why not just use google::protobuf::StringPrintf? You could put using google::protobuf::StringPrintf in a header to make it less verbose.

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

Previously, mberhault (marc) wrote…

I'm putting comments here because it's not otherwise clear what these are for. cryptopp doesn't have an include subdir. Any ideas? I don't particularly want to re-organize everything.

If you make install cryptopp, it puts its headers in /usr/local/include/cryptopp. I'm not sure how to make use of that in our cmake setup, though. You could use a symlink to recreate the directory structure?

c-deps/libroach/ccl/key_manager_test.cc, line 32 at r2 (raw file):

      {"64.key", "1234567890123456789012345678901234567890123456789012345678901234"},
  };
  for (auto k = keys.cbegin(); k != keys.cend(); ++k) {

Does for (auto k : keys) (c++11 range-based for loop) work in our build environment?

Comments from Reviewable

mberhault · 2017-12-13T22:41:48Z

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

c-deps/libroach/CMakeLists.txt, line 44 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

PRIVAGE?

uh. it's a new keyword? Nevermind, fixed.

c-deps/libroach/testutils.h, line 22 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Will this be a problem as soon as this header is included from multiple source files? I think the function definitions should go in a .cc file.

It's fine because we only include it from _test.cc and we build one executable per test file. Still, moving to its own .cc prevents future breakage. Done.

c-deps/libroach/testutils.h, line 53 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This evaluates the (textual) expansion of status twice, which is not what you want. You need to assign it to a variable.

ouch! good point. Done.

c-deps/libroach/testutils.h, line 56 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Add some braces around these two lines so it doesn't break if there's already an s in the local namespace.

Done. and the modified {ASSECT,EXPECT}_OK above as well

c-deps/libroach/util.cc, line 18 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Why not just use google::protobuf::StringPrintf? You could put using google::protobuf::StringPrintf in a header to make it less verbose.

oh yeah, that's probably easier. Sticking using where I'm calling it for now. I no longer have a util.h. I'll move it back to a header when it gets tedious.

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

Previously, bdarnell (Ben Darnell) wrote…

If you make install cryptopp, it puts its headers in /usr/local/include/cryptopp. I'm not sure how to make use of that in our cmake setup, though. You could use a symlink to recreate the directory structure?

We use the source tarball that we then build in a completely separate directory. I don't think we want to put semi-built things in our deps, being able to have straightforward refreshes from upstream is nice. What happens if I just use -I c-deps/? might that wreak havoc with other deps? (the answer is probably to try)

c-deps/libroach/ccl/key_manager_test.cc, line 32 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Does for (auto k : keys) (c++11 range-based for loop) work in our build environment?

It does, even better. Thanks! I forgot about those.

Comments from Reviewable

benesch

Build stuff looks good to me, but I have some high-level concerns about cryptopp. I realize it might be too late, and I assume you and @bdarnell have already evaluated the other crypto options, @mberhault?

The things that worry me are:

They're removing support for CMake in the next release source. The Makefile they ship with the project doesn't use autotools, either, so it's going to be a pain to do our out-of-tree cross-compilation if/when we upgrade.
Their source tree organization is not super friendly. It also seems to bring in a lot more than we need, but perhaps that's true of every crypto library.
The project itself seems to have a rather low bus factor. Seems like the only active contributor is the current maintainer.

I guess I'm just not convinced that we shouldn't just be using OpenSSL or LibreSSL.

Anyway, don't mean to hold you up! Obviously you're the one who will benefit/suffer the most from the choice of crypto library. I can be very easily convinced that cryptopp is the way to go—just wanted to put my thoughts somewhere.

benesch · 2017-12-14T00:27:47Z

c-deps/libroach/testutils.h

+// clang-format isn't so great for macros.
+// clang-format off
+
+#define EXPECT_OK(status) { auto s(status); EXPECT_TRUE(s.ok()) << "got: " << s.getState(); }


Why do you have to extract a variable? I'd expect this to work:

#define EXPECT_OK(status) EXPECT_TRUE(status.ok()) << "got: " << s.getState();

there's no s in the << s.getState() in your code. If I use status both times, it may be a function that gets run twice.

benesch · 2017-12-14T00:42:05Z

c-deps/libroach/ccl/key.cc

+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+#include "key.h"
+#include <filters.h>  // CryptoPP


I'd change the include path to be .. instead of ../cryptopp and then refer to everything as cryptopp/FOO.

benesch · 2017-12-14T00:50:02Z

c-deps/libroach/testutils.cc

+
+using google::protobuf::StringPrintf;
+
+namespace testutils {


nit: missing space (you have one at the end, and this appears to be our convention)

if you mean blank line, done. Otherwise, I'll need some clarification.

benesch · 2017-12-14T00:50:32Z

c-deps/libroach/testutils.cc

+    // Expected success.
+    if (status.ok()) {
+      return rocksdb::Status::OK();
+    } else {


nit: unnecessary else

benesch · 2017-12-14T00:53:19Z

c-deps/libroach-rebuild

 Bump the version below when changing libroach CMake flags. Search for "BUILD
 ARTIFACT CACHING" in build/common.mk for rationale.

-1


c-deps/cryptopp-rebuild should get this same explanatory comment (sorry, GH won't let me comment there)

Thanks for remembering to bump this one!

uh. oops. I thought I had it. Turns out mine was blank.

benesch · 2017-12-14T01:22:19Z

Yeah, blank line is what I meant. Also ignore my macro comment. Just saw Ben’s concerns about double eval.

…

On Wed, Dec 13, 2017 at 8:21 PM marc ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In c-deps/libroach/testutils.cc <#20670 (comment)> : > +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +// implied. See the License for the specific language governing +// permissions and limitations under the License. + +#include <google/protobuf/stubs/stringprintf.h> +#include <gtest/gtest.h> +#include <rocksdb/status.h> +#include <string> + +using google::protobuf::StringPrintf; + +namespace testutils { if you mean blank line, done. Otherwise, I'll need some clarification. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20670 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IB4ZCp1nB-WP3gBogESjL_j6AhI8ks5tAHgPgaJpZM4Q_9fL> .

mberhault · 2017-12-14T01:52:52Z

The main crypto libs for C++ are CryptoPP and Botan. They both provide reasonable AES interfaces (you can get fairly basic block-level primitives) and both support about the same ciphers and modes should we want to expand.

The C ones, and especially OpenSSL and LibreSSL are.... gross. Both to use and to dig through. I can read through the CryptoPP code and understand what's going on, and using it is fairly straightforward. I pretty quickly gave up on OpenSSL.

That said, it's definitely not too late and nothing says we have to stick with CryptoPP. The only things we need it for right now are: AES block-level encryption/decryption (we don't use the higher-level multi-block modes, we do our own), sha-256 (easy to find), and PRNG (but that's just /dev/[u]?random which isn't exactly hard to get to). Should we choose a different implementation, we just need to replace those small bits. It's not like we have a compatibility problem anyway, either you do AES properly or you don't.

So my opinion (I haven't done nearly enough research into all options to be anywhere close to objective) is to stick with CryptoPP. For info, here's how I used it when testing things out. We replace that one class and we're done. The other uses are just sha-256 and call to OS_GenerateRandomBlock.

mberhault · 2017-12-14T01:57:14Z

I've added a commit that puts c-deps in the include path. It works, I'm just wondering if there are any potential bad (present or future) side-effects. Thoughts?

benesch · 2017-12-14T05:47:20Z

.. is only a problem if we ever add a project structured like

c-deps/proj/proj.h
c-deps/proj/include/proj/proj.h

where the top-level proj.h is internal-only and the include/proj/proj.h is the public header file. If the import paths are .. and ../proj/include, the value of #include <proj.h> will include whichever happens to be in the first -I include path passed to the compiler.

Our dependencies right now always either mix public and private header files at the root (so the private headers are all called "-internal.h" or something), or put private header files within c-deps/proj/src/, so it's not an issue.

Symlinks would solve the problem nicely but they break the build on Windows.

The main crypto libs for C++ are CryptoPP and Botan. They both provide reasonable AES interfaces (you can get fairly basic block-level primitives) and both support about the same ciphers and modes should we want to expand.

The C ones, and especially OpenSSL and LibreSSL are.... gross. Both to use and to dig through. I can read through the CryptoPP code and understand what's going on, and using it is fairly straightforward. I pretty quickly gave up on OpenSSL.

That said, it's definitely not too late and nothing says we have to stick with CryptoPP. The only things we need it for right now are: AES block-level encryption/decryption (we don't use the higher-level multi-block modes, we do our own), sha-256 (easy to find), and PRNG (but that's just /dev/[u]?random which isn't exactly hard to get to). Should we choose a different implementation, we just need to replace those small bits. It's not like we have a compatibility problem anyway, either you do AES properly or you don't.

So my opinion (I haven't done nearly enough research into all options to be anywhere close to objective) is to stick with CryptoPP. For info, here's how I used it when testing things out. We replace that one class and we're done. The other uses are just sha-256 and call to OS_GenerateRandomBlock

Thanks for the explainer! Yeah, OpenSSL has a horrible, terrifying API; on the other hand, it's widely used. I get scared when I see bugs like weidai11/cryptopp#146 in cryptopp, though. Who knows what other nearly-invisible bugs are lurking.

All that said, happy to leave the decision to you. I really don't have much experience with crypto.

mberhault · 2017-12-14T13:26:05Z

An alternative to the wide include would be to just extract the source to cryptopp/cryptopp. This obviously stutters and makes our makefile rules a bit weirder, but I could live with it.

mberhault · 2017-12-14T14:44:25Z

c-deps/libroach/fmt.h

+
+#include <google/protobuf/stubs/stringprintf.h>
+
+namespace fmt = google::protobuf;


@bdarnell: I ended up switching to this, resulting in invocations of fmt::StringPrintf(...). I know this means that you could technically be using fmt::MessageLite but I don't think that's much of an issue. I've left the non-formatting uses of google::protobuf intact.

bdarnell · 2017-12-14T16:39:06Z

Symlinks would solve the problem nicely but they break the build on Windows.

Grumble, I keep forgetting about that. Adding c-dep to the include path is a little ugly, but I don't think it would cause any concrete problems.

Yeah, OpenSSL has a horrible, terrifying API; on the other hand, it's widely used.

Being widely-used is key. It's better to struggle with openssl's awful API than to use a crypto library that hasn't gotten much vetting. Using the highly scientific filter of "have I heard of it before this project?", open/libressl, cryptopp, and libsodium seem like the best bets to me.

They're removing support for CMake in the next release source. The Makefile they ship with the project doesn't use autotools, either, so it's going to be a pain to do our out-of-tree cross-compilation if/when we upgrade.

Yuck. And from that linked issue we probably don't want to use cryptopp with cmake in the current release either. This demonstrates that build issues are perhaps the dominant concern in choosing a crypto library - we need to compile with the right flags to get all the crypto properties right and also to use hardware-accelerated AES when available.

mberhault · 2017-12-14T16:45:17Z

Ok, I'll spend more time looking into alternatives. That's outside of this PR though.

If we do stick with cryptoPP, we can also do the c-deps/cryptopp/cryptopp directory. Also outside this PR.

Release Note: none. Add the `FileKeyManager` which loads active/old keys from files. This will be used for the store keys. A bunch of build/test things here: * finally use CryptoPP. Only linked against CCL code * add testutils for easier testing * change existing tests to testutils * add FormatString

mberhault · 2017-12-14T18:33:37Z

Rebased and squashed. PTAL?

benesch

Build-related stuff all LGTM! Haven't reviewed the actual crypto though.

mberhault · 2017-12-15T15:06:13Z

Ping!

bdarnell · 2017-12-16T01:42:47Z

Reviewed 1 of 7 files at r3, 1 of 2 files at r4, 2 of 3 files at r5, 1 of 2 files at r6, 4 of 4 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

c-deps/libroach/fmt.h, line 19 at r7 (raw file):

Previously, mberhault (marc) wrote…

@bdarnell: I ended up switching to this, resulting in invocations of fmt::StringPrintf(...). I know this means that you could technically be using fmt::MessageLite but I don't think that's much of an issue. I've left the non-formatting uses of google::protobuf intact.

This is fine, but I think you could also be more explicit about what you're bringing in with something like this:

namespace fmt {
    using google::protobuf::StringPrintf;
}

Comments from Reviewable

mberhault · 2017-12-16T01:46:09Z

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

c-deps/libroach/fmt.h, line 19 at r7 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This is fine, but I think you could also be more explicit about what you're bringing in with something like this:
namespace fmt {
    using google::protobuf::StringPrintf;
}

I started that way, but we also have StringAppendV and SStringPrintf throughout the code.

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

Previously, benesch (Nikhil Benesch) wrote…

I'd change the include path to be .. instead of ../cryptopp and then refer to everything as cryptopp/FOO.

Done.

Comments from Reviewable

mberhault requested review from a team, bdarnell and benesch December 13, 2017 03:28

mberhault commented Dec 13, 2017

View reviewed changes

mberhault mentioned this pull request Dec 13, 2017

security: encryption at rest #19783

Closed

29 tasks

mberhault force-pushed the marc/add_key_manager branch 6 times, most recently from cfe86b5 to ce7d6f2 Compare December 13, 2017 16:17

benesch approved these changes Dec 14, 2017

View reviewed changes

mberhault force-pushed the marc/add_key_manager branch from 0356e6b to f24b12a Compare December 14, 2017 01:56

mberhault commented Dec 14, 2017

View reviewed changes

mberhault force-pushed the marc/add_key_manager branch from 8936979 to 5202bdf Compare December 14, 2017 18:19

benesch approved these changes Dec 14, 2017

View reviewed changes

mberhault merged commit 08aa9af into cockroachdb:master Dec 16, 2017

mberhault deleted the marc/add_key_manager branch December 16, 2017 01:46


		#include <google/protobuf/stubs/stringprintf.h>

		namespace fmt = google::protobuf;

libroach: add key manager #20670

libroach: add key manager #20670

Uh oh!

Conversation

mberhault commented Dec 13, 2017

Uh oh!

cockroach-teamcity commented Dec 13, 2017

Uh oh!

mberhault commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdarnell commented Dec 13, 2017

Uh oh!

mberhault commented Dec 13, 2017

Uh oh!

benesch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesch commented Dec 14, 2017 via email

Uh oh!

mberhault commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mberhault commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benesch commented Dec 14, 2017

Uh oh!

mberhault commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdarnell commented Dec 14, 2017

Uh oh!

mberhault commented Dec 14, 2017

Uh oh!

mberhault commented Dec 14, 2017

Uh oh!

benesch left a comment

Choose a reason for hiding this comment

Uh oh!

mberhault commented Dec 15, 2017

Uh oh!

bdarnell commented Dec 16, 2017

Uh oh!

mberhault commented Dec 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mberhault commented Dec 14, 2017 •

edited

Loading

mberhault commented Dec 14, 2017 •

edited

Loading