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

Improve quic:alpn_information #368

Closed
rmarx opened this issue Jan 19, 2024 · 4 comments · Fixed by #385
Closed

Improve quic:alpn_information #368

rmarx opened this issue Jan 19, 2024 · 4 comments · Fixed by #385
Assignees

Comments

@rmarx
Copy link
Contributor

rmarx commented Jan 19, 2024

As reported by @hlandau on the mailing list:

quic:alpn_information:

RFC 7301 states that ALPN protocol strings are byte strings, not text.
How should ALPN protocol strings which are not valid UTF-8 or not
valid text be logged?

@LPardue
Copy link
Member

LPardue commented Jan 24, 2024

This issue is correct. Implementations that treat ALPN IDs as if they were strings can hit issues. Although qlog is not in the hot path, assuming sequence is safe to encode is not robust (especially as it is uncontrolled data).

My inclination would be to define a ALPN ID type for qlog. That could carry the raw bytes encoded as hexstring and/or the "friendly" representation if its safe. e.g.

ALPNIdentifier = {
  ? bytes: hexstring
  ? string: string
}

The reason being that most people would recognise h3 on sight, but scratch their heads about what 0x68 0x33 is.

Furthermore, while looking at the section, I think we should actually cite the ALPN RFC and tighten up the text at the same time.

@hlandau
Copy link

hlandau commented Jan 24, 2024

Personally I would just go with the hex encoding. Providing the ability to use either seems overkill. Not being able to eyeball a qlog file to read the ALPN might be annoying, but that will be the case anyway if an implementation does the robust thing and only serialises the hex encoding.

@marten-seemann
Copy link
Collaborator

Not being able to quickly read the "h3" ALPN from a qlog seems like a non-starter to me. I don't want to remember (or rather, look up 1000 times) what that is in hex.

@LPardue
Copy link
Member

LPardue commented Jan 24, 2024

Agree with Marten here.

HTTP headers might actually suffer the same issues now I think about it.

LPardue added a commit that referenced this issue Jan 28, 2024
ALPN IDs aren't guaranteed to be safe text, so allow logging the byte
sequence. However, don't force endpoint to do that if their willing
to do some safety checking or safe encoding.

Closes #368
LPardue added a commit that referenced this issue Feb 5, 2024
* Create ALPNIdentifier type to allow bytes or string presentation

ALPN IDs aren't guaranteed to be safe text, so allow logging the byte
sequence. However, don't force endpoint to do that if their willing
to do some safety checking or safe encoding.

Closes #368

---------

Co-authored-by: Robin Marx <rmarx@akamai.com>
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 a pull request may close this issue.

4 participants