[schwabe]

• Add rationale and background

  Add some background to explain what intention and design rationale behind some of the choices was

• Add request_id to probe request and probe reply

  This fixes the problem that when a client sends multiple probe requests it cannot distinguish which message caused which reply leading to guessing the actual RTT in case of packet loss.

• Drop message type

  This duplicates the TLVs and does not serve a real purpose in the protocol. It is better to treat the TLV as the messages.

• Clarify relationship to already existing TLV

  tls-crypt-v2 already uses a single TLV message. This message is compatible with the new TLV messages. Ensure that it is mentioned as well.

• Start numbering TLVs at 1

  Instead of having TLVs start at 0x200, we start now at 1 without the message type, there is also no confusion between different types of type.

• Clarify optional behaviour when messages contain malformed TLVs

• Reorder fields to align with byte boundary where logical

[ralflici]

I understand the intent is that CONTROL_DATA traffic is unlikely to require rotation, so a sender may remain on epoch 1 indefinitely in practice. However, making support for later epochs optional creates a latent failure: two peers can work correctly until one rotates, after which the other conformingly drops every CONTROL_DATA packet, with no ack or fallback mechanism.

Since the crypto derivation machinery is already implemented for the data channel and fixed epoch 1 does not save us much, maybe requiring the same epoch key semantics for CONTROL_DATA can make things simpler.

[ralflici]

I see you're reworded this section, thanks. But the following paragraph still says that implementations are only required to generate and accept epoch 1, which appears to contradict the new current-and-next rule.

Just to be clear, is the rule: MUST generate/accept 1 and SHOULD generate/accept 2?

Also there's a typ0 in "Implementation SHOULD implment multiple epoch..."

[syzzer]

Ralf has a good point about this approach allowing situations that work nicely and then fail for unclear reasons. That was also the case with the old situation, where only 1 was supported. It was not specced how to deal with rollover. How unlikely, it would be good to ensure that can´t happen. I see three options:

1. Require everyone to support multiple epochs. The downside is that a minimal userland implementation, that relies on DCO or hardware acceleration and therefor does not implement the data channel, still needs to implement the multiple epoch mechanism.
2. Specify that peers that do not implement epoch rollover MUST timely initiate a renegotiation (e.g. at ~90% of the max counter value).
3. As 2, but require that only epoch 1 is used. (Effectively the previous proposal, but specified how to deal with running out of counter values.)

