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

[connection] update path challenge according to the draft [JF & CC] #189

Closed
wants to merge 2 commits into from

Conversation

ElNiak
Copy link

@ElNiak ElNiak commented May 4, 2021

" Path validation succeeds when a PATH_RESPONSE frame is received that
contains the data that was sent in a previous PATH_CHALLENGE frame.
A PATH_RESPONSE frame received on any network path validates the path
on which the PATH_CHALLENGE was sent."

We try to modify the PATH_CHALLENGE part of the connection.py to follow this rule that we found with a testing tool Ivy. The problem before was that if the PATH_RESPONSE was received on a different path the challenge was considered as failed.

JF & CC

@jlaine
Copy link
Contributor

jlaine commented Jul 7, 2021

This looks like a good catch. Would you mind cleaning up the patch (removing commented code and debug statements)?

@jlaine
Copy link
Contributor

jlaine commented Dec 31, 2021

Hello @ElNiak any chance of getting the code into a shape I can merge it?

@ElNiak
Copy link
Author

ElNiak commented Jan 2, 2022

Hello, sorry we forget about that it will be done asap !

@jlaine
Copy link
Contributor

jlaine commented Feb 2, 2022

The PR seems to completely break the test suite would you mind updating it?

@@ -1147,6 +1162,17 @@ def _find_network_path(self, addr: NetworkAddress) -> QuicNetworkPath:
self._logger.debug("Network path %s discovered", network_path.addr)
return network_path

def _find_network_challenge(self) -> QuicNetworkPathChallenge:
# check existing network challenge
for idx, network_chall in enumerate(self._network_challenges):
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a complicated way of saying "always take the last challenge in the list"?

@jlaine jlaine added the changes requested Some changes are required before the PR can be merged. label Nov 16, 2023
@jlaine
Copy link
Contributor

jlaine commented Jan 7, 2024

Hi @ElNiak do you plan to complete the work on this PR?

@rthalley
Copy link
Contributor

Closing this in favor of the approach of [#483].

@rthalley rthalley closed this Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Some changes are required before the PR can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants