-
Notifications
You must be signed in to change notification settings - Fork 16
fix: Update TrusTEE mill to use StreamingAead #3341
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
Conversation
SanjayVas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SanjayVas reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh)
src/main/kotlin/org/wfanet/measurement/duchy/mill/trustee/TrusTeeMill.kt line 224 at r1 (raw file):
} private fun decryptRequisitionData(dek: KeysetHandle, data: ByteString): ByteArray {
nit: This isn't really taking advantage of the streaming aspect. The idea is to pass in each chunk while reading the blob, meaning you'd need to change getRequisitionData. See StreamingAead.decrypt in common-jvm.
Of course, there's less benefit if you need the whole plaintext in memory, but it still avoids having both the full plaintext and ciphertext in memory at the same time.
Maybe leave a TODO?
e839386 to
a71e389
Compare
renjiezh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/duchy/mill/trustee/TrusTeeMill.kt line 224 at r1 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: This isn't really taking advantage of the streaming aspect. The idea is to pass in each chunk while reading the blob, meaning you'd need to change getRequisitionData. See StreamingAead.decrypt in common-jvm.
Of course, there's less benefit if you need the whole plaintext in memory, but it still avoids having both the full plaintext and ciphertext in memory at the same time.
Maybe leave a TODO?
Added a TODO.
stevenwarejones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenwarejones reviewed 2 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh)
src/main/kotlin/org/wfanet/measurement/duchy/mill/trustee/TrusTeeMill.kt line 229 at r2 (raw file):
val streamingAead = dek.getPrimitive(StreamingAead::class.java) val decryptingStream = streamingAead.newDecryptingStream(data.newInput(), byteArrayOf()) return decryptingStream.readAllBytes()
i'd prefer this leverage/get combined with existing common-jvm classes like WithEnvelopeEncryption or StreamingEncryption or StreamingAeadStorageClient
renjiezh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/duchy/mill/trustee/TrusTeeMill.kt line 229 at r2 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
i'd prefer this leverage/get combined with existing common-jvm classes like
WithEnvelopeEncryptionorStreamingEncryptionorStreamingAeadStorageClient
That is a good point. I have created an ticket [https://github.com//issues/2800] for it.
Now my PRs are stacking on each other. It takes time for me to verify these type of change. Can we get PRs merged and then work on the improvements?
stevenwarejones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenwarejones reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh)
No description provided.