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

use host header as hostname if exists in http outbound #273

Closed
wants to merge 3 commits into from

Conversation

themez
Copy link

@themez themez commented Mar 14, 2019

CHANGE LOG

If there's a Host header, use it as hostname prior to url.host

INTERNAL LINKS

https://support.newrelic.com/tickets/343043/edit

NOTES

@NatalieWolfe
Copy link
Contributor

Hi @themez! Thanks for the contribution. Your code changes look excellent, but we have some doubts that this is the right way to solve your issue. From your support ticket I understand you do your own service name to IP address lookup and this means all your external requests are recorded as obscure IP addresses instead of the human-readable service name. Is this correct?

Using the Host header in this fashion opens the potential for our reporting to not match reality. It is conceivable that one could make a request to https://serviceA and set the host header to some other name, thus masking the real destination in our UI. Whether this is done maliciously or not, it would still mean that our agent is recording inaccurate values.

You mention not wanting to use DNS for this because you want to control load balancing very tightly. I believe DNS is the correct solution for what you are trying to do, and you can use very short TTLs to keep your load-balanced routing responsive.

What other options have you explored for this?

@themez
Copy link
Author

themez commented Mar 17, 2019

Hi @NatalieWolfe , in our microservices outbound throughput is large so we want to utilize http keep-alive, DNS is not a suitable solution in this case, but let's put this aside.

The most right way to record correct segment name I could think of is the agent providing a way to customize segment name, just like what we can do to transactions.
But since it's not provided currently, maybe we can utilize the Host header which is in HTTP 1.1 standard, it's made to indicate the server which the client want to access to.

I agree with that It can be malicious as the case you described, if a wrong destination is a HTTP 1.0 server, ignores Host header.

But let's imagine another situation, we make a curl request to a proxy server(Nginx), which serves two hostname: a.service and b.service.
curl -H 'Host: b.service' http://a.service, Nginx acted like described in RFC7230, routing the request to b.service.

In HTTP 1.1, Host IS the reality.

We can see either ignore Host or not, it can be malicious somehow, if someone intend to, so why not align to the standard. I believe in most case, NodeJs application does not send requests on whatever user set in UI, but have a business logic to do so. It's not NodeJs agent's duty to consider it might be malicious.

How do you think?

@michaelgoin
Copy link
Member

Hi @themez,

Thank you for the additional thoughts on this and sorry for the long delay responding. We are doing a bit more digging into this topic and virtual hosts, and also across our various language agents. I'll try to get back to you without much further delay.

On the topic of customizing a segment name, I would recommend submitting a feature request for that if it is something you'd like to see regardless. There may be other customers who also would like to have this functionality and it would help inform our future direction.

Thanks!

@michaelgoin
Copy link
Member

Hi @themez,

Thank you for your patience. I have opened a feature request on your behalf for this HOST header functionality. I see the merit in your argument around HTTP 1.1 and virtual hosting. I've provided those sorts of details and additional links inside the feature request.

Unfortunately, for this sort of change, we want to do a bit more due diligence over a quick PR acceptance. This sort of thing is typically best done with alignment across the various language agents and want to ensure appropriate safety-nets and maintenance considerations are in place. As such, we won't be incorporating this change at this time. We'll track this against other feature requests and determine if it is an effort we'll be able to prioritize.

Thank you for your PR and providing additional details why this, or similar functionality, will be beneficial.

Michael

@themez
Copy link
Author

themez commented May 29, 2019

@michaelgoin thanks for you reply, I understand your concerns.

bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this pull request Apr 19, 2024
bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this pull request Apr 23, 2024
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