Skip to content
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

Update identification of packet number space identifier #29

Closed
wants to merge 1 commit into from

Conversation

qdeconinck
Copy link
Contributor

Attempt to address #27, probably more text will be needed afterwards.

Attempt to address quicwg#27, probably more text will be needed afterwards.
@mirjak
Copy link
Collaborator

mirjak commented Oct 22, 2021

I believe this might need a few more updates in the rest of the text. Thee abandon frame support this already but there might be more text needed on the PN space section.

Copy link
Contributor

@huitema huitema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather keep it simple and require non null connection IDs for the MP variant. I think mixing 5 tuples into low level packet decryption and parsing is asking for trouble. Also, I am concerned that we are introducing here an untested variation, which might make the whole proposal weaker.

In any case, the text has to be specialized to the case in which the client, but not the server, use non null connection ID.

corresponds to the sequence number of the CID used. In the case one of the hosts uses
zero-length connection IDs, the packet number space identifier of its path corresponds to the
sequence number of the Connection ID used by its peer over the same 5-tuple. When both hosts
uses zero-length connection IDs, the packet number space identifier is 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree that this works.

For starter, it does not work if the server uses a zero length CID. It that case, in order to create a new path, a client sends a probe with a zero length CID. It arrives at the server on a five tuple that the server has not yet seen. How can the server know which encryption context it should use? The same issue would also happen in the case of NAT rebinding. The server sees a new five tuple, does not know what encryption to apply.

So, the first request is to specialize this text to the case in which the client uses zero length CID but the server does not.

Even in that case, using the five tuple is going to make client-side encryption offload significantly more complex. The offload engines will have to remember all the five tuples used in the connection, and the corresponding keys. This is a new requirement, and it may be difficult to meet by many implementations.

I think it would be much simpler to accept the limitation: using multiple number spaces requires that both sides use non-null connection IDs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree with your first point or to say it more generally, the host opening a new path needs to carry a CID.

Not entirely sure about the second point: Why do you think it is more complicated to path to a 5-tuple instead of matching the CID? Or do you mean that current offload engines only use the CID and this would simply be a change? Are these kind of offload engines already deployed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current API only uses the packet itself: look at the packet, find the number context, expand the number to 64 bits, attempt decryption. Using IP addresses makes this a bit more complicated, and introduces new failure modes such as variation of NAT. Using the peer's CID is also error prone. For example, in case of probes, the receiver has no idea of the value of the peer's CID.

See issue #96 for a discussion of options.

@mirjak
Copy link
Collaborator

mirjak commented Oct 23, 2021

Just as a matter of process: if we don't merge this PR (now) we need another PR (till Monday) that clearly specifies that in case of multipath_enabled=2 both endpoints MUST use connection IDs and if that is not the case the connection MUST be closed with an error. I guess that belongs in the negotiation section (as well as probably one sentence somewhere in the intro).

@mirjak mirjak added the For -01 label Oct 25, 2021
@mirjak mirjak removed the For -01 label Feb 9, 2022
When an endhost uses a non-zero-length Connection ID on a path, the Packet Number Space Identifier
corresponds to the sequence number of the CID used. In the case one of the hosts uses
zero-length connection IDs, the packet number space identifier of its path corresponds to the
sequence number of the Connection ID used by its peer over the same 5-tuple. When both hosts
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sequence number of the Connection ID used by its peer over the same 5-tuple. When both hosts
sequence number of the Connection ID used by its peer over the same 5-tuple.
This implies that if a connection ID is used by either host, only packets that carry
a connection ID can be used to open a new path. When both hosts

@mirjak
Copy link
Collaborator

mirjak commented Mar 3, 2022

Do we have reach any conclusion here or do we want to bring this up for further discussion on the list/during the meeting?

@huitema
Copy link
Contributor

huitema commented Mar 3, 2022

Quentin has opened issue #96, which is a good place for this discussion. I would rather table this PR, and not change the text before we agree on a solution to #96.

@mirjak
Copy link
Collaborator

mirjak commented Mar 3, 2022

@huitema not sure if this is the right order of things. This is a change/improvement to the multiple PNS approach. I think we want to know if/how this works in order to be able to make an informed decision about which approach to choose. In other words I think we should resolve this issue before we address #96. However, not sure we did reach agreement yet if this is actually an improvement and therefore we might bring this up for more working group discussion on the list or the next meeting.

@huitema
Copy link
Contributor

huitema commented Mar 3, 2022

I don't believe that making decryption dependent on evaluation of IP addresses is a good idea. There are too many ways in which this can lead to trouble, e.g., variations of load balancer and NAT configurations. I would table this PR.

@huitema
Copy link
Contributor

huitema commented Mar 3, 2022

I would personally like to have a decryption processing and ACK processing as simple ans straightforward as possible, to minimize risks of bugs, etc. Something like:

  • Number space is directly tied to DCID
  • MP-ACK are tied to number space, and identified by DCID of incoming packets

This leaves an ambiguity for the identifier in "Abandon PATH". I would be OK to simplify this to state either DCID used by peer, or some escape for "this path". That way we could see a simple reactive process:

  • receiver receives packet on path that it would rather not see in use anymore
  • if DCID in packet, receiver sends "ABANDON PATH(this DCID)"
  • else, receiver sends "ABANDON_PATH(this path)".

@mirjak
Copy link
Collaborator

mirjak commented Mar 4, 2022

@huitema I not sure I fully understand your proposal but it seems like a new/separate issue. Can you please open a new issue and if possible create an alternative PR?

@mirjak
Copy link
Collaborator

mirjak commented Jul 6, 2022

I believe this PR is OBE. @qdeconinck can we close it?

@qdeconinck
Copy link
Contributor Author

Given that #103 is now merged, we can close this now.

@qdeconinck qdeconinck closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants