Check detectors and relative paths before proto from X-Terraform-Get

[jbardin]

When checking for an unexpected protocol switch via X-Terraform-Get, we
first must run the configured detectors and check for a relative request
path, since X-Terraform-Get is not required to return a valid URL.

Fixes #368

[nywilken]

The changes look good to me. I have a couple of questions on the handling of sub directory and relative path requests. I'm approving in advance in case my questions are just due to my gaps of knowledge around go-getter.

But feel free to ping me again for another review if the questions result in a change.

[nywilken]

I was a little confused about the call to getSubdir the first time I read this code. Is the source being used here the value returned from the X-Terraform-Get header?

If so, would we not want to run the protocol switch check before returning? Or will this be handled in the final call to Get that is made in the getSubdir function?

[jbardin]

That's a good question. While the PR looks big because I dropped a level of indentation by returning early, I left the overall logic the same. I think you may be right that a subdir would bypass the the protocol checks since it goes on to call Get just like below -- I'll follow up with a refactor of getSubdir so we can pass in the same options

[jbardin]

The new proto checks were bypassed by the addition of a subdirectory component to the url. I refactored this a bit to move the getSubdir call after the checks, and pass in the same options we would use for Get.

[nywilken]

For my knowledge in the case of a relative path request how does go-getter know the url used in the previous request? I don't see it being passed into the next Get request.

[jbardin]

I don't think relative paths actually work in this mode, but I added the check for completeness, and so that we can return any intended error later, rather than a more confusing no getter available message.

[nywilken]

Thanks for answering my questions. The updated check for subDir looks good to me. I pinged @picatz just for visibility as it changes how the getter logic was understood at first.

[nywilken]

@picatz pinging for visibility as the need changes the mental model a little.

[picatz]

Thank you for the ping!

The intent for this original check was to totally prevent the HttpGetter from allowing cross-protocol switching unless the getter was already supported by a Client. I understand the reasoning for the FileGetter with a subdirectory component, but I hate this is allowed at all. It has the potential to be dangerous, and generally not desirable from a security standpoint.

Thankfully, I don't think it would cause problems for Nomad, who explicitly disable the FileGetter because it's dangerous in their context. They use a Client directly. But I could see this causing problems in other contexts. Likely outside of HashiCorp.

I would much rather this be a new option on the HttpGetter, rather than be implicitly allowed by default. Maybe something like:

httpGetter := &HttpGetter{
  AllowFileGetterForSubDirectoryComponent: true,
}

What do you think? Would that be an acceptable solution?

[jbardin]

To be clear, this is more limiting than the current release, since getSubdir was falling back to all the default getters. We can't really change this in version 1, since it's a breaking change. While X-Terraform-Get probably shouldn't have been so liberal in spec, the protocol does define any valid go-getter source string is a valid value, including subdirectories. I think the only valid way to avoid this is to disable X-Terraform-Get, which we have an option for already.

[picatz]

To be clear getSubdir was falling back to all default getters when using the HttpGetter directly?

Aside from that clarification: I think I agreed with you, that disabling X-Terraform-Get would generally be the "right" solution, as opposed to an additional option.

[jbardin]

Right now these checks are only run if subDir == "", otherwise we just call getSubDir, which runs Get with the default options, bypassing the forced http+https getters entirely. This change now at least will force the http+https getters with subdirectories too.