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

Jump to bottom

ngtcp2: fix QUIC transport parameter version #7771

Closed
wants to merge 2 commits into from
Closed

ngtcp2: fix QUIC transport parameter version #7771

wants to merge 2 commits into from

Conversation

katojunya
Copy link
Contributor

Bug details

In the ngtcp2 + OpenSSL combination, the QUIC transport parameter is set to duplicate version of QUICv1 and draft, as shown below. Some QUIC stack, for example facebook.com's mvfst, returns a parameter error for this Initial.

IETF QUIC
  Version: draft-29 (0xff00001d)
    .
    .
  TLSv1.3 Record Layer: Handshake Protocol: Client Hello
        Frame Type: CRYPTO (0x0000000000000006)
        Crypto Data
        Handshake Protocol: Client Hello
            Handshake Type: Client Hello (1)
            Extension: application_layer_protocol_negotiation (len=8)
      **====> Extension: quic_transport_parameters (drafts version) (len=55)**
      **====> Extension: quic_transport_parameters (len=55)**
	       .
	       .

As a result, curl returns Resource temporarily unavailable.

# curl --http3 -v https://www.facebook.com/
* Trying 2a03:2880:f10f:83:face:b00c:0:25de:443...
* Connect socket 5 over QUIC to 2a03:2880:f10f:83:face:b00c:0:25de:443
* connect to 2a03:2880:f10f:83:face:b00c:0:25de port 443 failed: Resource temporarily unavailable
* Trying 31.13.82.36:443...
* Connect socket 6 over QUIC to 31.13.82.36:443
* Connect to 31.13.82.36 port 443 failed: Resource temporarily unavailable
* Failed to connect to www.facebook.com port 443 after 40 ms: Resource temporarily unavailable
* Closing connection 0
curl: (7) Failed to connect to www.facebook.com port 443 after 40 ms: Resource temporarily unavailable

Cause and patch

The cause is a missing call to the OpenSSL API. This patch fixes the transport parameter version to use DRAFT version. I think it is probably due to OpenSSL bug.

Default QUIC version

This patch keeps that curl with ngtcp2 uses DRAFT version (h3-29).

I think QUICv1+h3 should be the default version in the near future. If default version is changed to QUICv1, SSL_set_quic_use_legacy_codepoint(qs->ssl, 0); and some modifies enables it.

@katojunya
ngtcp2: fix QUIC transport parameter version
834cdde 
fix inappropriate version setting for QUIC transport parameters.
this patch keeps curl with ngtcp2 uses QUIC draft version (h3-29).
@bagder bagder added the HTTP/3 h3 or quic related label Sep 24, 2021
bagder
bagder requested changes Sep 24, 2021
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Ugha, that's a long name that makes it cross the too wide barrier:

./vquic/ngtcp2.c:443:82: warning: Longer than 79 columns (LONGLINE)
                                    NGTCP2_TLSEXT_QUIC_TRANSPORT_PARAMETERS_DRAFT,

@katojunya
Copy link
Contributor Author

Thank you for reviewing my changes.

I modified the code to two-character indentation style, as with other functions with long arguments.

@bagder
Copy link
Member

bagder commented Sep 25, 2021

Thanks!

@bagder bagder closed this in 4a10a99 Sep 25, 2021
@katojunya katojunya deleted the fix-transport-parameter-version-draft branch September 26, 2021 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder requested changes

@tatsuhiro-t tatsuhiro-t tatsuhiro-t approved these changes

Assignees
No one assigned
Labels
HTTP/3 h3 or quic related
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@katojunya @bagder @tatsuhiro-t