Describe and improve test vectors

[brycx]

The current test vectors do not include empty payloads. Tests with those should definitely be added IMO (see also https://github.com/return/branca/pull/13)

[tuupola]

Yeah, I agree. Time to kick myself and also document the test vectors in the spec.

[brycx]

I have had some notes for the test vectors lying around (still looking for the ones where something is added/modified to the current ones). I'd be willing to help with this, if you're interested. I was thinking of using JSON, with a similar structure to that of Wycheproof.

[tuupola]

Yeah it would be great to see your ideas. Currently the tests concentrate on edge cases I found during the development of the implementations I made myself. Mostly min and max values of the timestamp and multiple leading nul bytes in the payload.

For decode tests we can provide tokens with different edge cases and tampered tokens. Ie. there would be bunch of test like this:

branca = Branca(key="supersecretkeyyoushouldnotcommit")
token = "1jJDJOEfuc4uBJh5ivaadjo6UaBZJDZ1NsWixVCz2mXw3824JRDQZIgflRqCNKz6yC7a0JKC"

assert branca.decode(token) == b"\x00\x00\x00\x00\x00\x00\x00\x00"
assert branca.timestamp(token) == 123206400

Because implementation should not allow user provided nonce (since it is a footgun) we cannot provide resulting tokens for encode testing. Instead I think the encoding tests could use the same edge cases as above and test would do both encode and decode. Something like:

branca = Branca(key="supersecretkeyyoushouldnotcommit")
token = branca.encode(b"\x00\x00\x00\x00\x00\x00\x00\x00", timestamp=123206400)

assert branca.decode(token) == b"\x00\x00\x00\x00\x00\x00\x00\x00"
assert branca.timestamp(token) == 123206400

The thinking behind this is that if implementation can successfully decode the provided tokens in the first part, then encoding should be ok too if implementation decodes the tokens it encoded itself.

Or what do you think?

[brycx]

The thinking behind this is that if implementation can successfully decode the provided tokens in the first part, then encoding should be ok too if implementation decodes the tokens it encoded itself.

I see the potential benefit to this, but I actually disagree. I believe that nudging users towards not implementing a function encode that takes a nonce is a good idea, but I feel the test vectors shouldn't be the place where this is applied. Test vectors are there, only to enable other libraries to test for equivalence with the specification. They should provide as much information as possible.

Having test vectors for an encode function allows us to also have tests, which aren't existent now. Basic tests for the key and nonce lengths for example. Or take the Rust implementation as an example: It has the following encode function:

pub fn encode(data: &str, key: &[u8], timestamp: u32) -> Result<String, BrancaError>

&str is implicitly valid UTF-8. The decode function also takes &str. In this case, round-trip tests using these two functions will always pass. It is valid to require &str in decoding, because base62 encoded tokens are always valid UTF-8. But encode should not take &str, because then it actually doesn't accept arbitrary bytes. If there were a test for encode using non-UTF-8 input, then the Rust implementation would have been forced to test with something other than &str (like raw bytes) and thereby most likely have avoided the issue.

In the end, the implementer of the specification is deciding whether or not to allow users to provide a nonce. They can always implement a private encoding function, which takes a nonce and use that for testing. The the public API would expose an encoding function, which automatically generates the nonce for the user.

[tuupola]

If there were a test for encode using non-UTF-8 input, then the Rust implementation would have been forced to test with something other than &str (like raw bytes) and thereby most likely have avoided the issue.

I think this can be tested without passing the nonce like I suggested? For encoding tests the vectors can provide the arbitrary bytes data and timestamp. I do not do Rust myself but if I pass array of bytes as &str to the following method wouldn't it throw an error? I do not see why nonce should be given by user for this test?

pub fn encode(data: &str, key: &[u8], timestamp: u32) -> Result<String, BrancaError>

In general I agree it would be much easier to do encoding testing if test vectors provided everything including the nonce and resulting token. My worry just is it kind of implies the implementation must provide user settable nonces.

[brycx]

For encoding tests the vectors can provide the arbitrary bytes data and timestamp. I do not do Rust myself but if I pass array of bytes as &str to the following method wouldn't it throw an error? I do not see why nonce should be given by user for this test?

Sorry, I probably wasn't clear. The Rust example wasn't really in relation to the nonce, but more so in providing test vectors for encoding in general. Yes, on a type-level you can't pass byte-array [u8] as &str, but you can create &str from invalid UTF-8, with lossy conversions (String::from_utf8_lossy() which was the case for the Rust's decode() function). Therein the invalid UTF-8 gets replaced by placeholder UTF-8, creating invalid results.

Providing raw-byte data and timestamp in test vectors, without the nonce, for encoding could be used for tests by (providing that the decode function handles non-UTF-8 correctly. ):

local_encoded = local_impl.encode(data)
other_decoded = local_impl.decode(test_vector)
local_decoded = local_impl.decode(local_encoded)

assert(other_decoded == local_decoded)

I realize this is a somewhat niche example, and a better one hasn't come to mind yet. IMO, making implementations easier to test is also a worthwhile goal to pursue.

[tuupola]

IMO, making implementations easier to test is also a worthwhile goal to pursue.

I agree on this too. Just bit torn between providing a footgun and and making testing easy. The thing with footguns is there is always someone who will use it despite the warnings. I will ponder this a bit.

You said you also had some other ideas and notes about the test vectors. Care to share those?

[brycx]

You said you also had some other ideas and notes about the test vectors. Care to share those?

These were mainly just some additional things to test and how to structure those tests. I'm working on a set of test vectors now, I'll share those as a PR when I've rounded some rough edges. They include the nonce atm, but can be removed if you decide against it.

[tuupola]

Fixed by #33 and #35.