Jojijacob signed store

[jojijac0b]

Create signed store to be used to upload and download signed blobs.

Create encrypted signed store to extend signed store and encrypt/decrypt when uploading and downloading model blobs respectively.

[wfa-reviewable]

This change is 

[stevenwarejones]

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @jojijac0b)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 25 at r1 (raw file):

/** Store of blob and signature. */
class SignedStore(storageClient: StorageClient) {

you should just be able to do

class SignedStore(private val storageClient: StorageClient) {

and delete the following line

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 44 at r1 (raw file):

    val signedBlob = content.onEach(privateKey.newSigner(x509)::update)
    val signature = privateKey.newSigner(x509).sign().toByteString()

generate the signature like here:

common-jvm/src/test/kotlin/org/wfanet/measurement/common/crypto/SignaturesTest.kt

Line 93 in 8f5eefc

val signature = runBlocking {

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 45 at r1 (raw file):

    val signedBlob = content.onEach(privateKey.newSigner(x509)::update)
    val signature = privateKey.newSigner(x509).sign().toByteString()
    storageClient.writeBlob(blobKeyForContent(blobKey), signedBlob)

there's not a concept of the signedBlob here --- you can just write the content out

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 51 at r1 (raw file):

  @Throws(BlobNotFoundException::class)
  suspend fun read(blobKey: String, x509: X509Certificate): VerifiedBlob? {

i don't think you need VerifiedBlob - just have this return a SignedBlob

common-jvm/src/main/kotlin/org/wfanet/measurement/common/crypto/SignedBlob.kt

Line 25 in 8f5eefc

class SignedBlob(wrapped: StorageClient.Blob, val signature: ByteString) :

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r1 (raw file):

  @Throws(BlobNotFoundException::class)
  suspend fun read(blobKey: String, x509: X509Certificate): VerifiedBlob? {
    val signedBlob = storageClient.getBlob(blobKeyForContent(blobKey)) ?: throw BlobNotFoundException(blobKey)

val content =

[jojijac0b]

Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 44 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

generate the signature like here:

common-jvm/src/test/kotlin/org/wfanet/measurement/common/crypto/SignaturesTest.kt

Line 93 in 8f5eefc

val signature = runBlocking {

whats the reasoning to use the collectAndSign method instead of just sign like its done here:

val signature = privateKey.sign(certificate, DATA)

[jojijac0b]

Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 51 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i don't think you need VerifiedBlob - just have this return a SignedBlob

common-jvm/src/main/kotlin/org/wfanet/measurement/common/crypto/SignedBlob.kt

Line 25 in 8f5eefc

class SignedBlob(wrapped: StorageClient.Blob, val signature: ByteString) :

would this verify that the content has not been corrupted?

[stevenwarejones]

Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 44 at r1 (raw file):

Previously, jojijac0b wrote…

whats the reasoning to use the collectAndSign method instead of just sign like its done here:

val signature = privateKey.sign(certificate, DATA)

you still need to pass in the data to sign

common-jvm/src/main/kotlin/org/wfanet/measurement/common/crypto/Signatures.kt

Line 55 in d6e938e

fun PrivateKey.sign(certificate: X509Certificate, data: ByteString): ByteString {

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 51 at r1 (raw file):

Previously, jojijac0b wrote…

would this verify that the content has not been corrupted?

this verifies it as it reads it -- https://github.com/world-federation-of-advertisers/common-jvm/blob/d6e938ee3c94030f0759695666c11be7a8768128/src/main/kotlin/org/wfanet/measurement/common/crypto/SignedBlob.kt#L45C7-L45C20

[jojijac0b]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 25 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you should just be able to do

class SignedStore(private val storageClient: StorageClient) {

and delete the following line

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 44 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you still need to pass in the data to sign

common-jvm/src/main/kotlin/org/wfanet/measurement/common/crypto/Signatures.kt

Line 55 in d6e938e

fun PrivateKey.sign(certificate: X509Certificate, data: ByteString): ByteString {

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 45 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

there's not a concept of the signedBlob here --- you can just write the content out

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 51 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

this verifies it as it reads it -- https://github.com/world-federation-of-advertisers/common-jvm/blob/d6e938ee3c94030f0759695666c11be7a8768128/src/main/kotlin/org/wfanet/measurement/common/crypto/SignedBlob.kt#L45C7-L45C20

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

val content =

Done.

[stevenwarejones]

Reviewed 4 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 16 at r2 (raw file):

  //  PrivateKey and X509Certificate used to create the signature when writing the data
  //  PublicKey used to encrypt the content
  internal suspend fun write(

i don't think write should be internal

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 23 at r2 (raw file):

    content: Flow<ByteString>
  ): String {
    val encryptedData = publicKeysetHandle.hybridEncrypt(content, null)

you can just do

val encryptedData = publicKeysetHandle.hybridEncrypt(content, null)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 27 at r2 (raw file):

  }

  internal suspend fun read(

ditto

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 32 at r2 (raw file):

    privateKeysetHandle: PrivateKeyHandle
  ): Flow<ByteString>? {
    val encryptedContent = signedStore.read(blobKey, x509)

this shouldn't compile in its current state.... see my other content about changing the return type of SignedStore.read

Also, you'll need to collect the flow here too. and then return it asFlow

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 33 at r2 (raw file):

  ): Flow<ByteString>? {
    val encryptedContent = signedStore.read(blobKey, x509)
    return privateKeysetHandle.hybridDecrypt(encryptedContent, null)

ditto on no need to pass null

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 25 at r2 (raw file):

class SignedStore(private val storageClient: StorageClient) {

  internal fun blobKeyForSignature(blobKey: String): String {

I'd make this private instead of internal. I'd also prefer

return "signature/$blobKey"

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 29 at r2 (raw file):

  }

  internal fun blobKeyForContent(blobKey: String): String {

ditto on making it private but use / instead of .

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 33 at r2 (raw file):

  }

  internal suspend fun write(

i think this, too, shouldn't be internal

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r2 (raw file):

  @Throws(BlobNotFoundException::class)
  internal suspend fun read(blobKey: String, x509: X509Certificate): SignedBlob? {

i think this should be public. Also - wdyt about just having this return a Flow<ByteString directly?

return SignedBlob(content, signature).readVerifying(x509)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r2 (raw file):

  @Throws(BlobNotFoundException::class)
  internal suspend fun read(blobKey: String, x509: X509Certificate): SignedBlob? {

return type should just be SignedBlob

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/DownloadClient.kt line 28 at r2 (raw file):

 * Interface for that downloads blob from shared storage.
 */
interface DownloadClient {

delete this file

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/HybridDecryptor.kt line 11 at r2 (raw file):

interface HybridDecryptor {

delete this file

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/HybridEncryptor.kt line 15 at r2 (raw file):

   * The keyId is used to retrieve the public key from a PrivateKeyStore. This is used to generate the KeysetHandle
   */
  suspend fun hybridEncrypt(

delete this file

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/UploadStorageClient.kt line 30 at r2 (raw file):

 * Interface for [StorageClient] that writes (blob, signature) pairs to shared storage.
 */
interface UploadStorageClient {

delete this file

[jojijac0b]

Reviewable status: 0 of 6 files reviewed, 14 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 16 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i don't think write should be internal

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 23 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you can just do

val encryptedData = publicKeysetHandle.hybridEncrypt(content, null)

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 27 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

ditto

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 32 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

this shouldn't compile in its current state.... see my other content about changing the return type of SignedStore.read

Also, you'll need to collect the flow here too. and then return it asFlow

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 33 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

ditto on no need to pass null

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 25 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I'd make this private instead of internal. I'd also prefer

return "signature/$blobKey"

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 29 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

ditto on making it private but use / instead of .

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 33 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i think this, too, shouldn't be internal

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i think this should be public. Also - wdyt about just having this return a Flow<ByteString directly?

return SignedBlob(content, signature).readVerifying(x509)

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

return type should just be SignedBlob

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/DownloadClient.kt line 28 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

delete this file

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/HybridDecryptor.kt line 11 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

delete this file

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/HybridEncryptor.kt line 15 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

delete this file

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/tink/UploadStorageClient.kt line 30 at r2 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

delete this file

Done.

[stevenwarejones]

Needs tests too.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 25 at r2 (raw file):

Previously, jojijac0b wrote…

Done.

can you make it private?

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 29 at r2 (raw file):

Previously, jojijac0b wrote…

Done.

can you make it private?

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 52 at r3 (raw file):

  @Throws(BlobNotFoundException::class)
  suspend fun read(blobKey: String, x509: X509Certificate): Flow<ByteString>? {

you don't need to return a nullable flow I don't think.... just Flow<ByteString>

[stevenwarejones]

Needs tests still.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)

[stevenwarejones]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 53 at r5 (raw file):

  }

  suspend fun read(

this should throw blobnotfound exception

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 57 at r5 (raw file):

    signingX509: X509Certificate,
    decryptingPrivateKeyHandle: PrivateKeyHandle
  ): Flow<ByteString>? {

you should need the nullable param here -- just Flow<ByteString>

[jojijac0b]

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 53 at r5 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

this should throw blobnotfound exception

Done.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 57 at r5 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

you should need the nullable param here -- just Flow<ByteString>

Done.

[jojijac0b]

Tests are there. I made an update right after the initial push that adds the test to fix the lint errors so it may not have shown up. Tests are SignedStoreTest and EncryptedSignedStoreTest

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @stevenwarejones)

[stevenwarejones]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier and @SanjayVas)

[SanjayVas]

Reviewed 3 of 6 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

a discussion (no related file):
I'm still trying to understand the use cases to determine whether this is using the current storage abstractions correctly, or if we instead need to change the abstractions to fit the new use case.

A reminder of the current types:

• StorageClient - Interface for raw storage access. The intent was not to use this directly but rather access it through a store.
  • StorageClient.Blob - Interface that provides a reference to a written blob for a given StorageClient implementation.
• Store - Abstract class which wraps a StorageClient for a specific blob type. It provides two features: private blob key prefix and deterministic blob key derivation.
  • Private blob key prefixes are intended to ensure that the only way a blob is accessed is through the Store implementation. This assumes that blobs are always written and read by the same entity, as that was the only use case when the interface was designed.
  • Deterministic blob key derivation is to ensure that you always get the same blob key for the same context (e.g. a Requisition). If you actually just want to use some unique identifier directly then you can just specify the context type as String.
• KmsStorageClient - A StorageClient which wraps another StorageClient, passively ensuring blobs are encrypted at rest with a single symmetric key managed by a KMS.
• SignedBlob - StorageClient.Blob implementation which wraps another StorageClient.Blob and a signature, providing methods for verifying that the blob matches the signature.
• StorageClient.createSignedBlob - Extension method for creating a SignedBlob, i.e. generating a signature while writing a blob.

If we want to handle cases where blobs are intended to be accessible by entities that may not be using Halo code (e.g. a DataProvider or ModelProvider), then we may want to rethink the Store interface a bit or at least rename it to something like PrefixStore. Ideally we could still have some Store interface so we could have some consistency around anything that abstracts a StorageClient, but I understand if that might too far out of scope for this change.

---

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 32 at r6 (raw file):

private const val WRITE_BUFFER_SIZE = BYTES_PER_MIB * 5

class EncryptedSignedStore(private val storageClient: StorageClient) {

Add KDoc documentation here.

I assume this is a different use case than KmsStorageClient which uses symmetric encryption and the same key for all blobs. This use of asymmetric encryption implies that the entity writing the blob is never the entity reading the blob.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 24 at r6 (raw file):

import org.wfanet.measurement.storage.StorageClient

/** Store of blob and signature. */

nit: Keep the abstractions at the right level. At the layer at which we're talking about both the content and the signature they are two separate blobs. At the layer that isn't aware of the signature we can treat it as one logical blob.

Code quote:

Store of blob and signature.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 41 at r6 (raw file):

    content: Flow<ByteString>
  ): String {
    // Since StorageClient has no concept of "overwriting" a blob, we first delete existing blobs.

We should change the StorageClient API if we actually need to support the overwrite use case.

Note that this may just be a matter of adjusting comments, as I don't think any of our existing implementations will actually stop you from overwriting a blob with the same key.

---

src/main/kotlin/org/wfanet/measurement/common/crypto/SignedStore.kt line 53 at r6 (raw file):

  }

  @Throws(BlobNotFoundException::class)

The @Throws annotation is mainly for compatibility with Java code. It is unlikely that Java code would use this library given the use of coroutines. Instead use the @throws KDoc tag in the function documentation.

As an aside, what's the reason for using an exception rather than returning null?

[SanjayVas]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

  //  PrivateKey and X509Certificate used to create the signature when writing the data
  //  PublicKey used to encrypt the content
  suspend fun write(

1. The pattern we've been using with encrypted and signed content is sign-then-encrypt, meaning the signature is part of the ciphertext. Why does this use encrypt-then-sign (i.e. why are the crypto properties we're seeking different for this use case)?
2. Is it okay to have the whole blob in memory? Hybrid encryption is efficient for large messages, but you need to have the entire blob in memory to encrypt/decrypt. Tink provides a streaming implementation for AEAD, but this is a symmetric scheme meaning you may need to do your own key exchange e.g. using hybrid encryption to pass an encrypted symmetric key.

[stevenwarejones]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

1. The pattern we've been using with encrypted and signed content is sign-then-encrypt, meaning the signature is part of the ciphertext. Why does this use encrypt-then-sign (i.e. why are the crypto properties we're seeking different for this use case)?
2. Is it okay to have the whole blob in memory? Hybrid encryption is efficient for large messages, but you need to have the entire blob in memory to encrypt/decrypt. Tink provides a streaming implementation for AEAD, but this is a symmetric scheme meaning you may need to do your own key exchange e.g. using hybrid encryption to pass an encrypted symmetric key.

1. Panel exchange currently uses an encrypt then sign strategy for blob storage which was the pattern we were following.
2. This is a good point. Is this what you were thinking @SanjayVas - https://developers.google.com/tink/encrypt-large-files-or-data-streams ?

[SanjayVas]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Panel exchange currently uses an encrypt then sign strategy for blob storage which was the pattern we were following.

Is this code intended to be used for panel exchange exclusively, or is this (also) for population? If for the latter, what is the cryptographic reasoning for following this pattern over sign-then-encrypt? (Just copying the way we do it for panel exchange without considering the cryptographic implications isn't a sound approach. Sign-then-encrypt is generally seen as the safer pattern, so we need to justify whenever we're not using it.)

Is this what you were thinking @SanjayVas - https://developers.google.com/tink/encrypt-large-files-or-data-streams ?

Yes.

[stevenwarejones]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Panel exchange currently uses an encrypt then sign strategy for blob storage which was the pattern we were following.

Is this code intended to be used for panel exchange exclusively, or is this (also) for population? If for the latter, what is the cryptographic reasoning for following this pattern over sign-then-encrypt? (Just copying the way we do it for panel exchange without considering the cryptographic implications isn't a sound approach. Sign-then-encrypt is generally seen as the safer pattern, so we need to justify whenever we're not using it.)

Is this what you were thinking @SanjayVas - https://developers.google.com/tink/encrypt-large-files-or-data-streams ?

Yes.

I'm okay with switching it to sign-then-encrypt.

[stevenwarejones]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I'm okay with switching it to sign-then-encrypt.

Question @SanjayVas - does this mean we would encrypt both pieces of the signed data -- the signature and the data or just the data. Feels like leaking the plaintext signature could tell you properties about the unencrypted data so safest would be to also encrypt the signature.

@jojijac0b - I suggest leaving SignedStore as-is but altering the EncryptedSignedStore to not use the underlying SignedStore but instead to follow its only flow of sign and then encrypt as Sanjay suggested. I could see adopters using either just a SignedStore or an EncryptedSignedStore so nice to give them both options.

[SanjayVas]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

does this mean we would encrypt both pieces of the signed data

For sign-then-encrypt, you combine the content and signature into a single blob then encrypt that to get a single ciphertext. It means you need to use some format where you can put both together. e.g. PKCS #7 or a protobuf message similar to SignedData from the CMMS public API.

[stevenwarejones]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

does this mean we would encrypt both pieces of the signed data

For sign-then-encrypt, you combine the content and signature into a single blob then encrypt that to get a single ciphertext. It means you need to use some format where you can put both together. e.g. PKCS #7 or a protobuf message similar to SignedData from the CMMS public API.

I don't think that works well for encrypting with a stream, right? -- eg just encrypt the signature in-memory but stream encrypt the data separately -- not to the same message. The store will keep track of the two blobs itself. Does that work for you @SanjayVas ?

[SanjayVas]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

I don't think that works well for encrypting with a stream, right

It depends on how you write the combined content + signature structure. For example consider a structure where you have the content followed by the signature, with a header/footer for each. Such a structure should allow for streaming all the way through.

eg just encrypt the signature in-memory but stream encrypt the data separately -- not to the same message. The store will keep track of the two blobs itself.

Traditionally with sign-then-encrypt you are doing a single encryption. Doing two separate encryptions might change the security implications.

[stevenwarejones]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I don't think that works well for encrypting with a stream, right

It depends on how you write the combined content + signature structure. For example consider a structure where you have the content followed by the signature, with a header/footer for each. Such a structure should allow for streaming all the way through.

eg just encrypt the signature in-memory but stream encrypt the data separately -- not to the same message. The store will keep track of the two blobs itself.

Traditionally with sign-then-encrypt you are doing a single encryption. Doing two separate encryptions might change the security implications.

@SanjayVas - what do you think about using https://docs.oracle.com/javase/8/docs/api/java/util/Scanner.html for this?

[SanjayVas]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

---

src/main/kotlin/org/wfanet/measurement/common/crypto/EncryptedSignedStore.kt line 38 at r6 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

@SanjayVas - what do you think about using https://docs.oracle.com/javase/8/docs/api/java/util/Scanner.html for this?

I'm not sure I understand where that would fit into the implementation. The details don't matter as much so long as we're doing things chunked rather than byte-by-byte. e.g. using ByteChannel instead of InputStream/OutputStream. Note that the StreamingAead primitive has ByteChannel methods. We already have functions in common-jvm to interface between coroutine Flow and ByteChannel, as the GCS Java API is also ByteChannel-based.

[stevenwarejones]

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

[stevenwarejones]

@SanjayVas - this is meant to cover the use case for only signed blobs but not encrypted. Encrypted will be covered in follow-ups.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

[SanjayVas]

From offline discussion, I still don't see a use case for opaquely storing the content and signature as two separate blobs.

1. If we're going to need this to be transparent (e.g. callers need to be able to interact with storage directly without necessarily using this code), then we shouldn't be hiding the use of two blobs as an implementation detail.
2. If we're going to want to encrypt the content as well, it should be done with sign-then-encrypt with a single ciphertext. In that case, the plaintext content and signature should never be stored.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

[stevenwarejones]

@SanjayVas - I believe the design here was across two use cases. I agree with you on the key transparency need that probably negates using the Store interface for VID model blobs. However, the other use case we were working toward was replacing the existing VerifyingStorageClient and SigningStorageClient used in panel exchange with something based around Store since the signatures are implementation details in that case. lmk what you think.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)

[SanjayVas]

replacing the existing VerifyingStorageClient and SigningStorageClient used in panel exchange with something based around Store since the signatures are implementation details in that case. lmk what you think.

Perhaps that should stay in the panel match tree in the main repo then, since I don't think it's a pattern that we necessarily want to replicate elsewhere.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jojijac0b and @Marco-Premier)