Add a secretbox and AES-CBC path for encrypt at rest #46916

smarterclayton · 2017-06-03T21:56:29Z

Add a secretbox and AES-CBC encrypt at rest provider and alter the config, based on feedback from security review. AES-CBC is more well reviewed and generally fits better with common criteria and FIPS, secretbox is newer and faster than CBC.

Add secretbox and AES-CBC encryption modes to at rest encryption.  AES-CBC is considered superior to AES-GCM because it is resistant to nonce-reuse attacks, and secretbox uses Poly1305 and XSalsa20.

k8s-github-robot · 2017-06-03T21:56:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

smarterclayton · 2017-06-03T21:59:38Z

We may decide to go with this to avoid nonce collisions and reduce the need to rotate.

smarterclayton · 2017-06-04T03:44:38Z

Added a CBC mode for comparison:

go test ./vendor/k8s.io/apiserver/pkg/storage/value/encrypt/aes/ -bench=Bench* -test.run=XXX
BenchmarkGCMRead_16_1024-8          	 2000000	       791 ns/op
BenchmarkGCMRead_32_1024-8          	 2000000	       862 ns/op
BenchmarkGCMRead_32_16384-8         	  200000	      8199 ns/op
BenchmarkGCMRead_32_16384_Stale-8   	  200000	      8236 ns/op
BenchmarkGCMWrite_16_1024-8         	 1000000	      2156 ns/op
BenchmarkGCMWrite_32_1024-8         	 1000000	      2253 ns/op
BenchmarkGCMWrite_32_16384-8        	  100000	     14217 ns/op
BenchmarkCBCRead_32_1024-8          	  500000	      2902 ns/op
BenchmarkCBCRead_32_16384-8         	   30000	     42035 ns/op
BenchmarkCBCRead_32_16384_Stale-8   	   30000	     41715 ns/op
BenchmarkCBCWrite_32_1024-8         	  300000	      4766 ns/op
BenchmarkCBCWrite_32_16384-8        	   30000	     53546 ns/op
BenchmarkSecretboxRead_32_1024-8         1000000              2032 ns/op
BenchmarkSecretboxRead_32_16384-8         100000             22749 ns/op
BenchmarkSecretboxRead_32_16384_Stale-8   100000             22696 ns/op
BenchmarkSecretboxWrite_32_1024-8         300000              4132 ns/op
BenchmarkSecretboxWrite_32_16384-8         50000             30591 ns/op

sakshamsharma · 2017-06-05T23:59:01Z

@smarterclayton In context of your comment on #46460 (which is merged now)

We'll need to update this based on #46916

I believe we simply need another configuration option for CBC? Is a small change, but perhaps we have to wait till next month for it (and the above changes) to go in now?

smarterclayton · 2017-06-06T00:05:00Z

I think given the concerns raised we want config for both cbc and secretbox ready - I rate the input high enough that I'd consider saying that we need these in order to ship.

…

On Mon, Jun 5, 2017 at 7:59 PM, Saksham Sharma ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> In context of your comment on #46460 <#46460> (which is merged now) We'll need to update this based on #46916 <#46916> I believe we simply need another configuration option for CBC? Is a small change, but perhaps we have to wait till next month for it (and the above changes) to go in now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46916 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p1VFfBLqI__rEQ26waKlPPehyPyeks5sBJZOgaJpZM4NvNkY> .

destijl · 2017-06-06T03:52:33Z

@smarterclayton this seems like a good idea to me. I can update the extension thread with a request to get this in, plus the config change to allow people to use it. Are you writing the config change too?

smarterclayton · 2017-06-06T14:15:23Z

I need to sync with @sakshamsharma on that.

smarterclayton · 2017-06-07T14:49:11Z

Yeah @simo5 signed off on the encryption stuff so we're only reviewing config.

Add tests for new transformers

sakshamsharma · 2017-06-07T16:27:33Z

Maybe we can replace the Boolean with an enumerated variable? We can easily add more underlying encryption algorithms later, which have the same parsing logic.

It may help reduce a lot of code reuse.

smarterclayton · 2017-06-07T21:28:55Z

@destijl did we get release approval for the exception?

destijl · 2017-06-07T22:02:35Z

I asked dawn on the thread I started for the exemption, I believe so.

sakshamsharma · 2017-06-07T22:05:54Z

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

-	aesTransformerPrefixV1 = "k8s:enc:aes:v1:"
+	aesCBCTransformerPrefixV1    = "k8s:enc:aescbc:v1:"
+	aesGCMTransformerPrefixV1    = "k8s:enc:aesgcm:v1:"
+	secretboxTransformerPrefixV1 = "k8s:enc:secretbox:v1"
 )


