core: add preamble format #20124

mberhault · 2017-11-17T19:07:51Z

Encryption at rest: part 1 of N.

Add preamble format for on-disk rocksdb instances:

new field in the --store flag: format=preamble (empty defaults to classic)
new on-disk version and added format field
proto definition of preamble
plaintext (no-op) block cipher stream

The on-disk version gets set to 2 iff we use the preamble format. This allows:

old binaries to keep reading classic format files created using new binaries
old binaries to fail with version check on preamble formats
new binaries to handle preamble/classic properly

Eventually, we may want to always write version 2. We may need to give it some time to give enough people to upgrade their binaries.

A few todos to resolve before merging this:

testing at the C++ level
is my message length correct? protos use little-endian, it's a little silly to write the length in network byte order
better error codes would be good. No built-in errorf in rocksdb::Status

Encryption-at-rest work: part 1. This adds support for per-store "preamble" format. Specified through the store flag with `format=preamble`. The default value is `classic`. Setting the preamble format causes a higher `COCKROACHDB_VERSION` value as well as the `format=preamble` field to be set. For now, stores created without `format=preamble` will not increase the `COCKROACHDB_VERSION` value to allow downgrade. This allows: * old binaries to fail if preamble is enabled. * new binaries to understand the new version but fail if not specified on an existing preamble-formatted store * stores in classic format can progress to future versions The preamble format is currently plaintext only, encryption to be added in a future PR.

cockroach-teamcity · 2017-11-17T19:07:58Z

This change is

mberhault · 2017-11-17T19:09:35Z

c-deps/libroach/preamble.h

+
+// PlaintextCipherStream implements BlockAccessCipherStream with
+// no-op encrypt/decrypt operations.
+class PlaintextCipherStream final : public rocksdb::BlockAccessCipherStream {


I forgot how this works. How do I get a concrete class without overriding the pure protected methods?

Er, isn't the point that you have to override the pure protected methods to get a concrete class?

yeah, I know. I suppose my complaint is about those protected methods being required.

Yeah, the encryption support in RocksDB that landed seems pretty specialized for the use case it was extracted from.

mberhault · 2017-11-17T19:10:02Z

pkg/cli/flags.go

 	// special severity value DEFAULT instead.
 	pf.Lookup(logflags.LogToStderrName).NoOptDefVal = log.Severity_DEFAULT.String()

-	// Security flags.


Just saw this in passing, that comment has nothing to do here.

bdarnell · 2017-11-18T16:25:29Z

LGTM, aside from doubts about whether we should be using this EncryptedEnv vs building our own.

Reviewed 8 of 10 files at r1, 6 of 7 files at r2, 2 of 4 files at r3, 1 of 1 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

c-deps/libroach/db.cc, line 1705 at r5 (raw file):

  PreambleHandler* preamble = nullptr;
  if (db_opts.use_preamble) {
    // The caller makes sure we're not an in-memory DB.

What's wrong with using the preamble format in memory?

c-deps/libroach/preamble.cc, line 40 at r5 (raw file):

  // Determine the serialized length and size of the length prefix.
  // TODO(mberhault): protobuf encoding is little-endian, so this is a little weird.

Weird in that you're writing this big-endian while protobuf uses little-endian? I don't think that's too bad; I'd rather use big-endian here and not even know what protobuf uses internally.

c-deps/libroach/preamble.cc, line 42 at r5 (raw file):

  // TODO(mberhault): protobuf encoding is little-endian, so this is a little weird.
  assert(byte_size < UINT16_MAX);
  uint16_t message_size = htons(byte_size);

s/message_size/encoded_size/g

c-deps/libroach/preamble.cc, line 46 at r5 (raw file):

  if ((byte_size + num_length_bytes) > prefixLength ) {
    // TODO(mberhault): is NoSpace the right thing? Nothing really fits.

I'd go with Corruption, same as below.

c-deps/libroach/preamble.h, line 24 at r5 (raw file):

// WARNING: changing this will result in incompatible on-disk format.
// The preamble length must fit in a uint16_t.
const static size_t defaultPreambleLength = 4096;

s/default/k/

This is also required to be a multiple of the page size, right? Mention that in the comment.

Unless you need them for tests, both constants in this header probably belong in the .cc file instead.

c-deps/libroach/preamble.h, line 48 at r5 (raw file):

// Blocksize for the plaintext cipher stream.
// TODO(mberhault): we can pick anything we like. What's good?
const static size_t plaintextBlockSize = 4096;

It looks like every write gets padded to a multiple of the block size, so setting it too high would waste space (but setting it too low wastes CPU to iterate over the data to do nothing in small chunks).

I'd set this to 16 for now to match the block size of the AES cipher version. In the long run, we should probably refactor the EncryptionProvider interface so it can return something other than a BlockAccessCipherStream that can be an efficient no-op.

c-deps/libroach/preamble.h, line 76 at r5 (raw file):

  // Length of data is equal to BlockSize();
  virtual rocksdb::Status EncryptBlock(uint64_t blockIndex, char *data, char* scratch) override {
		return rocksdb::Status::OK();

Don't mix tabs and spaces. We use two-space indents in our c++ code.

pkg/cli/cliflags/flags.go, line 448 at r5 (raw file):

</PRE>
The "format" field specifies the underlying file format for the store.
Allowed values are "classic" (default if omitted), or "preamble". Preamble

One day the preamble format will be the default and then the "classic" behind whatever comes next. Let's just give these numbers instead of names.

pkg/storage/engine/version.go, line 26 at r5 (raw file):

type storageVersion int
type formatVersion int

This two-level versioning scheme is confusing and needs more documentation. Or maybe two version numbers is the wrong approach - is the "format" a sequence of versions or is it a set of features that may or may not be present?

It's really unfortunate that EncryptedEnv doesn't give us any feasible migration path so we'll have to keep supporting the old format forever. It ought to be possible to make an Env that lets us encode the format somehow (maybe in the filename?) instead of an all-or-nothing cutover. Then we could treat this like a normal version upgrade and eventually get all files written with the preamble format.

pkg/storage/engine/version_test.go, line 84 at r5 (raw file):

// TestOldVersionFormat verifies that new version format (not numbers, but actual json fields)
// parses properly when using the older version data structures.

What do you mean? Just that the new Format field is ignored by old code?

Comments from Reviewable

mberhault · 2017-11-18T17:08:59Z

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

c-deps/libroach/db.cc, line 1705 at r5 (raw file):