[encryption] NaCL/secretbox + wrapper around etcd wal + snapshot by cyli · Pull Request #1701 · moby/swarmkit

cyli · 2016-10-27T00:37:56Z

This is some cherry-picked code from #1598.

This vendors the nacl/secretbox package, and also adds a wrapper around the etcd wal and snapshot packages that can make use of the encrypted records and encoder/decoder.

cc @aaronlehmann @aluzzardi @stevvooe @lvh

codecov-io · 2016-10-27T00:46:21Z

Current coverage is 55.95% (diff: 90.72%)

Merging #1701 into master will increase coverage by 0.31%

@@             master      #1701   diff @@
==========================================
  Files            89         93     +4   
  Lines         14631      14782   +151   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           8140       8271   +131   
- Misses         5409       5422    +13   
- Partials       1082       1089     +7

Powered by Codecov. Last update 110ffb7...6917257

lvh · 2016-10-27T14:54:15Z

+	enum Algorithm {
+		NONE = 0 [(gogoproto.enumvalue_customname) = "NotEncrypted"];
+		SECRETBOX_SALSA20_POLY1305 = 1 [(gogoproto.enumvalue_customname) = "NACLSecretboxSalsa20Poly1305"];
+	}


Is NONE for backwards compatibility, or have we stepped away from "encrypted by default, just sometimes with the key on disk"?

The NONE is sometimes used for encrypting the DEK only (it may optionally be encrypted with a KEK)

aluzzardi · 2016-10-27T17:20:58Z

Is the goal to cherry pick #1598 into a few PRs, rather than bulk merging?

cyli · 2016-10-27T17:24:48Z

@aluzzardi To cherry pick it into at least one or two others before rebasing and bulk merging that one, yes. #1598 is really really huge :| It takes about 30 seconds for me to even load the diff in chrome.

aaronlehmann · 2016-10-27T22:02:59Z

+	copy(lengthed[:], key)
+	return NACLSecretbox{
+		key: lengthed,
+	}


How about creating a NACLSecretbox above and copying directly into that? I believe that will save a copy.

aaronlehmann · 2016-10-27T22:04:30Z

+	if record.Algorithm != n.Algorithm() {
+		return nil, fmt.Errorf("not a NACL secretbox record")
+	}
+


Does this need a sanity check that record.Nonce has at least 24 bytes?

aaronlehmann · 2016-10-27T22:10:01Z

