Do not add standard ports to host prior to signing. Fixes #40

[brnkrygs]

Here's a potential fix for Issue #40

I applied the changes listed here manually to my codebase and re-ran the scenario that was breaking for me (as reported in #40 ), and I had no issues. What do you think about the approach here?

[mhart]

Hmmm, not too sure about this – why were you adding ports in the first place, if they're just standard ports?

[brnkrygs]

Good question :)

In short its because I habitually lean towards explicitness in configuration, and leverage templated configs across environments.

For a bit more detail on our specific scenario:

We have a number of different environments where our code is running. Those environments have similar services running on potentially different hosts and ports.

In order to improve operational flexibility, we expose config settings that are populated based on the environment the code is running in. One of those config settings was the port Elasticsearch was listening on. Signatures on requests to ES would magically start to fail if we happened to inject 443 or 80 as the port when populating our config for that environment.

It seemed like a risk to have some settings for port suddenly break connectivity, especially for our template-based config system.

Specifically thinking through the scenario of an operations team member standing up a new environment and configuring our system, that person would have to know ahead of time that some port configs, though technically legitimate, would break signatures and connectivity because the low-level node module involved drops the port in some cases.

It seemed like a better option was to absorb that edge case into the signature module rather than risk an outage from human error.

Does this seem to fit with your goals for the module? Can I help clarify any further?

Thanks for reaching out to engage with our situation! I appreciate your time and attention!

[mhart]

@brnkrygs well I'm not sure whether it's best to mess with the port handling or just add it to documentation – I'll probably need to do some testing in non-Node.js envs (ie, in the browser) to see what behaviour default ports exhibit. It's unclear from the AWS signature docs how they should be handled unfortunately.

The more I think about it, the more I think it should be fine to remove them if they're default – but I'd probably then want to remove them from options as well, so that it reflects the signature. Thoughts on this?

[mhart]

(to clarify: not remove them as an option – just remove the property from the options object if it happens to be a default port – so you can pass it in if you like, but it'll be removed)

[brnkrygs]

I see what you mean, and you're right, its definitely worth testing in browsers, even if just to see what those requests looks like. If browsers are dropping the port too, then I'd feel even more safe to moving ahead.

Do you have a quick way to set up a test environment for checking that out?

AWS's signature is based solely on the structure of the request that hits it, which is precisely the problem. Node at least silently strips the port so the requests don't match.

I'm torn on taking it out of the options object. I see the benefit of making it match exactly what is signed, and included in the actual request, but it seems like a bit of an unexpected side effect. Maybe if that behavior was well-documented I would be comfortable with it.

... though ...

I suppose, taking the port out of options would help manage any potential browser inconsistencies. That, actually, makes me really like the idea... taking port out would normalize all requests no matter the underlying request engine, and no matter what decisions browser vendors make along the way.

Sorry, that was a bit stream-of-consciousness... I hope I'm making sense.

[mhart]

Hmmm, bit of a snag – what if someone wants to use 443 with http, or 80 with https?

[brnkrygs]

I wondered about that... we could maybe check if options has a protocol set, and compare there.

I pulled that logic out to separate function in case you wanted to grow into it.

Its a scenario that could be tackled now, or tabled until someone actually runs into it.

edited for wording

[brnkrygs]

Was thinking about this again last night, and this may summarize my thoughts more succinctly:

The goal of this PR is to normalize requests prior to signing them such that they match the request that will actually get sent out.

Therefore, a (hopefully not too glib) answer to your legit question above:

what if someone wants to use 443 with http, or 80 with https?

would be:

Make it do exactly what node does now when requests are actually sent out.

[mhart]

@brnkrygs but Node.js knows whether you're sending http or https because of the module you use – not the options

[brnkrygs]

My understanding was that there was a protocol option available for the low-level request. I'll check how that behaves for each module.

https://nodejs.org/api/http.html#http_http_request_options_callback
and
https://nodejs.org/api/https.html#https_https_request_options_callback