(I realize this change comes from my comments earlier, so I'm to blame here.)

[ralflici]

Right, requiring the implementation to carry HKDF derivation machinery just for CONTROL_DATA is not really justified, so I agree now with options 2 or 3.

[syzzer]

What is the reason to use the same E_i here? That partly defeats the forward-secure aspect of the ratchet: assuming we stay at E_1, all older data channel keys of the current session can be derived from it.

I am aware that our current implementation keeps the source key material (the TLS master secret) in memory, so this currently already is the case. Still, specifying it this way prevents implementations from improving on that.

[syzzer]

If we revert to "allow only epoch 1", we could also fix this by specifying implementations SHOULD erase E_1 after deriving K_1.

[schwabe]

Okay, So currently E_1 is coming from the classic 256 byte cipher + auth "master key" from the send/recv cipher data from that.

Maybe our best option here is:

• Lets CONTROL_DATA use the same deriviation including the same "datakey upd", "data_key" and "data_iv" as the data channel but start with a different e1_send, e1_recv generated from a specific TLS key export like "EXPORTER-OpenVPN-controldata-keys" instead of "EXPORTER-OpenVPN-datakeys"

We could also change the the label during the key generation but I am not sure if that is useful or rather more annoying for implementing this.

[syzzer]

Agreed. Starting with a different TLS key export looks like the cleanest solution.

[syzzer]

Haven´t finished the full review yet, but some preliminary comments.

So far, this reads like a nice improvement.

[syzzer]

Interesting. Should we consider deprecating OCC_PING and implementing it over CONTROL_DATA instead. Or just use the new ECHO_REQ for that?

[schwabe]

We have to talk with our DCO people about this. The other thing is that OCC_PING is only sent if there is no other data channel traffic being sent, so a busy VPN will never see these packets while an idle VPN will send those at ping-interval.

So moving them to the user space would allow DCO channel to not implement these special data channel packet but at the cost of sending the user space ping packets all the time or have a protocol for the DCO module to trigger sending them.

[syzzer]

Right, thanks. Sounds like given the current situation it would just add more complexity. Let's leave this for now. Maybe we can think it over a bit more at the community meeting.

[schwabe]

I added a paragraph explaining the rationale to not replace the OCC_PING for now.

[syzzer]

it also lacks

Previously, session is was Capitalized.

[syzzer]

This own_session_id is the same ID

[syzzer]

Also, I'm struggling a bit to understand what you want to say here. Since we tie state to session id's, this needs to match the session id of other packet in the same session, right? So what do you mean with "is not required to be the same"?

[schwabe]

I tried to make it more clear that a client could use different IDs for probing latecy and initial connection could be used.

[syzzer]

Can you add the rationale behind this?

I'm guessing: if this TLV was set optional=0, and it intended to influence the behaviour of other TLVs in this message (the extension mechanism), we can no longer guarantee to correctly parse the other TLVs.

Or did you have other reasons?

[schwabe]

Mainly to have a predicatable behaviour of implementation what should happen if it encounters an non-optional TLV. If we allow the implementation to continue parsing the other TLVs in the message, the optional bit becomes meaningless since then the behaviour is the same as for optional=1.

[syzzer]

Ah, so you want to allow only related TLVs to be sent together in a single message? That is still partly holding onto the message concept, but I guess it could work and maybe simplify things. That would need to be specced somewhere though. (Or did I miss it?)

Until now, I was assuming we'd only require that related TLV be sent in a single message, but we would also allow sending unrelated TLVs.

[schwabe]

No not really but I think I need to think a bit more about this and adjust the text accordingly.

[syzzer]

I think the most important high-level remark I have is that the OOB-part to me seems more of a control channel protocol redesign than just adding a feature. There are some really good ideas in there, but if we actually treat it like a protocol redesign, I think we can do even better and prevent us from having to do another round later on.

We might be able to:

• Design a format that is better extensible in the future
• Move more details into the tls-crypt enveloppe, making fingerprinting less trivial
• Get rid of "key_id" for control channel messages
• And maybe more good stuff ;-)

[mfil]

Suggested change

	The responsibility of keep-alive could also be moved from the data channel to the control
	channel. That would however at the cost of sending the keep-alive packets all the time or
	have a protocol for the DCO implementation to indicate if other (data) packets on the data
	channel have sent that make the keep-alive unnecessary. Therefore, the OCC_PING packets are
	still the keep-alive mechanism that the protocol uses.
	The responsibility of keep-alive could also be moved from the data channel to the control
	channel. That would however come at the cost of sending the keep-alive packets all the time or
	have a protocol for the DCO implementation to indicate if other (data) packets on the data
	channel have been sent that make the keep-alive unnecessary. Therefore, the OCC_PING packets are
	still the keep-alive mechanism that the protocol uses.

Also, it's not entirely clear to me, but does this paragraph just say "we could do this thing but we won't"?

EDIT: After reading the discussion with @syzzer above, I now get why this is here, but I still think that the text flows weirdly. It goes like "Here's the new CONTROL_DATA packet type! It's kind of like this old thing. You might think that we're going to replace it with the new thing, but we won't". I think it's better to start with explaining what we actually want to use it for.

[mfil]

Suggested change

	Since neither tls-auth nor tls-crypt-the replay_packet_id are
	strictly tied to a connection but rather use a global state the client should use the same rules.
	This includes avoiding to reuse replay_packet_id ids and
	have ids going backwards.
	Since neither the tls-auth nor the tls-crypt replay_packet_id are
	strictly tied to a connection but rather use a global state the client should use the same rules.
	This includes avoiding to reuse replay_packet_id ids and
	have ids going backwards.

Also, could you add a link to the section where these rules are explained?

[schwabe]

I added a link to the section but we have come around to really explain the rules in the RFC yet.

[mfil]

How is the sender going to know if the message was accepted?

[mfil]

The first sentence seems to say that the client (do you mean implementation?) should parse the bytes that it expects to be there like it would normally and skip over the rest. But then you say that it should be treated as malformed.

Maybe you mean "Messages that don't specify this kind of extensibility" in the second sentence?

[mfil]

Doesn't the replay_packet_id already contain a timestamp when we use tls-{auth,crypt}?

[schwabe]

Kind of but not really. The replay_packet_id contains a timestamp but that timestamp is typically only updated when the program restarts or similar and the other half of the packet is counted up. So the time in there can go stale quite quickly. The other problem is that it is only a 32 bit time, so we would run into y2k38 problems, which I want to avoid. We might redefine the replay_packet_id for solving the y2k38 problem, so I don't want to add another 32 bit time dependency to the replay_packet_id format that might be harder to fix later.

[mfil]

Why do we need to do this sorting for every priority group? I thought we always pick a server that's tied for the lowest value.

[schwabe]

Well priority is the highest thing that we look at. If you have something like this

server A prio 20
server B prio 10
Server C prio 10
server D prio 5
server Eprio 5,

then you would first try to connect to D and E, then B and C and only then try A. The latency only orders them inside the same priority.

[mfil]

I'm not sure about the exact semantics of weight, but if you want a server with twice the weight to be picked twice as often, this doesn't work that way. I've read the Wikipedia article on SRV records and it says that if you have servers with weight 60, 20 and 20, you want the first one to be picked 60% of the time.

[schwabe]

I think you are right. My math only works if the number of servers is 2 or they all have the same weight.

[mfil]

Suggested change

	To ensure that that connect_lifetime has not passed, the client use use the transmission time of its own
	To ensure that that connect_lifetime has not passed, the client must use the transmission time of its own

[mfil]

Suggested change

	If less than connect_lifetime seconds have passed and the client can send from the same source IP and port,
	and use the same client session ID, the client MAY skip the handshake and start with third packet of
	the three-way handshake instead. In this case the client MUST behave as if the server send a
	CONTROL_HARD_RESET_SERVER_V2 with an ACK as response to the reset of the client. The client session
	id MUST be the same as the one of the probe request and the server session id MUST be the same as the session
	id from the probe reply.
	If less than connect_lifetime seconds have passed and the client can send from the same source IP and port,
	the client MAY skip the handshake and start with third packet of
	the three-way handshake instead. In this case the client MUST behave as if the server send a
	CONTROL_HARD_RESET_SERVER_V2 with an ACK as response to the reset of the client. The client session
	id MUST be the same as the one of the probe request and the server session id MUST be the same as the session
	id from the probe reply.

The client session ID was mentioned twice.

[mfil]

Shouldn't this be 1-7, because of the sequence_id?

[lstipakov]

I think this PR now mixes two separate things. The OOB probe is simple: a stateless request and reply before any session exists, no reliability layer, a lost probe is just a lost probe. CONTROL_DATA is an in-session channel with its own keys and epochs, and most of the recent discussion is about that.

Could we split it? Land the OOB probe part (probe request/reply, TLV framing, message handling) on its own, and move CONTROL_DATA and any wider control channel redesign to a separate PR. There are implementations of the probe part in openvpn2 and openvpn3 waiting for it to settle. If the control channel format changes later, OOB can move with it.

[schwabe]

@lstipakov the control data is the simple thing. It is not really blocking anything.

And the problem with moving the wider protocol design to another PR does not work right now because we need to get at least get to a point where know that the new introduced format OOB packet format does not cause problems for a wider protocol redesign and we have a OOB packet format that is then deprecated shortly after it has been introduced.

You can help in the discussionn and design or just be a little more patient.

[schwabe]

I think the most important high-level remark I have is that the OOB-part to me seems more of a control channel protocol redesign than just adding a feature. There are some really good ideas in there, but if we actually treat it like a protocol redesign, I think we can do even better and prevent us from having to do another round later on.

We might be able to:

• Design a format that is better extensible in the future
• Move more details into the tls-crypt enveloppe, making fingerprinting less trivial

So currently we have in the envelope the following:

struct control_tlscrypt_v2 {
int opcode:5;
int keyid:3;
uint64_t own_session_id;
uint64_t replay_packet_id;
uint8 auth_tag[32];
uint8[] enc_tlv_payload;
}

• auth_tag and opcode I think need to stay in the unencrypted part. auth_tag for obivous reason and opcode since we otherwise have a hard time keeping compatiblity
• Without own_session_id in the cleartext, it would a lot harder to correctly identify a client as the only thing we would have is the IP/Port of the clien.t

So that leaves basically only replay_packet_id as something we could encrypt as well. My gut feeling says that we should rather postpone that format change to something like a tls-crypt v3 format with other changes like allowing more flexibility in algorithm (e.g. have AES-SIV/AES-GCM-SIV as alternative to our current AES wrapping)

• Get rid of "key_id" for control channel messages
• And maybe more good stuff ;-)

[ralflici]

This looks like a duplicate of part of the previous paragraph. I think a small table covering probe_only and zero/nonzero connect_lifetime would make the state machine substantially easier to understand and avoid the duplicated rejection wording.

[syzzer]

Agreed. Also because I think there is currently an ambiguity: iiuc, the current draft allows server implementations to answer probes without fully supporting the CONTROL_V2 handshake. In that case, a client currently can not distinguish between "server refuses handshake, go away" and "server want to continue with CONTROL_V1".

So we'd need to require probe-answering servers to support the CONTROL_V2 handshake (my preference, keep the combinatorial matrix smaller) , or find a way to differentiate between the two scenarios. The table is a good plan either way.

[ralflici]

Something's missing in this sentence? I just can't figure out what though.

[syzzer]

Agreed. Also because I think there is currently an ambiguity: iiuc, the current draft allows server implementations to answer probes without fully supporting the CONTROL_V2 handshake. In that case, a client currently can not distinguish between "server refuses handshake, go away" and "server want to continue with CONTROL_V1".

So we'd need to require probe-answering servers to support the CONTROL_V2 handshake (my preference, keep the combinatorial matrix smaller) , or find a way to differentiate between the two scenarios. The table is a good plan either way.

[syzzer]

It looks like the current specification no longer includes the client session ID in the second CONTROL_V2 handshake packet (probe reply), but just the server session ID. The HARD_RESET_SERVER_V2 does include the 64-bit client session ID. That would reduce the 64-bit resistance we currently have against off-path attackers to guessing the reply to the 32 bits of the request id.

I can see two ways to go from here:

1. Stick to the original approach, and include the peer session ID unconditionally in the second packet (to prevent reply forgery) and third packet (used as HMAC cookie). Could just be a "Session ID echo TLV" (better name welcome).
2. Increase the request_id to 64 bits, leave the peer session ID out of the second packet (request_id now offers the same protection), and include an "HMAC cookie TLV" in the third packet that echo's the server session ID. This shaves of a few bytes, but requires the probe request/reply to always be present in the first two handshake packets to prevent reply forgery. And makes the handshake deviate more from the current handshake.

As a side note: we can use this chance to no longer always send the peer session id with the ACK block (as we do now). I think that was only really needed for packet handshake packet 2/3, but we also send for any later packet that has ACKs.

[schwabe]

I think we should go for the 64 bit request_id here to avoid having a weaker resistance here and keep otherwise the protocol the same to avoid having too many changes in the time when we have the new CONTROL_V2 packets for the handshake but still use the CONTROL_V1 for the rest of the control channel.

[syzzer]

Okay, that resolves the resistance, as long as we are clear about requiring the probe TLVs to be present in the first two packets and how they should be parsed. Maybe something like this in the CONTROL_V2 (or relationship) section:

A CONTROL_V2 packet that starts a new session MUST contain exactly one probe_request TLV. A server MUST silently drop a CONTROL_V2 packet from an unknown session that does not contain a probe_request. The server's reply MUST contain exactly one probe_reply echoing the request_id. The client MUST only continue the handshake with a probe_reply whose request_id matches an outstanding probe_request.

