storageccl: implement on-the-fly decrypting remote sst reader

[dt]

The chunked encrypted encoding means we can jump to arbitrary chunks and
start decrypting. This allows implementing ReadAt and thus a pebble SST
reader can be created that decrypts on the fly, as it reads.

Release note: none.

[cockroach-teamcity]

This change is 

[miretskiy]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

---

pkg/ccl/storageccl/encryption.go, line 225 at r1 (raw file):

	}

	r.chunk = -1 // invalidate the current buffered chunk while we fill it.

is this needed?

---

pkg/ccl/storageccl/encryption.go, line 232 at r1 (raw file):

		return err
	}
	r.buf = r.buf[:n]

n could be 0 -- is this a problem (i.e. can we call r.g.Open below?)

---

pkg/ccl/storageccl/encryption.go, line 279 at r1 (raw file):

		// Return EOF if this was the last chunk (<chunksize).
		if len(r.buf) < encryptionChunkSizeV2 {

if I understand correctly, r.buf contains depcrypted 64KB (or less) chunk.
What happens if p is say, 10 bytes, and my file was 20 bytes?
I think I'll return 10, eof here, which is not correct.

Am I wrong?

---

pkg/ccl/storageccl/encryption_test.go, line 150 at r1 (raw file):

			require.Equal(t, expectedEmptyErr != nil, gotEmptyErr != nil)
			require.Equal(t, expectedEmpty, gotEmpty)
		})

Perhaps few more test cases?

Test case for file w/ 1 chunk < 32 bytes;

• ReadAt into a large buf (>32)
• ReadAt into a tiny buffer (1 - 10 bytes)
  Test case 1 byte file.

[dt]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @miretskiy)

---

pkg/ccl/storageccl/encryption.go, line 225 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

is this needed?

Yes (depending on what callers do if they get an error): r.chunk indicated which chunk's data is currently inr.buf. Just below this, we're going to overwrite r.buf with first the ciphertext, then the cleartext of a new chunk, and then update r.chunk to reflect what's in it. If we return after we overwrite r.buf but before we successfully load the the new chunk and update r.chunk, we need to make sure we no longer claim to have the previous chunk still loaded.

This is unlikely to be an issue in practice: if a caller said "readAt(5) and we loaded the chunk containing 5, then said readAt(100) and got an error become someone tampered with some bytes in chunk containing 100, they probably would either a) bubble that error and close/discard this reader entirely, or b) try to read 100 again, but technically, they could ask to read 5 again, in which case we should try to load it again, since it isn't loaded any more.

---

pkg/ccl/storageccl/encryption.go, line 232 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

n could be 0 -- is this a problem (i.e. can we call r.g.Open below?)

that's fine -- r.Open(nil) will fail to authenticate which is what we want (it indicates someone tampered with the file, so failing to authenticate is correct)

---

pkg/ccl/storageccl/encryption.go, line 279 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

if I understand correctly, r.buf contains depcrypted 64KB (or less) chunk.
What happens if p is say, 10 bytes, and my file was 20 bytes?
I think I'll return 10, eof here, which is not correct.

Am I wrong?

If p is length 10 and we have 20 bytes of cleartext, we'd have already returned 10, nil in the case above this one (if read == len(p)). We only get here if we ran out of r.buf without filling p, and if that happened and r.buf is short, we know we're at EOF.

---

pkg/ccl/storageccl/encryption_test.go, line 150 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Perhaps few more test cases?

Test case for file w/ 1 chunk < 32 bytes;

• ReadAt into a large buf (>32)
• ReadAt into a tiny buffer (1 - 10 bytes)
  Test case 1 byte file.

the rand test

[miretskiy]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

---

pkg/ccl/storageccl/encryption.go, line 225 at r1 (raw file):

Previously, dt (David Taylor) wrote…

Yes (depending on what callers do if they get an error): r.chunk indicated which chunk's data is currently inr.buf. Just below this, we're going to overwrite r.buf with first the ciphertext, then the cleartext of a new chunk, and then update r.chunk to reflect what's in it. If we return after we overwrite r.buf but before we successfully load the the new chunk and update r.chunk, we need to make sure we no longer claim to have the previous chunk still loaded.

This is unlikely to be an issue in practice: if a caller said "readAt(5) and we loaded the chunk containing 5, then said readAt(100) and got an error become someone tampered with some bytes in chunk containing 100, they probably would either a) bubble that error and close/discard this reader entirely, or b) try to read 100 again, but technically, they could ask to read 5 again, in which case we should try to load it again, since it isn't loaded any more.

technically, doing anything after an error is illegal and undefined.
I'm okay w/ being paranoid though.

---

pkg/ccl/storageccl/encryption.go, line 232 at r1 (raw file):

Previously, dt (David Taylor) wrote…

that's fine -- r.Open(nil) will fail to authenticate which is what we want (it indicates someone tampered with the file, so failing to authenticate is correct)

Let's say the caller tries to read way past end of the file -- as the first call.
You would expect to get an io.EOF in that case. However, reading more than 1 byte past the end could return EINVAL (as in invalid offset) as opposed to EIOF -- and afaik, that happens on linux (I'm not sure what golang might do -- maybe it changes error code).

So, in that case we would return some authentication error -- which I guess is fine, but I just want to make sure that that's what we want.

---

pkg/ccl/storageccl/encryption.go, line 279 at r1 (raw file):

Previously, dt (David Taylor) wrote…

If p is length 10 and we have 20 bytes of cleartext, we'd have already returned 10, nil in the case above this one (if read == len(p)). We only get here if we ran out of r.buf without filling p, and if that happened and r.buf is short, we know we're at EOF.

Ack.

[dt]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, and @miretskiy)

---

pkg/ccl/storageccl/encryption.go, line 225 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

technically, doing anything after an error is illegal and undefined.
I'm okay w/ being paranoid though.

I don't think that's quite true -- if you get an error from one ReadAt call you're allowed to try it again, try a different one, etc. So we want to make sure we don't have one failed ReadAt leave the reader in a state where a future call would return incorrect results. In practice I think most callers presented with an error would bail and close the reader, but they're within their rights to try to readAt again, so this isn't even just paranoia.

---

pkg/ccl/storageccl/encryption.go, line 232 at r1 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

Let's say the caller tries to read way past end of the file -- as the first call.
You would expect to get an io.EOF in that case. However, reading more than 1 byte past the end could return EINVAL (as in invalid offset) as opposed to EIOF -- and afaik, that happens on linux (I'm not sure what golang might do -- maybe it changes error code).

So, in that case we would return some authentication error -- which I guess is fine, but I just want to make sure that that's what we want.

Yep, indeed, that is what we want: we're using authenticated encryption, to ensure we detect tampering, so presumably we also care about an attacker truncating the ciphertext file. We only return a real EOF when we decrypt and authenticate the short block that indicates end of cleartext -- anything else could be result of something malicious. For all we know, our "read way past the end of the file" is only past the end of the tampered file. All we know for sure is that we can't yield some authenticated bytes at that offset.

[dt]

@miretskiy is this good to go?

[miretskiy]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dt, and @miretskiy)

[dt]

TFTR!

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)