[go] crypto: add tests for the Block interface
Manuel Sabin (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Garrett
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
- Filippo Valsorda
- Garrett
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Garrett
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Garrett
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Garrett
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin added 1 comment![]( "Open in Gerrit")
t.Run("InvertibleEncryptDecrypt", func(t *testing.T) {Maybe call this Cycle to match with cipher.BlockMode tester name
Related details
- Filippo Valsorda
- Garrett
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russell Webb (Gerrit)
Russell Webb added 8 comments![]( "Open in Gerrit")
rng := newRandReader(t)Call this in each test case.
t.Run("Cycle", func(t *testing.T) {
// Check Decrypt inverts Encrypt
before, ciphertext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
rng.Read(before)
block.Encrypt(ciphertext, before)
block.Decrypt(after, ciphertext)
expectEqual(t, after, before, "plaintext is different after an encrypt/decrypt cycle")
// Check Encrypt inverts Decrypt (assumes block ciphers are deterministic)
before, plaintext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
rng.Read(before)
block.Decrypt(plaintext, before)
block.Encrypt(after, plaintext)
expectEqual(t, after, before, "ciphertext is different after a decrypt/encrypt cycle")
})I recommend splitting these up into two separate test cases. In this case it's not crucial, but:
In the abstract, each test case should verify one desired behavior.
In the concrete, if the merged test case fails on the first expect, the second expect will never run, so the person running the test won't know if it would have passed or failed.
src, dst, before := make([]byte, blockSize*2), make([]byte, blockSize*2), make([]byte, blockSize*2)
rng.Read(src)
copy(before, src)
cipher(dst[:blockSize], src[:blockSize])
expectEqual(t, src, before, "block cipher modified src")
// cipher on the same src data should yield same output even if dst=src
copy(src, before)
cipher(src[:blockSize], src[:blockSize])
expectEqual(t, src[:blockSize], dst[:blockSize], "block cipher behaves differently when dst = src")
rng.Read(src)
rng.Read(dst)
copy(before, dst)
cipher(dst, src[:blockSize])
expectEqual(t, dst[blockSize:], before[blockSize:], "block cipher modified dst past BlockSize bytes")
// cipher should yield same dst when given same src even if src is longer
// than BlockSize
copy(before, dst)
cipher(dst, src) // src unchanged from last cipher call except longer
expectEqual(t, dst[:blockSize], before[:blockSize], "block cipher affected by src data beyond BlockSize bytes")In this case, having four separate test cases is probably even more valuable - if I'm debugging my cipher.Block, I probably want to know whether each test is passing/failing, rather than just which is the first to fail.
copy(beforePrefix, dst[:blockSize])Optional: Consider giving your indices into dst (blocksize, blocksize*2) semantically meaningful names (maybe startOfBlock and startOfSuffix?)
t.Run("BufferOverlap", func(t *testing.T) {
// Make src and dst slices point to same array with inexact overlap
src := make([]byte, blockSize+1)
rng.Read(src)
dst := src[1:]
mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
})Optional: also test single-byte overlaps (off-by-one is a mischievous beast).
t.Run("BufferOverlap", func(t *testing.T) {
// Make src and dst slices point to same array with inexact overlap
src := make([]byte, blockSize+1)
rng.Read(src)
dst := src[1:]
mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
})Optional: also test overlap where src starts later in the underlying array than dst. (Defining both src and dst as slices into a common buffer might make this easier)
bytes := func(n int) []byte { return make([]byte, n+1)[0:n] }Optional: rename this (perhaps subslice?) to avoid confusion with https://pkg.go.dev/bytes
mustPanic(t, "output not full block", func() { cipher(bytes(blockSize-1), bytes(blockSize)) })I'm guessing the cipher function can't reasonably extend the dst slice here because it's not returning the updated dst slice?
Related details
- Filippo Valsorda
- Garrett
- Manuel Sabin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin added 8 comments![]( "Open in Gerrit")
Call this in each test case.
Done
t.Run("Cycle", func(t *testing.T) {
// Check Decrypt inverts Encrypt
before, ciphertext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
rng.Read(before)
block.Encrypt(ciphertext, before)
block.Decrypt(after, ciphertext)
expectEqual(t, after, before, "plaintext is different after an encrypt/decrypt cycle")
// Check Encrypt inverts Decrypt (assumes block ciphers are deterministic)
before, plaintext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
rng.Read(before)
block.Decrypt(plaintext, before)
block.Encrypt(after, plaintext)
expectEqual(t, after, before, "ciphertext is different after a decrypt/encrypt cycle")
})I recommend splitting these up into two separate test cases. In this case it's not crucial, but:
In the abstract, each test case should verify one desired behavior.
In the concrete, if the merged test case fails on the first expect, the second expect will never run, so the person running the test won't know if it would have passed or failed.
The tests actually all run even if an earlier one fails. When breaking the tests I can get these errors to all print out in one test run.
Happy to split tests but I've also been grouping some tests based on patterns I've seen in the pre-existing crypto tests and Filippo's drafts for BlockMode/Stream tests. I'm still getting a sense of Go's idioms, so I'm not sure which is more idiomatic.
Do you have leanings given that all the tests do in fact run and print (hopefully descriptive error msgs) even if an early one fails?
src, dst, before := make([]byte, blockSize*2), make([]byte, blockSize*2), make([]byte, blockSize*2)
rng.Read(src)
copy(before, src)
cipher(dst[:blockSize], src[:blockSize])
expectEqual(t, src, before, "block cipher modified src")
// cipher on the same src data should yield same output even if dst=src
copy(src, before)
cipher(src[:blockSize], src[:blockSize])
expectEqual(t, src[:blockSize], dst[:blockSize], "block cipher behaves differently when dst = src")
rng.Read(src)
rng.Read(dst)
copy(before, dst)
cipher(dst, src[:blockSize])
expectEqual(t, dst[blockSize:], before[blockSize:], "block cipher modified dst past BlockSize bytes")
// cipher should yield same dst when given same src even if src is longer
// than BlockSize
copy(before, dst)
cipher(dst, src) // src unchanged from last cipher call except longer
expectEqual(t, dst[:blockSize], before[:blockSize], "block cipher affected by src data beyond BlockSize bytes")In this case, having four separate test cases is probably even more valuable - if I'm debugging my cipher.Block, I probably want to know whether each test is passing/failing, rather than just which is the first to fail.
See above comment on Cycle
Optional: Consider giving your indices into dst (blocksize, blocksize*2) semantically meaningful names (maybe startOfBlock and startOfSuffix?)
Done
t.Run("BufferOverlap", func(t *testing.T) {
// Make src and dst slices point to same array with inexact overlap
src := make([]byte, blockSize+1)
rng.Read(src)
dst := src[1:]
mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
})Optional: also test single-byte overlaps (off-by-one is a mischievous beast).
Done
t.Run("BufferOverlap", func(t *testing.T) {
// Make src and dst slices point to same array with inexact overlap
src := make([]byte, blockSize+1)
rng.Read(src)
dst := src[1:]
mustPanic(t, "invalid buffer overlap", func() { cipher(dst, src) })
})Optional: also test overlap where src starts later in the underlying array than dst. (Defining both src and dst as slices into a common buffer might make this easier)
Done
bytes := func(n int) []byte { return make([]byte, n+1)[0:n] }Optional: rename this (perhaps subslice?) to avoid confusion with https://pkg.go.dev/bytes
Done
mustPanic(t, "output not full block", func() { cipher(bytes(blockSize-1), bytes(blockSize)) })I'm guessing the cipher function can't reasonably extend the dst slice here because it's not returning the updated dst slice?
Probably not, unless it does some shenanigans with the slice header. But I think this test is just enforcing that cipher will panic if given too small of a dst slice
Related details
- Filippo Valsorda
- Garrett
- Russell Webb
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Garrett
- Russell Webb
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Garrett
- Russell Webb
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
Related details
- Filippo Valsorda
- Garrett
- Russell Webb
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin added 1 comment![]( "Open in Gerrit")
cryptotest.TestBlock(t, keylen/8, NewCipher)NewCipher seems to conditionally return different implementations.
Should we call each implementation individually here instead? i.e. boring.NewAESCipher, newCipherGeneric, and newCipher?
Related details
- Filippo Valsorda
- Garrett
- Russell Webb
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manuel Sabin (Gerrit)
Manuel Sabin uploaded new patchset![]( "Open in Gerrit")
- Filippo Valsorda
- Russell Webb
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russell Webb (Gerrit)
Russell Webb voted and added 8 comments![]( "Open in Gerrit")
Votes added by Russell Webb
| Code-Review | +1 |
8 comments
t.Run("Cycle", func(t *testing.T) {
// Check Decrypt inverts Encrypt
before, ciphertext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
rng.Read(before)
block.Encrypt(ciphertext, before)
block.Decrypt(after, ciphertext)
expectEqual(t, after, before, "plaintext is different after an encrypt/decrypt cycle")
// Check Encrypt inverts Decrypt (assumes block ciphers are deterministic)
before, plaintext, after := make([]byte, blockSize), make([]byte, blockSize), make([]byte, blockSize)
rng.Read(before)
block.Decrypt(plaintext, before)
block.Encrypt(after, plaintext)
expectEqual(t, after, before, "ciphertext is different after a decrypt/encrypt cycle")
})Manuel SabinI recommend splitting these up into two separate test cases. In this case it's not crucial, but:
In the abstract, each test case should verify one desired behavior.
In the concrete, if the merged test case fails on the first expect, the second expect will never run, so the person running the test won't know if it would have passed or failed.
The tests actually all run even if an earlier one fails. When breaking the tests I can get these errors to all print out in one test run.
Happy to split tests but I've also been grouping some tests based on patterns I've seen in the pre-existing crypto tests and Filippo's drafts for BlockMode/Stream tests. I'm still getting a sense of Go's idioms, so I'm not sure which is more idiomatic.
Do you have leanings given that all the tests do in fact run and print (hopefully descriptive error msgs) even if an early one fails?
Good catch; I stand corrected. I do still like isolating test cases from each other in terms of cognitive load and state leak between them, but I don't think it's as critical. I'll leave it to Filippo's preferences.
t.Run("Aliasing", func(t *testing.T) {I really do think this would be better as separate test cases, particularly because the cognitive load of thinking about how they interact is non-trivial.
For example, figuring out that the "dst" value calculated in the first test case is being used as the expected value for the second was not immediately obvious - it'd be easier to understand if it was called "expectedCryptext" or something.
// cipher on the same src data should yield same output even if dst=srcThis is confusing to me because "dst" is overloaded - it refers to both the first argument of cipher and the variable defined on line 71, where the former is correct and the latter is the "obvious" interpretation.
Can we say "input and output locations are the same" or something to that effect?
t.Errorf("block cipher behaves differently when dst = src; got %x, want %x", src[:blockSize], dst[:blockSize])Nit: I'd describe this as 'different cryptext produced when...'
src, dst, before := make([]byte, blockSize*2), make([]byte, blockSize*2), make([]byte, blockSize*2)
rng.Read(src)
copy(before, src)
cipher(dst[:blockSize], src[:blockSize])
expectEqual(t, src, before, "block cipher modified src")
// cipher on the same src data should yield same output even if dst=src
copy(src, before)
cipher(src[:blockSize], src[:blockSize])
expectEqual(t, src[:blockSize], dst[:blockSize], "block cipher behaves differently when dst = src")
rng.Read(src)
rng.Read(dst)
copy(before, dst)
cipher(dst, src[:blockSize])
expectEqual(t, dst[blockSize:], before[blockSize:], "block cipher modified dst past BlockSize bytes")
// cipher should yield same dst when given same src even if src is longer
// than BlockSize
copy(before, dst)
cipher(dst, src) // src unchanged from last cipher call except longer
expectEqual(t, dst[:blockSize], before[:blockSize], "block cipher affected by src data beyond BlockSize bytes")Manuel SabinIn this case, having four separate test cases is probably even more valuable - if I'm debugging my cipher.Block, I probably want to know whether each test is passing/failing, rather than just which is the first to fail.
See above comment on Cycle
Acknowledged
ctrlDst := make([]byte, blockSize) // Record a control ciphertextnit: expectedDst?
mustPanic(t, "output not full block", func() { cipher(bytes(blockSize-1), bytes(blockSize)) })Manuel SabinI'm guessing the cipher function can't reasonably extend the dst slice here because it's not returning the updated dst slice?
Probably not, unless it does some shenanigans with the slice header. But I think this test is just enforcing that cipher will panic if given too small of a dst slice
Acknowledged
// src comes after dst with inexact overlapWould it be accurate to say "one byte overlap" here?
Related details
- Filippo Valsorda
- Manuel Sabin
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |