Use multihash for encoding the attestation user_data #27
Conversation
Because the `hashSeparator` character can occur within the binary hash digest values representing the https cert and optional app key, it's not useful for dividing the user_data value into separate fields. Instead use the multihash encoding, which prefixes two bytes. The first (0x12) define the hash digest as SHA-256 just as the previous "sha256:" prefix did. The second (0x20) marks the digest length. This more compactly delimits the values while allowing the same extensibility. Resolves #23
|
NB we shouldn't deploy this to the star-randsrv app until after brave/brave-core#18954 has made it into stable release. |
attestation.go
Outdated
| func (a *AttestationHashes) Serialize() []byte { | ||
| str := fmt.Sprintf("%s%s%s%s%s", | ||
| str := fmt.Sprintf("%s%s%s%s", |
There was a problem hiding this comment.
i'm unsure if fmt.Sprintf is a robust fit for creating a binary value. it may be better to use append or something similar
There was a problem hiding this comment.
You mean something like
serialized := []byte(hashPrefix)
serialized = append(serialized, a.tlsKeyHash[:]...)
serialized = append(serialized, hashPrefix[:]...)
serialized = append(serialized, a.appKeyHash[:]...)
return serializedThat's a lot more typing with the variable looping and slice+splat operators. That was the best I could come up with, but I don't really know go.
There was a problem hiding this comment.
There's also a go-multihash package we could use to perform the encoding at insert time, so we only need to concatenate the two slices. That might be cleaner overall.
There was a problem hiding this comment.
There's also a go-multihash package we could use to perform the encoding at insert time
Yeah, i suppose you could, but I think what you wrote above looks fine & lightweight enough. You could probably drop the [:] and just keep the ... at the end of the slice that is to be appended. A shortened version could look something like this (assuming that hashPrefix is changed to a byte slice):
serialized := []byte{}
serialized = append(serialized, append(hashPrefix, a.tlsKeyHash...)...)
serialized = append(serialized, append(hashPrefix, a.appKeyHash...)...)
return serialized
There was a problem hiding this comment.
or even:
serialized := append(hashPrefix, a.tlsKeyHash[:]...)
serialized = append(serialized, append(hashPrefix, a.appKeyHash[:]...)...)
return serialized
There was a problem hiding this comment.
The [:] is required because the AttestationHashes members are arrays, not slices.
There was a problem hiding this comment.
good point, edited suggestion above
There was a problem hiding this comment.
I actually like your first suggestion better, since it aligns the two calls and makes it more obvious what's different between the lines. See f04c24d.
Also move `hashPrefix` to a var so it can be initialized as a byte slice with better documentation of what the fields are. Review suggestion from Darnell Andries.
|
The corresponding brave-core change is in 1.54 scheduled for stable release July 18. I'll hold off on merging until then to avoid blocking other work. |
|
Could we please hold off for a few months? Even though 1.54 will be released in the coming weeks, it does take a while for the majority of clients to update (i.e. Android users) |
Integration changes from the separate nonce module
|
Looking at data from last week, we're about halfway through the transition (300k supporting multihash vs 400k not), so we should revisit after the next release. |
|
@DJAndries Are you ok with merging this now? The browser-side change has been deployed for ~7 months, and attestation has been disabled for about the same amount of time. |
i'm cool with it 👍 |
Great. Could you mark it as approved please, so I can merge? |
Because the
hashSeparatorcharacter can occur within the binary hash digest values representing the https cert and optional app key, it's not useful for dividing the user_data value into separate fields.Instead use the multihash encoding, which prefixes two bytes. The first (0x12) define the hash digest as SHA-256 just as the previous "sha256:" prefix did. The second (0x20) marks the digest length. This more compactly delimits the values while allowing the same extensibility.
Resolves #23