+		decoder = NoopCoder
+	}
+	r := api.MaybeEncryptedRecord{}
+	if err := proto.Unmarshal(encoded, &r); err != nil || r.Size() != len(encoded) {


What's the purpose of the r.Size() check? Worried this could cause problems if the generated protobuf code changes slightly.

When I was testing to see if I could detect whether this was an encrypted record (for instance, loading just regular entry data that wasn't encoded as a MaybeEncryptedRecord), it sometimes partially unmarshalled without error into this record. So I just wanted to make sure it was a valid encoding.

r.Size() calculates the size that would be needed to marshal this object. But I don't think protobuf makes any guarantees about only being able to serialize things one way. I'm not comfortable with this check because any change in the way the generated code serializes records could make it impossible to decode previously-saved records. Also, if we ever add a new field to MaybeEncryptedRecord it might cause problems. I think it's better to rely on the error returned by Unmarshal.

aaronlehmann · 2016-10-27T22:23:58Z

+
+	encryptedRecord, err := encoder.Encode(plaintext)
+	if err != nil {
+		return nil, fmt.Errorf("unable to encode data: %s", err.Error())


Use errors.Wrap

aaronlehmann · 2016-10-27T22:24:03Z

+
+	data, err := proto.Marshal(encryptedRecord)
+	if err != nil {
+		return nil, fmt.Errorf("unable to marshal as MaybeEncryptedRecord: %s", err.Error())


Use errors.Wrap

aaronlehmann · 2016-10-27T23:11:57Z

LGTM

diogomonica

LGTM

diogomonica · 2016-10-27T23:46:26Z

+
+// This package defines the interfaces and encryption package
+
+const humanReadablePrefix = "SWARMMKEY-v1-"


should be SWMKEY-1-

diogomonica · 2016-10-27T23:47:28Z

+
+func (n noopCoder) Decode(e api.MaybeEncryptedRecord) ([]byte, error) {
+	if e.Algorithm != n.Algorithm() {
+		return nil, fmt.Errorf("not an unencrypted record")


double negative

diogomonica · 2016-10-27T23:53:34Z

+
+// Encode turns a slice of bytes into a serialized MaybeEncryptedRecord slice of bytes
+func Encode(plaintext []byte, encoder Encoder) ([]byte, error) {
+	if encoder == nil {


Why are we allowing a nil encoder in the first place? Shouldn't we require an encoder explicitly, so lazy people (like me) can never use Encode the wrong way (have to explicitly define they want to use the nopEncoder

That's a good point.

I just error if it's nil now.

It's okay to panic...

diogomonica · 2016-10-27T23:58:02Z

+// using this package
+func GenerateSecretKey() []byte {
+	secretData := make([]byte, naclSecretboxKeySize)
+	if _, err := rand.Read(secretData); err != nil {


This is correct, but not using ReadFull gives me the ebejeebies.

Feel free to leave it like this since it's cleaner. I just really wanted to use the word ebejeebies.

diogomonica · 2016-10-28T00:02:30Z

+
+func TestEncodeDecode(t *testing.T) {
+	// not providing an encoder/decoder will just use the noop encoder/decoder
+	msg := []byte("hello swarmkit")


should be hello again swarmkit

diogomonica · 2016-10-28T00:04:01Z

+// Encode encrypts some bytes and returns an encrypted record
+func (n NACLSecretbox) Encode(data []byte) (*api.MaybeEncryptedRecord, error) {
+	var nonce [24]byte
+	if _, err := io.ReadFull(rand.Reader, nonce[:]); err != nil {


why ReadFull here and Read above?

Tab complete. :D Will fix.

Fixed the previous case

Just to be clear, both cases need to use io.ReadFull.

Yep, both use io.ReadFull now.

diogomonica · 2016-10-28T00:04:21Z

+		return nil, err
+	}
+
+	encrypted := secretbox.Seal(nil, data, &nonce, &n.key)


Add comment on why the first nil?

diogomonica · 2016-10-28T00:04:47Z

+	if record.Algorithm != n.Algorithm() {
+		return nil, fmt.Errorf("not a NACL secretbox record")
+	}
+	if len(record.Nonce) != 24 {


Should this size be a constant throughout the code? NONCE_SIZE

diogomonica · 2016-10-28T00:05:24Z

+	require.NoError(t, err)
+
+	coder := NewNACLSecretbox(key)
+	data := []byte("Hello world")


should be Hello again world

stevvooe · 2016-10-28T01:03:40Z

+const humanReadablePrefix = "SWMKEY-1-"
+
+// ErrCannotDecode is the type of error returned when some data cannot be decoded as plaintext
+type ErrCannotDecode struct {


I'm not sure if we should introduce this error type if we aren't switching on it. Just use errors.Errorf.

I do in the other PR

I can just introduce it there, though, if that helps.

This looks reasonable: https://github.com/docker/swarmkit/pull/1598/files#diff-8a1870ff8ffb551be79bf7069e478962R171.

Make sure you wrap in errors.Cause, in case we wrap the error.

Make sure you wrap in errors.Cause, in case we wrap the error.

At the point you detect the error. 🐹

Yep, will do that in #1598 (just checking that you mean that we ok leaving the error type declaration here in this PR)

stevvooe · 2016-10-28T01:17:49Z

+}
+
+// Encode encrypts some bytes and returns an encrypted record
+func (n NACLSecretbox) Encode(data []byte) (*api.MaybeEncryptedRecord, error) {


Since we aren't working with arbitrary types, I would recommend calling these Encrypt/Decrypt.

Sounds good. And the corresponding interfaces would then be Encrypter/Decrypter?

Yea. I think this will help differentiate from Marshal/Unmarshal/Decode/Encode. 🐶

Signed-off-by: cyli <ying.li@docker.com>

…ecretbox encoder/decoder. Signed-off-by: cyli <ying.li@docker.com>

…nd decodes. Signed-off-by: cyli <ying.li@docker.com>

aluzzardi

LGTM

Signed-off-by: cyli <ying.li@docker.com>

GordonTheTurtle added the status/0-triage label Oct 27, 2016

cyli force-pushed the manager-encryption-package branch 2 times, most recently from 44557b3 to 69ea3f3 Compare October 27, 2016 03:55

cyli mentioned this pull request Oct 27, 2016

At rest encryption: TLS keys and wrappers around the etcd wal/snap packages #1598

Merged

5 tasks

cyli force-pushed the manager-encryption-package branch 2 times, most recently from 908352f to 928ba99 Compare October 27, 2016 04:37

lvh reviewed Oct 27, 2016

View reviewed changes

aaronlehmann reviewed Oct 27, 2016

View reviewed changes

cyli force-pushed the manager-encryption-package branch 3 times, most recently from ec45097 to acf8cbe Compare October 27, 2016 23:05

diogomonica approved these changes Oct 28, 2016

View reviewed changes

cyli force-pushed the manager-encryption-package branch 2 times, most recently from 9d89f91 to 471523a Compare October 28, 2016 00:40

stevvooe reviewed Oct 28, 2016

View reviewed changes

cyli force-pushed the manager-encryption-package branch 2 times, most recently from 697a978 to 4316be9 Compare October 28, 2016 02:04

cyli added 3 commits October 27, 2016 19:07

Vendor nacl/secretbox

2df311b

Signed-off-by: cyli <ying.li@docker.com>

Implement an encryption encoder/decoder, and provide a default nacl/s…

ec53416

…ecretbox encoder/decoder. Signed-off-by: cyli <ying.li@docker.com>

Implement encryption wrapper around WALs and snapshots that encodes a…

3e2cebe

…nd decodes. Signed-off-by: cyli <ying.li@docker.com>

aluzzardi approved these changes Oct 28, 2016

View reviewed changes

Rename encoder->encrypter and decoder->decrypter, etc.

6917257

Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the manager-encryption-package branch from 4316be9 to 6917257 Compare October 28, 2016 02:07

diogomonica merged commit 137c3d1 into moby:master Oct 28, 2016

cyli deleted the manager-encryption-package branch October 28, 2016 02:30

thaJeztah mentioned this pull request Nov 19, 2022

manager/state/raft/storage: fix TestCreateOpenInvalidDirFails #3087

Merged


		// This package defines the interfaces and encryption package

		const humanReadablePrefix = "SWARMMKEY-v1-"

Conversation

cyli commented Oct 27, 2016

Uh oh!

codecov-io commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 55.95% (diff: 90.72%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aluzzardi commented Oct 27, 2016

Uh oh!

cyli commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

cyli Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aaronlehmann commented Oct 27, 2016

Uh oh!

diogomonica 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!

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!

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

codecov-io commented Oct 27, 2016 •

edited

Loading

cyli commented Oct 27, 2016 •

edited

Loading

cyli Oct 27, 2016 •

edited

Loading