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

Removed attempt to connect to server info host when TLS is enabled #500

Merged

Conversation

brooksmtownsend
Copy link
Contributor

After some back and forth with @Jarema , I was having some trouble connecting to connect.ngs.global with the nats client. I was getting an error for UnsupportedNameType, which I tracked down in rustls. When I took a look at this function, it seems that attempting to connect to NGS always resulted in the info.host variable filled in as an IP address, which as you can see in the above function creates an error.

Simply removing this fixes the issue for me but I wanted to open this PR for discussion. I'm not entirely sure if connecting to an IP address with TLS is a totally valid use case, or if ignoring the info.host value here is the proper solution. What also worked for me is reordering the connections to try creating a server name from tls_host first, but that's not really solving much more than just removing the option.

Anyways, looking forward to discussing this. If we wanted to make this a bit more robust, I can create a ServerName from both values and match to ensure that the value is a ServerName::DnsName rather than just taking tls_host

@brooksmtownsend brooksmtownsend changed the title Removed attempt to connect to server host Removed attempt to connect to server info host when TLS is enabled Jun 14, 2022
@philpennock
Copy link
Member

The correct behavior, for the NATS protocol, per the Go behavior is: if you connected to a NATS server with a DNS hostname, and the server advertises reconnection URLs to you which use IP addresses, then reuse the original hostname as the verification hostname: that has to remain valid across reconnection attempts. Only change the hostname which you validate if the server gave you a connection address which has a hostname in it.

@brooksmtownsend
Copy link
Contributor Author

Addressed @philpennock's note in my latest commit, the behavior is

  1. parse hostname that we validate if the server gives us a new hostname
  2. fallback to tls hostname IF it's a DNS hostname
  3. error if not

Note that as far as I can tell, this new logic will prevent TLS connections to IP addresses at the async_nats layer, but regardless it's an error as specified above in the rustls library so it couldn't go through either way.

@philpennock
Copy link
Member

LGTM as someone who knows about NATS/security/TLS, but I'm carefully not approving as I'm not a nats.rs owner.

@caspervonb caspervonb requested review from caspervonb and Jarema and removed request for caspervonb June 14, 2022 22:14
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Looks good to me.

If it's doable, could we have a test please? 🙂

@Jarema
Copy link
Member

Jarema commented Jun 15, 2022

thanks @philpennock for filling in the details.

Looks good to me, but the test would be great to have 🙂

@caspervonb
Copy link
Collaborator

One concern here is that match is in source order but that is not guaranteed and up to code generation as far as I know so this could lead to non-determinism.

Signed-off-by: Brooks Townsend <[email protected]>

changed logic to only do TLS verify with hostname

Signed-off-by: Brooks Townsend <[email protected]>

avoided unwraps with a structured match

Signed-off-by: Brooks Townsend <[email protected]>

used symbol binding to simplify match

Signed-off-by: Brooks Townsend <[email protected]>

added test to confirm rejecting tls over ip

Signed-off-by: Brooks Townsend <[email protected]>

refactored match to if/else to ensure ordering

Signed-off-by: Brooks Townsend <[email protected]>
@brooksmtownsend brooksmtownsend force-pushed the brooksmtownsend/connect-over-dnsname branch from b9016e9 to 34dfe54 Compare June 15, 2022 15:46
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

lgtm! 🙂

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGMT!

@Jarema Jarema merged commit 87da933 into nats-io:main Jun 16, 2022
@brooksmtownsend brooksmtownsend deleted the brooksmtownsend/connect-over-dnsname branch June 16, 2022 12:59
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.

4 participants