Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

engineccl: Fix data key generation for v2 encryption #121111

Merged
merged 3 commits into from Mar 28, 2024

Conversation

bdarnell
Copy link
Member

Prior tests of v2 encryption were too low-level and did not cover the data key generation. The new roachtest provides end-to-end coverage.

This fixes a bug in not-yet-released functionality

Epic: none
Release note: none

@bdarnell bdarnell requested review from a team as code owners March 26, 2024 10:31
@bdarnell bdarnell requested review from srosenberg, DarrylWong and jbowens and removed request for a team March 26, 2024 10:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell, @DarrylWong, @jbowens, and @srosenberg)


pkg/cmd/roachtest/tests/encryption.go line 38 at r1 (raw file):

		httpClient := roachtestutil.DefaultHTTPClient(c, t.L())

		// Starting with two copies of "plain" lets us avoid special-casing the first key in our rotation test.

I don't get this.. Looks like the first loop iteration is doing a plain to plain rotation.. do we want that?


pkg/cmd/roachtest/tests/encryption.go line 40 at r1 (raw file):

		// Starting with two copies of "plain" lets us avoid special-casing the first key in our rotation test.
		keys := []string{"plain", "plain"}
		// Rotate through multiple keys with different key sizes and both implementation versions.

[nit] Comments usually wrap at 80


pkg/ccl/storageccl/engineccl/testdata/data_key_manager line 254 at r1 (raw file):

get-active-data-key
----
encryption_type:AES_128_CTR_V2 creation_time:26 source:"data key manager" parent_key_id:"v2key"

[nit] missing \n

@bdarnell bdarnell force-pushed the encryption-roachtest branch 2 times, most recently from 4f6b3f1 to 2e891d9 Compare March 28, 2024 07:23
Copy link
Member Author

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had trouble getting the test to work in remote roachtests (before I sent the PR I had only tested it locally), so I snuck in an additional feature to address part of #110123: Now you can specify path=* instead of matching the exact path from the store flag.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @jbowens, @RaduBerinde, and @srosenberg)


pkg/cmd/roachtest/tests/encryption.go line 38 at r1 (raw file):

Previously, RaduBerinde wrote…

I don't get this.. Looks like the first loop iteration is doing a plain to plain rotation.. do we want that?

Updated comments to explain what's going on: we have to specify pairs of keys so to have the current key be "plain" we need to specify the previous key as "plain" too. Or we could make the addition of the --enterprise-encryption key conditional on the loop index (skip it on the first start) but this way felt simpler.


pkg/cmd/roachtest/tests/encryption.go line 40 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Comments usually wrap at 80

Rewrapped everything.


pkg/ccl/storageccl/engineccl/testdata/data_key_manager line 254 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] missing \n

Fixed.

Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looked at the new commit to address part of #110123 and it lgtm

Reviewed 2 of 8 files at r2, 1 of 5 files at r3, 1 of 1 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bdarnell, @DarrylWong, @RaduBerinde, and @srosenberg)


-- commits line 15 at r4:
looks like some of the template survived

Matching the path between --store and --enterprise-encryption is difficult in
roachtests, and may be difficult in other orchestration frameworks as well.
Customizing keys on a per-store basis is rarely necessary so a wildcard can
sidestep this difficulty.

Updates cockroachdb#110123
Release note (cli change): The `--enterprise-encryption` flag now accepts the
special value `path=*` to apply the specified keys to all stores.
Prior tests of v2 encryption were too low-level and did not cover the data key
generation. The new roachtest provides end-to-end coverage.

This fixes a bug in not-yet-released functionality

Epic: none
Release note: none
The key format is changing so defer to "gen encryption-key" instead of details
of the v1 key format.

Epic: none
Release note: None
Copy link
Member Author

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @jbowens, @RaduBerinde, and @srosenberg)


-- commits line 15 at r4:

Previously, jbowens (Jackson Owens) wrote…

looks like some of the template survived

Derp, I wish that part of the template started with # like the rest.

@bdarnell
Copy link
Member Author

bors r=RaduBerinde,jbowens

@craig craig bot merged commit cc6cf24 into cockroachdb:master Mar 28, 2024
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants