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

Add SNI to protocols.csv #138

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Add SNI to protocols.csv #138

merged 1 commit into from
Sep 8, 2022

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Sep 8, 2022

This registers the SNI component which can be used by transports to set the sni field in their client hello. See RFC 6066 section 3 for more details.

We need this so that secure websocket clients can know what SNI to send when connecting to an ip address.

@@ -21,6 +21,7 @@ code, size, name, comment
446, V, garlic64,
447, V, garlic32,
448, 0, tls, Transport Layer Security
449, V, sni, Server Name Indication RFC 6066 § 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it great that we use base 10 here and base 16 in go-multiaddr? 🥺

@MarcoPolo MarcoPolo merged commit 64f2586 into master Sep 8, 2022
@MarcoPolo MarcoPolo deleted the marco/sni branch September 8, 2022 14:05
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This change looks good to me.

As far as I can tell, this is missing an update to https://github.com/multiformats/multicodec/blob/master/table.csv, correct? @MarcoPolo can you do the update?


On a general note, I would suggest keeping pull requests open for longer than 12h before merging. That would allow everyone in the community to comment, no matter which timezone they are in. Based on past experience, pull requests to multiformats/multiaddr are usually not urgent, thus the delay would not hurt productivity.

Would folks agree with the above suggestion?

@MarcoPolo
Copy link
Contributor Author

As far as I can tell, this is missing an update to https://github.com/multiformats/multicodec/blob/master/table.csv, correct? @MarcoPolo can you do the update?

Ah thanks. I didn't realize we had these two sources of truth. That doesn't seem ideal. I wonder if there's a strategy we can use to keep them in sync. Maybe a CI check in this repo?

On a general note, I would suggest keeping pull requests open for longer than 12h before merging. That would allow everyone in the community to comment, no matter which timezone they are in.

Sounds good to me. What should the timeout be? 24 hours?

This change I considered pretty uncontroversial because there's been an sni example in the readme for 5 years. So it's more surprising that this didn't already exist. But in general I agree.

I'll make a PR template to make these things explicit.

achingbrain pushed a commit to multiformats/js-multiaddr that referenced this pull request Mar 6, 2023
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