[syzzer]

Do we need this at all? Probes are intended to be sent to many servers, so keeping state at the server side seem like a bad idea. Also, the client knows it used tls-crypt-v2, and you already specify that all client MUST support replying with the WKc. Can't we just say the client MUST always include the WKc when completing the handshake?

[syzzer]

Also, let's make explicit that the Early Negotiation TLV is only valid for V1, to ensure nobody tries to use that for the WKc resend signalling and we can phase that one out at some point.

[schwabe]

This is mostly written to allow probing on TCP sockets. There it cannot be truly stateless and giving the server a hint if the socket can be closed or not is something I think we want for that usecase.

[syzzer]

I think I'm missing something here. How does indicating whether to include the WKc in the third packet relate to closing the TCP socket?

Is it that when the server sets this bit, it will close the TCP socket after the reply?

Then too, would it not be simpler to require a handshake initiated using tls-crypt-v2 + CONTROL_V2 to simply always include the WKc in packet 3?

[ralflici]

I agree with @syzzer here: requiring unconditional WKc makes both the implementation and the protocol simpler. The only benefit I can think of is the packet size (leaving more room for the TLS ClientHello) but that cost is already handled by the existing control-channel fragmentation logic.

[syzzer]

So I was thinking more about the key_id, and why we actually need it in the control channel. For the (unreliable) data channel and CONTROL_DATA we actually need it, because there we do accept packet encrypted with the LAME_DUCK key. But for the control channel we actually don't need it in the header.

In the current openvpn master (74ba5863), the old control channel is directly rejected when the renegotiation starts:

• The side triggering soft reset moves the primary key_state to lame duck as soon as it decides to renegotiate (tls_process() at ssl.c:3166 calls key_state_soft_reset()). The other side does the same when it accepts the soft reset (ssl.c:3943).
• From then on, incoming control packets with the old key_id are rejected by tls_pre_decrypt() (ssl.c:4038).
• Outgoing control channel messages during the renegotiation are queued until S_ACTIVE by tls_send_payload() (ssl.c:4228).

So the control channel pauses for the whole renegotiation, and we might actually loose messages during reneg:

• Anything the peer sends under the old key_id before it sees the SOFT_RESET is rejected.
• Unacked messages of the old key_state are not retransmitted.

So a PUSH_UPDATE just before a renegotiation might be lost.

Since there's only ever one active control channel anyway, can't we just simplify things and use one reliable channel per session:

• The reliability layer really becomes just a single stream below TLS. A renegotiation is just a message with a TLV with the new key_id. (We still need the key_id for the data channel.)
• Because the single reliable layer guarantees in-order delivery, we no longer drop messages. Everything before the reneg TLV belongs to the old TLS session, everything after it to the new one.
• With that, we no longer need key_id in the CONTROL_V2 header.

We would need to think about dynamic tls-crypt. Do we keep it? Then we need to signal when we switch keys.

The cost on the implementation side: the reliable state (send_reliable, rec_reliable, rec_ack, lru_acks) moves from key_state to tls_session.

(And for this RFC section, we should probably describe the actual behaviour: we stop processing control packets with the old key_id once renegotiation starts. New control channel messages are queued for the new session.)

[schwabe]

Because the single reliable layer guarantees in-order delivery, we no longer drop messages. Everything before the reneg TLV belongs to the old TLS session, everything after it to the new one.

But you still need some clear marker since mixing data from the old with the new session will not really work. Otherwise you might feed a TLS record from the old session into a new TLS object that is confused because that is not a valid response to a ClientHello. And then you still have to either feed the data packet to the old session or throw it away.

We would need to think about dynamic tls-crypt. Do we keep it? Then we need to signal when we switch keys.

dynamic tls-crypt also has the effect to strongly tie the renogotiation to the originial session and not let another client take over "in the right moment" to potientially skip MFA.

We might also take a step and ask ourselves if we really need the renegotiation at all.

