storage: pass encryption options to libroachccl. by mberhault · Pull Request #20570 · cockroachdb/cockroach

mberhault · 2017-12-07T22:25:12Z

Release Note: none.

CCL-only per-store encryption options are serialized and passed to
libroach through DBOptions.

Upon DBOpen, a hook is run extracting the options in CCL code, doing
nothing in OSS code.
This will eventually initialize encryption-related objects.

A few other things:

hide encryption flag, it's not ready for use
generate pb.{cc,h} for CCL protobuf and store in separate path
print out encryption flags and error out when passed

cockroach-teamcity · 2017-12-07T22:25:17Z

This change is

mberhault · 2017-12-07T22:26:14Z

Makefile

 endif

 CPP_PROTO_ROOT := $(LIBROACH_SRC_DIR)/protos
+CPP_PROTO_CCL_ROOT := $(LIBROACH_SRC_DIR)/protosccl


Should this be $(LIBROACH_SRC_DIR)/ccl/protos instead? There's no reason to put it in the base directory.

either way it kind of stutters. We can have protosccl/ccl/... or ccl/protos/ccl/.... I'm not sure I care enough about generated files though.

mberhault · 2017-12-07T22:27:30Z

pkg/ccl/storageccl/engineccl/rocksdb.go

+}
+
+func setupCCLHooks() engine.DBHooks {
+	return engine.DBHooks(unsafe.Pointer(C.GetCCLHooks()))


eep! I'm still playing with hooks. I'd rather avoid unsafe if I can.

mberhault · 2017-12-08T13:31:27Z

Makefile

 CPP_HEADERS := $(subst $(PKG_ROOT),$(CPP_PROTO_ROOT),$(CPP_PROTOS:%.proto=%.pb.h))
 CPP_SOURCES := $(subst $(PKG_ROOT),$(CPP_PROTO_ROOT),$(CPP_PROTOS:%.proto=%.pb.cc))

+CPP_PROTOS_CCL := $(filter %ccl/baseccl/encryption_options.proto,$(GO_PROTOS))


I think we may want to split CCL protos out of GO_PROTOS, otherwise we're building with them even in OSS code.

mberhault · 2017-12-08T20:39:05Z

Removed the stdout logging for all but the "found extra options" when encryption flags are passed.
Made the flags hidden and DBOpen fails when present with "encryption not implemented" (after printing the parsed values).

mberhault · 2017-12-08T22:34:11Z

This is ready for review.

bdarnell · 2017-12-10T18:54:01Z

Reviewed 10 of 17 files at r1, 1 of 6 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

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

// OpenHook does nothing in OSS mode.
__attribute__((weak)) DBStatus OpenHook(const DBOptions opts) { return kSuccess; }

s/OpenHook/DBOpenHook/? (or dbOpenHook to match the lowerCase names below)

Should this fail if extra_options is non-empty?

pkg/ccl/baseccl/encryption_options.proto, line 33 at r4 (raw file):

// EncryptionOptions defines the per-store encryption options.
message EncryptionOptions {

Unless EncryptionOptions are the only CCL options we ever have that need this treatment, the use of a field named ExtraOptions to pass EncryptionOptions is awkward. I would put a more generic ExtraOptions protobuf around EncryptionOptions to give us space for other CCL options in the future.

Comments from Reviewable

mberhault · 2017-12-10T19:04:52Z

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

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

Previously, bdarnell (Ben Darnell) wrote…

s/OpenHook/DBOpenHook/? (or dbOpenHook to match the lowerCase names below)

Should this fail if extra_options is non-empty?

Renamed to DBOpenHook to match the DBOpen function.

I prefer to have the OSS code be blissfully ignorant of extras_options. In the encryption case, we have plenty of safeguards already (encryption flag doesn't exist, registry will have unknown file type).

pkg/ccl/baseccl/encryption_options.proto, line 33 at r4 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Unless EncryptionOptions are the only CCL options we ever have that need this treatment, the use of a field named ExtraOptions to pass EncryptionOptions is awkward. I would put a more generic ExtraOptions protobuf around EncryptionOptions to give us space for other CCL options in the future.

I started out that way but adding a whole extra message we don't need seemed premature.
Since this is only used at runtime to communicate between go/cgo and not persisted or used across processes, we can add a new message that contains the field EncryptionOptions encryption_options anytime we like.

Comments from Reviewable

Release Note: none. CCL-only per-store encryption options are serialized and passed to libroach through DBOptions. Upon DBOpen, a hook is run extracting the options in CCL code, doing nothing in OSS code. This will eventually initialize encryption-related objects. A few other things: * hide encryption flag, it's not ready for use * generate pb.{cc,h} for CCL protobuf and store in separate path * print out encryption flags and error out when passed

mberhault · 2017-12-10T19:37:40Z

Review status: 10 of 16 files reviewed at latest revision, 5 unresolved discussions.

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

Previously, mberhault (marc) wrote…

Renamed to DBOpenHook to match the DBOpen function.

I prefer to have the OSS code be blissfully ignorant of extras_options. In the encryption case, we have plenty of safeguards already (encryption flag doesn't exist, registry will have unknown file type).

Nevermind, erroring out just in case someone tries to pass them through. You're right, the extra check doesn't hurt.
I'll add tests for this once #20594 lands.

Comments from Reviewable

bdarnell · 2017-12-11T02:22:20Z

Review status: 10 of 16 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

pkg/ccl/baseccl/encryption_options.proto, line 33 at r4 (raw file):

Previously, mberhault (marc) wrote…

I started out that way but adding a whole extra message we don't need seemed premature.
Since this is only used at runtime to communicate between go/cgo and not persisted or used across processes, we can add a new message that contains the field EncryptionOptions encryption_options anytime we like.

Ack, I forgot that this was only used in-process so it can be changed freely.

Comments from Reviewable

mberhault requested review from a team December 7, 2017 22:25

mberhault requested review from bdarnell and benesch December 7, 2017 22:25

mberhault commented Dec 7, 2017

View reviewed changes

mberhault force-pushed the marc/try_libroachccl_hooks branch 2 times, most recently from 3db97d5 to e43e2f3 Compare December 7, 2017 23:16

mberhault mentioned this pull request Dec 7, 2017

security: encryption at rest #19783

Closed

29 tasks

mberhault commented Dec 8, 2017

View reviewed changes

mberhault force-pushed the marc/try_libroachccl_hooks branch from 3e28564 to c058d92 Compare December 8, 2017 13:34

mberhault force-pushed the marc/try_libroachccl_hooks branch from 02618e6 to a3ca866 Compare December 8, 2017 22:32

mberhault changed the title ~~WIP: storage: pass encryption options to libroach ccl.~~ storage: pass encryption options to libroach ccl. Dec 8, 2017

mberhault changed the title ~~storage: pass encryption options to libroach ccl.~~ storage: pass encryption options to libroachccl. Dec 8, 2017

mberhault force-pushed the marc/try_libroachccl_hooks branch from a3ca866 to b648372 Compare December 8, 2017 22:40

mberhault force-pushed the marc/try_libroachccl_hooks branch from b648372 to c792b1d Compare December 10, 2017 19:05

mberhault force-pushed the marc/try_libroachccl_hooks branch from c792b1d to a077cb9 Compare December 10, 2017 19:36

mberhault merged commit d1343a2 into cockroachdb:master Dec 11, 2017

mberhault deleted the marc/try_libroachccl_hooks branch December 11, 2017 02:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: pass encryption options to libroachccl.#20570

storage: pass encryption options to libroachccl.#20570
mberhault merged 1 commit intocockroachdb:masterfrom
mberhault:marc/try_libroachccl_hooks

mberhault commented Dec 7, 2017 •

edited

Loading

Uh oh!

cockroach-teamcity commented Dec 7, 2017

Uh oh!

mberhault Dec 7, 2017

Uh oh!

mberhault Dec 8, 2017

Uh oh!

mberhault Dec 7, 2017

Uh oh!

mberhault Dec 8, 2017

Uh oh!

mberhault commented Dec 8, 2017

Uh oh!

mberhault commented Dec 8, 2017

Uh oh!

bdarnell commented Dec 10, 2017

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

bdarnell commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Uh oh!

cockroach-teamcity commented Dec 7, 2017

Uh oh!

mberhault Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

mberhault Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

mberhault Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

mberhault Dec 8, 2017

Choose a reason for hiding this comment

Uh oh!

mberhault commented Dec 8, 2017

Uh oh!

mberhault commented Dec 8, 2017

Uh oh!

bdarnell commented Dec 10, 2017

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

mberhault commented Dec 10, 2017

Uh oh!

bdarnell commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mberhault commented Dec 7, 2017 •

edited

Loading