Add encryption to diskqueue #31961

leehinman · 2022-06-16T21:52:30Z

What does this PR do?

Adds support for AES 128 CTR encryption to disk queue.

Why is it important?

Necessary for endpoint running under elastic-agent

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~- [ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

go test -bench=1M -benchtime 1x -count 10 -timeout 600m -benchmem > results.txt

benchstat results.txt
name               time/op
Async1M-16          22.0s ± 1%
EncryptAsync1M-16   20.5s ± 1%
Sync1M-16           25.2s ± 1%
EncryptSync1M-16    27.5s ± 0%

name               alloc/op
Async1M-16         3.43GB ± 0%
EncryptAsync1M-16  3.43GB ± 0%
Sync1M-16          3.42GB ± 0%
EncryptSync1M-16   3.42GB ± 0%

name               allocs/op
Async1M-16          40.6M ± 0%
EncryptAsync1M-16   40.5M ± 0%
Sync1M-16           39.5M ± 0%
EncryptSync1M-16    40.1M ± 0%

Related issues

Relates elastic-agent-shipper#33
Relates improve disk queue performance by coalescing writes #31935
Relates Add encryption to diskqueue #31949

elasticmachine · 2022-06-16T21:54:35Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

elasticmachine · 2022-06-16T23:19:47Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-06-20T22:17:37.742+0000
Duration: 92 min 12 sec

Test stats 🧪

Test	Results
Failed	0
Passed	22282
Skipped	1937
Total	24219

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

cmacknz · 2022-06-17T19:50:37Z

Benchmarks for this implementation:

name               time/op
Async1M-16          22.0s ± 1%
EncryptAsync1M-16   20.5s ± 1%
Sync1M-16           25.2s ± 1%
EncryptSync1M-16    27.5s ± 0%

name               alloc/op
Async1M-16         3.43GB ± 0%
EncryptAsync1M-16  3.43GB ± 0%
Sync1M-16          3.42GB ± 0%
EncryptSync1M-16   3.42GB ± 0%

name               allocs/op
Async1M-16          40.6M ± 0%
EncryptAsync1M-16   40.5M ± 0%
Sync1M-16           39.5M ± 0%
EncryptSync1M-16    40.1M ± 0%

Compared to the previous implementation: #31949

name           time/op
V1Async1M-16    38.8s ± 2%
V2Async1M-16    37.9s ± 8%
V1Sync1M-16     37.7s ± 1%
V2Sync1M-16     40.3s ± 0%

name           alloc/op
V1Async1M-16   3.47GB ± 0%
V2Async1M-16   3.46GB ± 0%
V1Sync1M-16    3.44GB ± 0%
V2Sync1M-16    3.45GB ± 0%


name           allocs/op
V1Async1M-16    43.4M ± 0%
V2Async1M-16    42.0M ± 0%
V1Sync1M-16     40.5M ± 0%
V2Sync1M-16     41.2M ± 0%

This version seems almost twice as fast ad allocates slightly less. What do you think caused this @leehinman? I want to make sure the improvement isn't a mirage :)

cmacknz · 2022-06-17T20:02:46Z

go test -bench=1M -benchtime 1x -count 10 -timeout 600m -benchmem Can we make this a mage target or even just a script in the disk queue directory? You've already figured out a reasonable config for running tests and we should save future developers from having to do that themselves.

libbeat/publisher/queue/diskqueue/docs/on-disk-structures.md

cmacknz · 2022-06-17T20:11:27Z

libbeat/publisher/queue/diskqueue/encryption.go

+
+func (er *EncryptionReader) Read(buf []byte) (int, error) {
+	if cap(er.ciphertext) >= len(buf) {
+		er.ciphertext = er.ciphertext[:len(buf)]


Are there any security concerns with the re-slicing here? Part of the previous ciphertext will potentially hang around at the end of the underlying array.

No, having ciphertext around is acceptable. We expect Malory to be able to "see" cipher text. Also I would assume anyone that could view memory of the process could also view the segment file which would have the cipher text as well.

cmacknz · 2022-06-17T20:21:02Z

libbeat/publisher/queue/diskqueue/segments.go

@@ -134,15 +135,26 @@ type segmentHeader struct {
 	// If the segment file has not been completely written, this field is zero.
 	// Only present in schema version >= 1.
 	frameCount uint32
+
+	// options holds flags to enable features, for example encryption.
+	options uint8


Any reason not to just make this larger for more future proofing? Probably nobody will notice 1-3 extra bytes in each segment file depending on uint16 or uint32 as the larger size.

I waffled on this. In the end I picked uint8 because I couldn't think of even 8 options, but we could easily go with uint32, that would match version & count.

switched to uint32

libbeat/publisher/queue/diskqueue/segments.go

libbeat/publisher/queue/diskqueue/segments_test.go

leehinman · 2022-06-18T00:19:06Z

This version seems almost twice as fast ad allocates slightly less. What do you think caused this @leehinman? I want to make sure the improvement isn't a mirage :)

So this is because #31935 wasn't merged when the benchmark was run for #39149, that is what accounts for the speed up and drop in allocations.

One thing that is directly comparable between #31935 and this PR is

"1M_10k-16 21.7s ± 0%" is the same test as "Async1M-16 22.0s ± 1%" in this PR. The times are close enough that I would say the changes in this PR that add the options field, and switches to a segmentReader instead of direct io.File, don't change the speed significantly. The allocations are the same between the two.

Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

leehinman · 2022-06-18T01:13:20Z

go test -bench=1M -benchtime 1x -count 10 -timeout 600m -benchmem Can we make this a mage target or even just a script in the disk queue directory? You've already figured out a reasonable config for running tests and we should save future developers from having to do that themselves.

Short term, I added comment to header of benchmark_test.go about how to run. I'll work with team to see what people want long term.

cmacknz · 2022-06-20T19:23:58Z

libbeat/publisher/queue/diskqueue/benchmark_test.go

@@ -15,8 +15,18 @@
 // specific language governing permissions and limitations
 // under the License.

-// example: go test -bench=. -benchtime 3x -timeout 60m
+// Usage:


cmacknz · 2022-06-20T19:26:43Z

libbeat/publisher/queue/diskqueue/segments_test.go

+	}
+	dir, err := os.MkdirTemp("", t.Name())
+	assert.Nil(t, err)
+	//	defer os.RemoveAll(dir)


Was this supposed to stay?

cmacknz

LGTM, wait for @faec to review before merging.

faec

looks good, thanks!

* Add encryption to diskqueue Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 16, 2022

leehinman added enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Jun 16, 2022

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 16, 2022

mergify bot assigned leehinman Jun 16, 2022

leehinman marked this pull request as ready for review June 16, 2022 21:54

leehinman requested a review from a team as a code owner June 16, 2022 21:54

leehinman requested review from fearful-symmetry and faec and removed request for a team June 16, 2022 21:54

leehinman mentioned this pull request Jun 16, 2022

Add encryption to diskqueue #31949

Closed

5 tasks

Add encryption to diskqueue

8e5d606

leehinman force-pushed the diskqueue_encryption_v2 branch from 8026f5e to 8e5d606 Compare June 16, 2022 22:41

cmacknz requested a review from rdner June 17, 2022 19:51

cmacknz reviewed Jun 17, 2022

View reviewed changes

libbeat/publisher/queue/diskqueue/docs/on-disk-structures.md Outdated Show resolved Hide resolved

cmacknz reviewed Jun 17, 2022

View reviewed changes

libbeat/publisher/queue/diskqueue/segments.go Outdated Show resolved Hide resolved

cmacknz reviewed Jun 17, 2022

View reviewed changes

libbeat/publisher/queue/diskqueue/segments_test.go Show resolved Hide resolved

cmacknz reviewed Jun 17, 2022

View reviewed changes

libbeat/publisher/queue/diskqueue/segments_test.go Show resolved Hide resolved

leehinman and others added 3 commits June 17, 2022 19:19

Update libbeat/publisher/queue/diskqueue/docs/on-disk-structures.md

1c32077

Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

Update libbeat/publisher/queue/diskqueue/segments.go

66562d4

Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

improve segment tests

34b6fd9

improve comment for running diskqueue benchmarks

3d28c63

leehinman force-pushed the diskqueue_encryption_v2 branch from e7e45c6 to 3d28c63 Compare June 18, 2022 01:17

switch options from uint8 to uint32

54dbb3f

cmacknz reviewed Jun 20, 2022

View reviewed changes

cmacknz approved these changes Jun 20, 2022

View reviewed changes

cleanup in tests

681b1c7

faec approved these changes Jun 21, 2022

View reviewed changes

leehinman merged commit 0e4a36b into elastic:main Jun 21, 2022

leehinman mentioned this pull request Jun 21, 2022

Add compression to disk queue #32024

Merged

5 tasks

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023

Add encryption to diskqueue (#31961)

97c4cf1

* Add encryption to diskqueue Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add encryption to diskqueue #31961

Add encryption to diskqueue #31961

leehinman commented Jun 16, 2022

elasticmachine commented Jun 16, 2022

elasticmachine commented Jun 16, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

cmacknz commented Jun 17, 2022

cmacknz commented Jun 17, 2022

cmacknz Jun 17, 2022 •

edited

Loading

leehinman Jun 18, 2022

cmacknz Jun 17, 2022

leehinman Jun 18, 2022

leehinman Jun 20, 2022

leehinman commented Jun 18, 2022

leehinman commented Jun 18, 2022

cmacknz Jun 20, 2022

cmacknz Jun 20, 2022

cmacknz left a comment

faec left a comment

Add encryption to diskqueue #31961

Add encryption to diskqueue #31961

Conversation

leehinman commented Jun 16, 2022

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

elasticmachine commented Jun 16, 2022

elasticmachine commented Jun 16, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

cmacknz commented Jun 17, 2022

cmacknz commented Jun 17, 2022

cmacknz Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

leehinman Jun 18, 2022

Choose a reason for hiding this comment

cmacknz Jun 17, 2022

Choose a reason for hiding this comment

leehinman Jun 18, 2022

Choose a reason for hiding this comment

leehinman Jun 20, 2022

Choose a reason for hiding this comment

leehinman commented Jun 18, 2022

leehinman commented Jun 18, 2022

cmacknz Jun 20, 2022

Choose a reason for hiding this comment

cmacknz Jun 20, 2022

Choose a reason for hiding this comment

cmacknz left a comment

Choose a reason for hiding this comment

faec left a comment

Choose a reason for hiding this comment

elasticmachine commented Jun 16, 2022 •

edited by jenkins-beats-ci bot

Loading

cmacknz Jun 17, 2022 •

edited

Loading