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

Drop standard port (443 for HTTPS, 80 for HTTP) from host header #309

Closed
wants to merge 1 commit into from
Closed

Drop standard port (443 for HTTPS, 80 for HTTP) from host header #309

wants to merge 1 commit into from

Conversation

jeskew
Copy link

@jeskew jeskew commented Oct 20, 2015

Including the port can complicate certain authentication schemes (such as AWS authentication) that expect only non-standard ports to be included in the host header.

@jeskew
Copy link
Author

jeskew commented Oct 20, 2015

The Travis failure does not appear related to the change in this PR.

@polyfractal
Copy link
Contributor

Yeah, you can ignore the Travis failure (Travis is broken atm, being worked on re: #286) :)

Sooo, I'll admit that I'm a bit confused why this is necessary. Could you explain how it breaks AWS authentication? I don't think there is an provision in the HTTP spec that says you shouldn't include the port for standard schemes?

I'm worried about introducing explicit edge-cases like this because the clients try hard to remain un-opinionated. For example, a user may be relying on those same header values for an upstream service, proxy, etc before it reaches the ES cluster. It would be opinionated of us to start removing headers to fix an edge-case with a service.

The new 2.0 branch is basically 100% pluggable, so I think a better solution would be to write a custom AwsConnectionFactory + AwsConnection that munges the host as required to satisfy AWS.

I'm looking at what'd be required to inject a custom ConnectionFactory, and while it's possible, it's a bit complex because that particular class is the nexus of the dependency chain (it requires a handler, serializer, logger, etc). You'd have to wire up a significant portion of the dependency graph yourself.

We could work on making injecting a custom ConnectionFactory a little more ergonomic. e.g. instantiating from a class name in addition to direct object injection.

@jeskew
Copy link
Author

jeskew commented Oct 20, 2015

There's a provision in RFC 3986 that URIs should omit a port when it's the default for a given scheme, but you're right that it's a "should" and not a "must." AWS authentication requires finding the SHA-256 of a canonicalized form of a request, so minor variations (like including :80 or :443 at the end of a host header) will result in an invalid signature. This might affect other digest-based authentication schemes (like Hawk), but I haven't done enough research to say that with certainty.

As far as the ConnectionFactory is concerned... under the 2.0 branch, it's much easier to inject a custom handler. An authenticating handler is less than 10 lines of code, though one of those is dedicated to normalizing the host header:

$requestToHandle['headers']['host'][0] 
    = parse_url($requestToHandle['headers']['host'][0])['host'];

@polyfractal
Copy link
Contributor

That's a good point, a custom Handler would definitely be the path of least resistance in this case. Although I'd still like to improve the ConnectionFactory situation, it shouldn't be quite so complicated to inject your own custom connection machinery there.

Re: Hawk, it appears that the port is always used. It's unclear if it's mandatory (since Hawk doesn't have a written protocol per-se, just the reference code which acts as the protocol), but in all cases that I could find they always fallback to an explicit port if not provided. E.g.:

That was a brief and very unscientific survey of the code, so I may be wrong :)

After chatting with some of the other client devs, I'm inclined to keep this functionality out of core and let the end-user plug in a custom handler instead. It feels like the perfect use-case for a pluggable component (small custom logic that is opinionated). Especially since other authentication schemes may work differently, and it's a "should" not "must" requirement in the HTTP spec.

@polyfractal
Copy link
Contributor

Totally unrelated, if you have a moment to sign the Elastic CLA it'll expedite any future PRs. :)

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