• data channel key rotation: can also done by peers periodically increase their epoch
• recheck if certificates are still available on the peer. This also only really makes sense if certificates are in a secure device
• rechecking user/password: questionable since clients save username/password and also often sessions use session user/password instead
• period authorisation checks via management/scripts: Can be replaced by a simple timer.

[syzzer]

Okay, all of this was assuming a complete switch to CONTROL_V2. Which we won't do just yet. So we should really discuss this, including the other valid points you raise, when we prepare for the next step. And leave it for now.

[syzzer]

Improved a lot again. A have a few remaining points to consider, but I think it is almost ready for implementation by our own core devs.

I focused on content mostly, not language. We can do a language round in a later MR when the content is settled.

[syzzer]

Okay, that resolves the resistance, as long as we are clear about requiring the probe TLVs to be present in the first two packets and how they should be parsed. Maybe something like this in the CONTROL_V2 (or relationship) section:

A CONTROL_V2 packet that starts a new session MUST contain exactly one probe_request TLV. A server MUST silently drop a CONTROL_V2 packet from an unknown session that does not contain a probe_request. The server's reply MUST contain exactly one probe_reply echoing the request_id. The client MUST only continue the handshake with a probe_reply whose request_id matches an outstanding probe_request.

[syzzer]

Should the client use the same or a new session ID for the fallback?

For stateless servers, it doesn't really matter I guess. But for stateful servers, how should they handle receiving both a probe and hard reset? I think p2p mode would currently replace the first session in TM_INITIAL with the second when the hard reset arrives with a new session id.

Maybe we should require servers that accept probes to reply to probes in a stateless way. Or require stateful servers to treat the second attempt (hard reset) as part of the same handshake, which would require specifying that the second packet uses the same session ID.

[syzzer]

Also a check question for you to think about: should a server that replied to both the probe and hard reset be able to see from the CONTROL_V1 packet (third handshake packet) from which reply is originated? Or is that irrelevant?

It feels right to tie the third packet to the earlier packets of the handshake, but I can't really come up with a reason that would require it in this use case.

If we want that: should we suggest that the server chooses the session ID HMAC such that it can distinguish between the two cases? (E.g. hash in a bit that represents the handshake type, verify HMAC for both options. Or use one bit of the session ID to indicate the version.)

[syzzer]

I think I'm missing something here. How does indicating whether to include the WKc in the third packet relate to closing the TCP socket?

Is it that when the server sets this bit, it will close the TCP socket after the reply?

Then too, would it not be simpler to require a handshake initiated using tls-crypt-v2 + CONTROL_V2 to simply always include the WKc in packet 3?

[syzzer]

Okay, all of this was assuming a complete switch to CONTROL_V2. Which we won't do just yet. So we should really discuss this, including the other valid points you raise, when we prepare for the next step. And leave it for now.

[syzzer]

All the what used in probing?

[ralflici]

That covers handshake continuation, but not latency probing. I suppose an unmatched reply should be ignored for all purposes, including RTT, reachability, and candidate selection.

So, in more general terms, what about something like "A client MUST ignore a probe_reply unless its request_id matches an outstanding probe_request."

[ralflici]

nit: This clarifies the purpose of bit 2 and this last sentence describes the common use case. (Not a blocker but) maybe specifying the server-side requirement for bit 2 in a more normative way could help clarifying the necessary guarantee. For example something like: "A server MUST set bit 2 only when the session state, or the keys required to validate its stateless cookie, are shared across all endpoints covered by bit 1."

[ralflici]

I guess the same parameters is intended here. Merely saying that the parameters are configured does not establish their equivalence.

[ralflici]

nit: CONTROL_CLIENT_HARD_RESET_V2/V3 -> CONTROL_HARD_RESET_CLIENT_V2/V3

[ralflici]

nit: probe_response -> probe_reply

[ralflici]

nit: duplicate "only"

[ralflici]

I agree with @syzzer here: requiring unconditional WKc makes both the implementation and the protocol simpler. The only benefit I can think of is the packet size (leaving more room for the TLS ClientHello) but that cost is already handled by the existing control-channel fragmentation logic.