-
Notifications
You must be signed in to change notification settings - Fork 5.9k
bip324: Remove garbage authentication packet (breaking change) #1498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
ACK, though I want to wait for some more opinions - the BIP is fine as is, and I'm not sure whether this minor improvement is worth the breaking change at this point. For a bit of history, this was a conscious decision earlier, to separate the garbage phase from the version negotiation phase. But arguably, the result isn't all that consistent either. The idea was to use the "packet" interface for everything after garbage terminator, but the abstraction is not perfect: packets have a decoy bit, but the decoy bit of the garbage authentication is ignored. In retrospect, seeing both specification and (one) implementation, it appears simpler to merge the auth packet and version packet. As for conceptual way of looking at it, I believe this makes sense:
|
|
ACK
Should be clearer saying that a
Kinda confusing now that |
b6f04a4 to
3d0f725
Compare
Rephrased and restructured this to improve clarity.
Happy to improve here, but I can't follow. It's still 3 phases altogether, and the first two are 1.5 roundtrips. What do you find confusing? |
Want to leave a review or (Concept) ACK also in bitcoin/bitcoin#28525 then? :) |
It was not immediately clear why garbage authentication is not in the list following |
|
Concept ACK -- fewer states seems like a win, and it seems like the simpler this is, the more easily it could be used for other applications The 1.5 round trip handshake could arguably be more obscure if described/implemented as:
(otherwise the "garbage-terminator + version packet" may be identifiable as a fixed size response?) Does it make sense to have a decoy packet before the version packet rather than after it? The version packet tells you how to interpret the header, so it seems slightly odd to be parsing headers before you've actually figured out what the header format is. Perhaps it would it be simpler but still workable to just have the first packet after the garbage terminator always be the version packet, and have its entire contents (with no header) be interpreted as the version negotiation? In that case, an initial "empty" version message could literally be empty, with no header and no content. In that case after the first version message, you'd pass every message's content to a version-specific decoder that splits out the header, and routes the body to the appropriate application handler. (In order for the responder to be able to append decoy packets before receiving the initiator's version packet, you'd still need to guarantee that packets with the high-bit of the first byte are ignored no matter what version ends up being negotiated) It might be slightly clearer to make the pseudocode more stateful as far as the aad info goes, ie drop it as a parameter to |
Just for my understanding: You're saying that the handshake could look more obscure to wiretappers, right? And not that the description of the handshake in the BIP would be more obscure to readers? ;)
I don't think so. The "garbage-terminator + version packet" is always preceded by some form "garbage". So if you want to get rid of the fixed-size response, you can also just send garbage. And yes, if you need more bytes, you can always send more decoys after the version packet, but I don't think this needs to be in the handshake description. (You can always send decoys.)
I agree that it seems odd to parse headers before you've agreed on a version. And @sipa had similar remarks when we talked about this somewhere else. The thing is that we bootstrap communications, so we build up the layers step by step. And yes, you can argue that at the point when we've just received the garbage terminator, we have not agreed on a header format yet. And I agree that the possibility to send decoy packets before the version packet doesn't really add functionality. Instead of ignoring the ignore bit (what we do in the current BIP), we could also get rid of this by dropping the header byte entirely (what you suggest). Another way is to simply disallow packages where the ignore bit is set. Though, I don't believe that this will make things simpler. It's adding back a special rule again. So I think if we agree that it makes sense to drop the explicit garbage authentication packet and get rid of one state in the state machine, then I see the following possibilities after the garbage terminator.
Among these approaches, I like 1 and 3 most. I think approach 1 (in this PR) is conceptually nice because it decouples the AAD stuff from the protocol states. You simply keep a variable around that tells what you expect as AAD in the next received packet [1]. Approach 3 does not need this variable and just needs an
Will probably give this a shot later. [1] Note that when you wrote your comment here, the way I was implementing this (https://github.com/real-or-random/bitcoin/blob/8753b6af9bf8767be50d71d1bf7e3711738fcfd2/src/net.cpp#L1239-L1278) technically did not achieve this decoupling yet. I've improved this now in the implementation PR this by renaming the |
|
The purpose of having decoy functionality available during the VERSION phase is mostly for future extensions that may extend the number of messages that can be sent in that phase. I agree it's rather pointless right now, but with the prospect of extensions, I think it's cleaner to have it available for the entirety of the phase. Just as a note, if we'd go for an approach with 2 layers, where some parts use headerless packets and other parts use headerful packets, the length descriptor semantics need to change too (because they're currently excluding the header byte length). @ajtowns I don't believe there is a benefit to splitting up the garbage into a part sent before and after the other side's key; whatever you sent as garbage part 2 can instead be sent as decoy data (even if only possible after the version packet). |
Yeah, that's what I mean, and your further reply makes sense. Still kinda think mentioning decoy packets as well as garbage in the 1.5 round trip summary would be clearer; but the rest of the BIP already makes it pretty clear anyway, so 🤷♂️
Hmm, I guess I look at the layers as:
ie, I guess I think "negotiate for future extensibility" is essentially a "meta application" that's built into the bip324 (or future extension bip) implementation, that while it gets data from packets just like a regular application, also controls how data from packets goes to applications, so it's really a low-level thing, not a high-level thing. Maybe "built-in as part of the implementation" vs "user of the API" is more the distinction that I'm thinking of.
Yeah, I guess that makes sense: better to be able to have a full 0xFF_FFFF bytes of application data in a packet than be off by two instead of just one. I also guess that's easy enough to extend in some future version if it's ever desirable:
LGTM 👍 |
|
I'm really not convinced that separating headerless and headerful packets is worth it. At a conceptual level you can say:
So while it does make sense to see the version negotiation as the thing that negotiates there being a decoy mechanism, and so logically that phase itself shouldn't already have them, I don't think this practically gains us anything:
|
8738bc8 to
2a32d65
Compare
|
I've forced-push to address the comments. I'm undrafting the PR because there seems to be broad agreement that we want to do this.
Ok, that makes sense. We mentioned it everywhere in the BIP except in this summary. Added it to the summary for consistency.
I tried this, but I'm not convinced that this will make it clearer: Keeping the state local to |
2a32d65 to
ddb6a5c
Compare
See also bitcoin/bips#1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing.
|
LGTM |
|
ACK |
| #* At this point, both parties have the same keys, and all further communication proceeds in the form of encrypted packets. Packets have an '''ignore bit''', which makes them '''decoy packets''' if set. Decoy packets are to be ignored by the receiver apart from verifying they decrypt correctly. Either peer may send such decoy packets at any point after this. These form the primary shapability mechanism in the protocol. How and when to use them is out of scope for this document. | ||
| #* At this point, both parties have the same keys, and all further communication proceeds in the form of '''encrypted packets'''. | ||
| #** Encrypted packets have an '''ignore bit''', which makes them '''decoy packets''' if set. Decoy packets are to be ignored by the receiver apart from verifying they decrypt correctly. Either peer may send such decoy packets at any point from here on. These form the primary shapability mechanism in the protocol. How and when to use them is out of scope for this document. | ||
| #** For each of the two directions, the first encrypted packet that will be sent in that direction (regardless of it being a decoy packet or not) will make use of the associated authenticated data (AAD) feature of the AEAD to authenticate the garbage that has been sent in that direction.<ref name="why_garbage_auth">'''Why does the protocol authenticate the garbage?''' Without garbage authentication, the garbage would be modifiable by a third party without consequences. We want to force any active attacker to have to maintain a full protocol state. In addition, such malleability without the consequence of connection termination could enable protocol fingerprinting.</ref> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what kind of fingerprinting will be possible. Maybe it's just me. Can you explain in more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be very uncommon for a fully encrypted protocol not to abort in case of an attacker modifying data on the wire. So as an attacker, you could flip some bits in the garbage, notice that the connection is not dropped, and then conclude that v2 is running. (edit: Note that I'm touching this sentence.)
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
One thing you may want to improve is making it clear that garbage-terminator is not a part of garbage when doing aad. This is something i had to double-check everywhere manually on my own :) A BIP could be explicit about this. |
|
Concept ACK, but haven't studied the BIP changes, only the code change. |
by merging it with the version packet. Or more accurately, by merging it with the first packet sent after garbage termination, which may be a decoy packet or the version packet. The new protocol simplifies implementations: - A protocol state machine won't need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing. The reason for having a separate garbage authentication packet was to materialize the separation of the key exchange phase and version negotiation phase even in the bytestream on the wire. However, this is not necessary, and arguably, these phases are still properly separated: Since the AEAD will ensure that AAD (=garbage) is checked before looking at the contents (=version), the peers won't interpret version data before having authenticated the garbage.
ddb6a5c to
75dc363
Compare
Added "not including the garbage terminator" at an appropriate location in the text. |
Sjors
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 75dc363 modulo some example code suggestions
| send(v2_enc_packet(peer, decoy_content_len * b'\x00', aad=aad)) | ||
| aad = b'' | ||
| # Send version packet. | ||
| send(v2_enc_packet(peer, TRANSPORT_VERSION, aad=aad)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
aad = b''Maybe also rename to send_aad.
| v2_receive_packet(peer) | ||
| # Receive, decode, and ignore version packet. | ||
| # This includes skipping decoys and authenticating the received garbage. | ||
| v2_receive_packet(peer, aad=received_garbage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, the v2_receive_packet helper now includes assumptions that only make sense in the handshake.
Maybe instead it can return a type and content? And also not do aad = b''.
So then this code becomes something like:
# Receive, decode, and ignore version packet.
# This includes skipping decoys and authenticating the received garbage.
aad = received_garbage
while true:
(type, content) = v2_receive_packet(peer, aad=aad)
aad = b''
break if type == VERSION
throw if type != IGNOREThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, the
v2_receive_packethelper now includes assumptions that only make sense in the handshake.
Does it? Can you spell them out explicitly?
If you're referring to "Only the first packet is expected to have non-empty AAD.", then I think this will also be the case if we ever use AAD for anything else than garbage. Even in that case, you'd always want to skip decoy packets and it's reasonable to include the AAD in the first packet. This assumes that the AAD will always be determined by the received (non-decoy) packets or the outer header (see my response in the Core PR: bitcoin/bitcoin#28525 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on how the AAD is used, which you elaborated on here: bitcoin/bitcoin#28525 (comment)
If the AAD is set at the level of traffic shaping then it might be different for decoy and real messages. If it's only used at the application level then I agree it's probably fine to set it in the first (decoy) message.
If we believe the latter is much more likely, then I suppose a simple comment in the helper function would suffice:
# This includes skipping decoys and authenticating the received garbage.
# We assume that for future AAD uses, beyond the handshake, its contents
# is determined before a package is received and is checked in the first
# message, even if that's a decoy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truth is, we have no idea how the AAD would be used in future extensions, or even whether it'll ever get used for anything but garbage authentication.
In light of that, it feels unnecessary to impose a particular "grand design" for future extensions or their interaction with AAD and explain it in the specification. We simply pick a particular design because it's concise and can be rationalized, but I don't think we need to specify future behavior - that feels like an analogue of premature optimization (premature specification?).
"Set the expected AAD when processing garbage + terminator, verify the expected AAD whenever any packet is received, and reset the expected AAD to nothing after verifying a packet" is one concise and simple description, and I can imagine that it remains usable for hypothetical extensions that appropriate AAD for other purposes. And if not, those extensions can just change the specification.
|
I consider this ready for merging. @luke-jr @kallewoof |
|
ACK 75dc363 fwiw |
Editorial change after discussion in bitcoin#1498.
e3720bc net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing) b0f5175 net: Drop v2 garbage authentication packet (Tim Ruffing) Pull request description: Note that this is a breaking change, see also bitcoin/bips#1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing. ACKs for top commit: naumenkogs: ACK e3720bc sipa: ACK e3720bc. Re-ran the v2 transport fuzzer overnight. ajtowns: ACK e3720bc - simpler and more flexible, nice achow101: ACK e3720bc Sjors: utACK e3720bc theStack: Code-review ACK e3720bc Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
e3720bc net: Simplify v2 recv logic by decoupling AAD from state machine (Tim Ruffing) b0f5175 net: Drop v2 garbage authentication packet (Tim Ruffing) Pull request description: Note that this is a breaking change, see also bitcoin/bips#1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing. ACKs for top commit: naumenkogs: ACK e3720bc sipa: ACK e3720bc. Re-ran the v2 transport fuzzer overnight. ajtowns: ACK e3720bc - simpler and more flexible, nice achow101: ACK e3720bc Sjors: utACK e3720bc theStack: Code-review ACK e3720bc Tree-SHA512: 16600ed868c8a346828de075c4072e37cf86440751d08ab099fe8b58ff71d8b371a90397d6a4247096796db68794275e7e0403f218859567d04838e0411dadd6
| </pre> | ||
| Upon receiving the encoded responder public key, the initiator derives the shared ECDH secret and instantiates the encrypted transport. It then sends the derived 16-byte <code>initiator_garbage_terminator</code> followed by an authenticated, encrypted packet with empty contents<ref name="send_empty_garbauth">'''Does the content of the garbage authentication packet need to be empty?''' The receiver ignores the content of the garbage authentication packet, so its content can be anything, and it can in principle be used as a shaping mechanism too. There is however no need for that, as immediately afterward the initiator can start using decoy packets as (a much more flexible) shaping mechanism instead.</ref> to authenticate the garbage, and its own version packet. It then receives the responder's garbage and garbage authentication packet (delimited by the garbage terminator), and checks if the garbage is authenticated correctly. The responder performs very similar steps but includes the earlier received prefix bytes in the public key. As mentioned before, the encrypted packets for the '''version negotiation phase''' can be piggybacked with the garbage authentication packet to minimize roundtrips. | ||
| Upon receiving the encoded responder public key, the initiator derives the shared ECDH secret and instantiates the encrypted transport. It then sends the derived 16-byte <code>initiator_garbage_terminator</code>, optionally followed by an arbitrary number of decoy packets. Afterwards, it receives the responder's garbage (delimited by the garbage terminator). The responder performs very similar steps but includes the earlier received prefix bytes in the public key. Both the initiator and the responder set the AAD of the first encrypted packet they send after the garbage terminator (i.e., either an optional decoy packet or the version packet) to the garbage they have just sent, not including the garbage terminator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since there's no limit on the number of decoy packets that a peer can send (after sending the garbage terminator, before sending the version packet), wouldn't we want to disconnect from the handshake at some point - so that the connection can be dropped and we use that slot for a more useful peer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @sipa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stratospher We certainly do want to disconnect useless peers, but I don't think the transport layer is the right place to address that. After all, peers can be equally useless by just behaving weirdly at the application level (e.g. never do anything except respond to pings). In Bitcoin Core, connection rotation and inbound eviction already deal with these cases (but can be improved, of course), and apply just fine here, because if a node only sends decoys, the application layer will just see a connection that never sends a version/verack packet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! makes sense.
See also bitcoin/bips#1498 The benefit is a simpler implementation: - The protocol state machine does not need separate states for garbage authentication and version phases. - The special case of "ignoring the ignore bit" is removed. - The freedom to choose the contents of the garbage authentication packet is removed. This simplifies testing.
by merging it with the version packet. Or more accurately, by merging it with the first packet sent after garbage termination, which may be a decoy packet or the version packet.
The new protocol simplifies implementations:
The reason for having a separate garbage authentication packet was to materialize the separation of the key exchange phase and version negotiation phase even in the bytestream on the wire. However, this is not necessary, and arguably, these phases are still properly separated: Since the AEAD will ensure that AAD (=garbage) is checked before looking at the contents (=version), the peers won't interpret version data before having authenticated the garbage.