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

Fix HTTP::Client certificate validation error on FQDN (host with trailing dot) #12778

Conversation

compumike
Copy link
Contributor

Fixes #12777

I have tested this change and it works for me.

The test I added to spec/manual/https_client_spec.cr fails without this change, and passes with it.

@compumike compumike force-pushed the fix-http-client-fqdn-ssl-cert-validation-bug branch from a757248 to 5e69f8d Compare December 12, 2022 01:02
src/http/client.cr Outdated Show resolved Hide resolved
Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
@compumike
Copy link
Contributor Author

I've applied @HertzDevil 's suggested change.

Are there any other changes desired before this can be merged?

If it helps, to clarify my urgency / security implications / technical use-case, I'd like to use this in production as I'm adding an HTTP endpoint uptime & latency monitoring feature to my on-call rotations / monitoring / alerting SaaS. I prefer to always use a fully-qualified domain when connecting to any customer-specified endpoints so that I can be sure that the user-specified URLs aren't inadvertently (or maliciously) probing my internal infrastructure. (Obviously I also need to disallow known internal hosts, /^.+\.local\.?$/i for example. In my Kubernetes cluster environment, the search line from /etc/resolv.conf looks like search my_namespace_redacted.svc.cluster.local svc.cluster.local cluster.local.) To accomplish this security improvement, I simply modify any parsed URI host and make sure it has a trailing dot before I send to HTTP::Client. But this security improvement is currently broken for scheme == "https" without this PR.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Thanks!

@beta-ziliani beta-ziliani added this to the 1.7.0 milestone Dec 19, 2022
@straight-shoota
Copy link
Member

straight-shoota commented Dec 19, 2022

@compumike This is off topic, but considering FQDN resulution as a security mechanism sounds concerning to me. I'm not too deep into this topic, but I could spot some potential loopholes.
For example, a user-provided domain name could could resolves to a CNAME record that points to an internal domain name which might cause your nameserver to resolve to that.
If you don't want internal domain names to be accessible for domain resolution, I'd use a DNS resolver for user-provided domains that doesn't know about any internals.

@compumike
Copy link
Contributor Author

@straight-shoota I greatly appreciate the way you think 😄 -- will consider that suggestion and consider other ways of restricting the reach of user-provided URLs at both the DNS level and the IP level. Thank you!

@straight-shoota straight-shoota changed the title Fix HTTP::Client certificate validation error on FQDN (host with trailing dot) Fix HTTP::Client certificate validation error on FQDN (host with trailing dot) Dec 20, 2022
@straight-shoota straight-shoota merged commit adfc4ce into crystal-lang:master Dec 20, 2022
carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Dec 21, 2022
…ailing dot) (crystal-lang#12778)

Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTP::Client throws OpenSSL::SSL::Error for FQDN (domain with trailing dot)
5 participants