Adding Data Encryption Key (DEK) Key Encryption Key (KEK) integration… #60236

immutableT · 2018-02-22T18:30:38Z

… tests via KMS Plugin Mock.

What this PR does / why we need it:
Adding integration tests between KubeAPI and KMS Plugin.
Concretely, this test verifies the following integration contracts:

Raw records in ETCD that were processed by KMS Provider should be prefixed with k8s:enc:kms:v1:grpc-kms-provider-name:
Data Encryption Key (DEK) should be generated by envelopeTransformer and passed to KMS gRPC Plugin
KMS gRPC Plugin should encrypt the DEK with a Key Encryption Key (KEK) and pass it back to envelopeTransformer
The payload (ex. Secret) should be encrypted via AES CBC transform
Prefix-EncryptedDEK-EncryptedPayload structure should be deposited to ETCD

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

immutableT · 2018-02-22T18:32:10Z

/assign @mikedanese

mikedanese · 2018-02-23T03:58:50Z

test/integration/master/kms_plugin_mock.go

+
+	// Allow users of the plugin to sense requests that were passed to KMS.
+	encryptRequest chan *kmsapi.EncryptRequest
+	decryptRequest chan *kmsapi.DecryptRequest


why are you using channels as opposed to a slice?

It was suggested to me in the past that channels are preferred to direct memory access when it comes to communication between GO routines.

Of course, a slice or even kmsapi.EncryptRequest would work just fine here.

mikedanese · 2018-02-23T04:02:26Z

test/integration/master/kms_transformation_test.go

+
+func (r rawDEKKEKSecret) getDEKLen() int {
+	// DEK's length is stored in the two bytes that follow the prefix.
+	return int(binary.BigEndian.Uint16(r[len(kmsPrefix) : len(kmsPrefix)+dekKeySizeLen]))


fyi: cryptobyte is a library for reading length prefixed data. we don't have to pull it in just for this though.

https://godoc.org/golang.org/x/crypto/cryptobyte

Thank you.
It would seem that envelop.go would benefit from cryptobyte as well.

mikedanese · 2018-02-23T04:03:40Z

test/integration/master/kms_plugin_mock.go

+	decryptRequest chan *kmsapi.DecryptRequest
+}
+
+func NewBase64Plugin() (*base64Plugin, error) {


this seems like it should be a private function.

I plan to suggest the use of this mock in grpc_service_unix_test.go.

mikedanese · 2018-02-23T04:05:30Z

test/integration/master/kms_transformation_test.go

+		t.Fatalf("failed to create mock of KMS Plugin: %v", err)
+	}
+	defer pluginMock.cleanUp()
+	go pluginMock.grpcServer.Serve(pluginMock.listener)


consider wrapping this in a Fatalf()

go t.Fatalf("err serving: %v", pluginMock.grpcServer.Serve(pluginMock.listener))

I believe this will block execution of the test on that line.
GO routine would need to wait for the evaluation arguments passed to t.Fatalf - waiting for Server func to exit.

mikedanese · 2018-02-23T04:06:47Z

test/integration/master/kms_transformation_test.go

+	defer test.cleanUp()
+
+	secretETCDPath := test.getETCDPath()
+	var rawSecretAsSeenByETCD rawDEKKEKSecret


why are you forward declaring this as opposed to just using := on the next line?

If I don't forward declare it as rawDEKKEKSecret, it will become []byte on the next line, and I would have to cast it to rawDEKKEKSecret later.
Would love to know if there is a more elegant way of doing this.

mikedanese · 2018-02-24T00:39:33Z

/ok-to-test
/lgtm
/approve

fejta-bot · 2018-02-24T03:37:50Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.