@smarterclayton

Context:

sakshamsharma 23 hours ago
I would ideally think that we need shorter prefixes. Like: gcm, cbc, secb.
smarterclayton 9 hours ago
We will almost certainly need to version them in the future, and we want to ensure we have enough key space to evolve if we have to.

Versioned prefixes are fine, what I mean is, we could do k8s:enc:secb:v1 etc, because these prefixes are not user/developer/admin facing. Of course, the premise is that these long prefixes are a storage overhead. If this premise is not significant, we can ignore this.

smarterclayton · 2017-06-07T23:19:13Z

I think the prefixes in the at rest store are as terse as we can make them to preserve future expansion

dchen1107 · 2017-06-08T00:29:49Z

The release team's decision on the exception request of encrypt secrets in etcd which includes this pr is yes. Thanks!

cc/ @kubernetes/kubernetes-release-managers

smarterclayton · 2017-06-08T00:48:38Z

Applying labels based on two part review

k8s-github-robot · 2017-06-08T02:01:53Z

Automatic merge from submit-queue (batch tested with PRs 46979, 47078, 47138, 46916)

@smarterclayton

Automatic merge from submit-queue (batch tested with PRs 47510, 47516, 47482, 47521, 47537) Fix typo in secretbox transformer prefix Introduced by #46916 via cherry picked commit [here](sakshamsharma@12bb591). Urgent fix in my opinion, ideally should be merged before production. @smarterclayton

@destijl

Automatic merge from submit-queue Add encryption provider support via environment variables These changes are needed to allow cloud providers to use the encryption providers as an alpha feature. The version checks can be done in the respective cloud providers'. Context: #46460 and #46916 @destijl @jcbsmpsn @smarterclayton

php-coder · 2017-06-19T16:46:55Z

staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/secretbox/secretbox_test.go

+
+func TestSecretboxKeyRotation(t *testing.T) {
+	testErr := fmt.Errorf("test error")
+	context := value.DefaultContext([]byte("authenticated_data"))


Why we're passing different values as a context here and bellow while secretboxTransformer doesn't use the context at all? Why not pass nil instead?

php-coder · 2017-06-19T16:56:17Z

staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/secretbox/secretbox_test.go

+	if stale {
+		p = value.NewPrefixTransformers(nil,
+			value.PrefixTransformer{Prefix: []byte("first:"), Transformer: NewSecretboxTransformer(key1)},
+			value.PrefixTransformer{Prefix: []byte("second:"), Transformer: NewSecretboxTransformer(key2)},


Order the same: looks like code or comment is wrong.

php-coder · 2017-06-19T16:58:50Z

staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/aes/aes_test.go

+	if stale {
+		p = value.NewPrefixTransformers(nil,
+			value.PrefixTransformer{Prefix: []byte("first:"), Transformer: NewCBCTransformer(block1)},
+			value.PrefixTransformer{Prefix: []byte("second:"), Transformer: NewCBCTransformer(block2)},


The same here: the order hasn't been changed.

php-coder · 2017-06-22T16:08:41Z

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

+			return result, fmt.Errorf("could not obtain secret for named key %s: %s", keyData.Name, err)
+		}
+
+		if len(key) != 32 {


Is there are reason why we're validating key length only for Secretbox?

Looks we can do the same for AES-GCM (16,24,32) and AES-CBC (32).

@smarterclayton Let me know if you agree and I'll prepare PR for that.

We're validating it only for secretbox because it requires a fixed size array as input. AES-GCM returns a sane error itself, if the key size is not valid. For secretbox, if we did not validate and convert it to a [32]byte safely, it may throw a runtime error. We need to throw a sane error, and AES-GCM already does that. I'm not sure about CBC though.

php-coder · 2017-06-22T16:11:18Z

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

+		}
+
+		keyArray := [32]byte{}
+		copy(keyArray[:], key)


@smarterclayton Copying the key looks useless. Why we can't use DecodeString() result as-is?

The intention of copying was to convert a byte slice to a [32]byte, and, however ugly, this came up as the best way to do it when I verified it via some quick google search 😄

Thank you for the explanation!

@smarterclayton

Automatic merge from submit-queue (batch tested with PRs 49850, 47782, 50595, 50730, 51341) Fix benchmarks to really test reverse order of the keys **What this PR does / why we need it**: This PR modifies the code to do what comments says -- reverse the order of keys. It also fixes the logic that was wrong and didn't allow stale data. **Special notes for your reviewer**: This change resolves the following review comments: - #41939 (comment) - #46916 (comment) - #46916 (comment) **Release note**: ```release-note NONE ``` PTAL @smarterclayton

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2017

k8s-github-robot assigned timothysc and lavalamp Jun 3, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 3, 2017

smarterclayton added this to the v1.7 milestone Jun 3, 2017

smarterclayton mentioned this pull request Jun 3, 2017

Encryption of secrets at the database layer kubernetes/community#607

Merged

smarterclayton assigned jcbsmpsn and destijl and unassigned timothysc and jcbsmpsn Jun 3, 2017

smarterclayton force-pushed the secretbox branch from 0cc9445 to f6d2579 Compare June 3, 2017 22:13

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 3, 2017

smarterclayton force-pushed the secretbox branch 2 times, most recently from 27094db to 21c67d7 Compare June 4, 2017 03:44

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

smarterclayton mentioned this pull request Jun 5, 2017

Add configuration for encryption providers #46460

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2017

smarterclayton force-pushed the secretbox branch from 21c67d7 to a50cd3a Compare June 6, 2017 01:57

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2017

bump(golang.org/x/crypto/nacl):d172538b2cfce0c13cee31e647d0367aa8cd2486

868cdec

smarterclayton force-pushed the secretbox branch from dfa0f46 to 1dfb0d0 Compare June 7, 2017 15:11

Add configuration for AESCBC, Secretbox encryption

1307340

Add tests for new transformers

smarterclayton force-pushed the secretbox branch from 1dfb0d0 to 1307340 Compare June 7, 2017 15:11

smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 7, 2017

smarterclayton changed the title ~~Add a secretbox implementation for encryption~~ Add a secretbox and AES-CBC path for encrypt at rest Jun 7, 2017

sakshamsharma reviewed Jun 7, 2017

View reviewed changes

smarterclayton added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 8, 2017

k8s-github-robot merged commit d16d64f into kubernetes:master Jun 8, 2017

This was referenced Jun 14, 2017

Add encryption provider support via environment variables #47481

Merged

Fix typo in secretbox transformer prefix #47537

Merged

php-coder reviewed Jun 19, 2017

View reviewed changes

php-coder mentioned this pull request Jun 20, 2017

Fix benchmarks to really test reverse order of the keys #47782

Merged

php-coder reviewed Jun 22, 2017

View reviewed changes

lavalamp removed their assignment Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a secretbox and AES-CBC path for encrypt at rest #46916

Add a secretbox and AES-CBC path for encrypt at rest #46916

smarterclayton commented Jun 3, 2017 •

edited

Loading

k8s-github-robot commented Jun 3, 2017

smarterclayton commented Jun 3, 2017

smarterclayton commented Jun 4, 2017 •

edited

Loading

sakshamsharma commented Jun 5, 2017

smarterclayton commented Jun 6, 2017 via email

destijl commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

smarterclayton commented Jun 7, 2017

sakshamsharma commented Jun 7, 2017 •

edited

Loading

smarterclayton commented Jun 7, 2017

destijl commented Jun 7, 2017

sakshamsharma Jun 7, 2017 •

edited

Loading

smarterclayton commented Jun 7, 2017 via email

dchen1107 commented Jun 8, 2017 •

edited

Loading

smarterclayton commented Jun 8, 2017

k8s-github-robot commented Jun 8, 2017

php-coder Jun 19, 2017

php-coder Jun 19, 2017

php-coder Jun 19, 2017

php-coder Jun 22, 2017

sakshamsharma Jun 23, 2017

php-coder Jun 22, 2017

sakshamsharma Jun 23, 2017

php-coder Jun 23, 2017

Add a secretbox and AES-CBC path for encrypt at rest #46916

Add a secretbox and AES-CBC path for encrypt at rest #46916

Conversation

smarterclayton commented Jun 3, 2017 • edited Loading

k8s-github-robot commented Jun 3, 2017

smarterclayton commented Jun 3, 2017

smarterclayton commented Jun 4, 2017 • edited Loading

sakshamsharma commented Jun 5, 2017

smarterclayton commented Jun 6, 2017 via email

destijl commented Jun 6, 2017

smarterclayton commented Jun 6, 2017

smarterclayton commented Jun 7, 2017

sakshamsharma commented Jun 7, 2017 • edited Loading

smarterclayton commented Jun 7, 2017

destijl commented Jun 7, 2017

sakshamsharma Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

smarterclayton commented Jun 7, 2017 via email

dchen1107 commented Jun 8, 2017 • edited Loading

smarterclayton commented Jun 8, 2017

k8s-github-robot commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jun 3, 2017 •

edited

Loading

smarterclayton commented Jun 4, 2017 •

edited

Loading

sakshamsharma commented Jun 7, 2017 •

edited

Loading

sakshamsharma Jun 7, 2017 •

edited

Loading

dchen1107 commented Jun 8, 2017 •

edited

Loading