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

hysteria2, grpc: fix the use of GetTLSConfig #3176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dyhkwong
Copy link
Contributor

@dyhkwong dyhkwong commented Oct 2, 2024

When reviewing #3162 (comment), I found that hysteria2 and grpc dialer do not fill ServerName with server address if ServerName is not specified. All other dialers use config.GetTLSConfig(tls.WithDestination(dest)) or security.OptionWithDestination{Dest: dest}).

@dyhkwong
Copy link
Contributor Author

dyhkwong commented Oct 2, 2024

Another issue: If a server uses IP certificate, and tlsSettings.serverName not specified on client side, the client will report error tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config. Client must fill tlsSettings.serverName with the IP to be able to connect. Is it expected behavior or a bug?

@xiaokangwang
Copy link
Contributor

xiaokangwang commented Oct 2, 2024

Client must fill tlsSettings.serverName with the IP to be able to connect.

I think it is more or less unintented and could be changed if necessary. Other than contacting https://1.1.1.1 , I don't see a obvious way it could be used.

If such change was necessary, we should have a look at how it impacts SNI .

Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I believe this pull request is ready to be merged before next minor release.

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.

2 participants