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

Do not fail parse_resolv_conf on invalid hostname #1740

Merged

Conversation

schultetwin1
Copy link
Contributor

@schultetwin1 schultetwin1 commented Jul 13, 2022

On macOS, user's can set their hostname to something invalid such as
matt.schulte's macbookpro. This will cause parse_resolv_conf to fail
because "into_resolver_config" will think the hostname contains the
system domain and then fail to parse that system domain.

Instead, we should treat the hostname as if it contains no domain (the
same as if the hostname contained no ".").

This fixes #1739

On macOS, user's can set their hostname to something invalid such as
matt.schulte's macbookpro. This will cause parse_resolv_conf to fail
because "into_resolver_config" will think the hostname contains the
system domain and then fail to parse that system domain.

Instead, we should treat the hostname as if it contains no domain (the
same as if the hostname contained no ".".
@schultetwin1 schultetwin1 force-pushed the no_parse_error_for_invalid_hostnames branch from d59a8ba to ce230c4 Compare July 13, 2022 19:18
@bluejekyll
Copy link
Member

If I’m understanding the change here, we’re ignoring a malformed name. I guess I don’t know what we want in that case. Im a little surprised that Apple doesn’t escape those strings.

@Noah-Kennedy
Copy link
Contributor

@bluejekyll windows doesn't either iirc

@schultetwin1
Copy link
Contributor Author

If I’m understanding the change here, we’re ignoring a malformed name. I guess I don’t know what we want in that case. Im a little surprised that Apple doesn’t escape those strings.

That's correct but I look at it a little differently.

I would say we are treating malformed names the same way we are treating names without a "." in them. So if your hostname is "mattscomputer" or "matt's computer", or "matt.schulte's computer" this now treats them all the same way. Previously, "mattscomputer" or "matt's computer" would result in None for the domain and parse_resolv_conf would not fail. "matt.schulte's computer" would result in an parse_resolv_conf failing.

@bluejekyll
Copy link
Member

Would you have time to add a test (with some documentation) to that effect? I’m ok with merging this, but I worry that we might regress this in the future as it’s not an obvious case we’re trying to support.

Thanks for the explanation and the PR!

@schultetwin1
Copy link
Contributor Author

Would you have time to add a test (with some documentation) to that effect? I’m ok with merging this, but I worry that we might regress this in the future as it’s not an obvious case we’re trying to support.

Future regressions are a good point. I can add some documentation but unfortunately I'm not sure if there is a minimal way to add testing here. The get_system_domain eventually calls into a crate that will call a system call to get the hostname. I'm not sure how I could control the return of that syscall without modifying the actual machine's hostname (which I'd like to avoid if possible).

@Noah-Kennedy
Copy link
Contributor

@djc any reason this isn't merged?

@djc djc merged commit 19e3d2b into hickory-dns:main Jul 29, 2022
@schultetwin1 schultetwin1 deleted the no_parse_error_for_invalid_hostnames branch August 26, 2022 21:51
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.

parse_resolv_conf fails if machine's hostname contains a "." followed by a non alphanumeric value
5 participants