[WIP] Add SmbGetter

[sylviamoss]

This PRs adds a SmbGetter and a SmbDetector to download files from shared folders using SMB protocol.
On Unix, the SMB getter will try to get files from samba using the smbclient when a URL is prefixed with smb://, for example smb://foo/bar/dir.
On Windows, the SMB getter will try to get files using the windows file system when the URL is prefixed with smb://, //, or \\. This will be done by using the existing FileGetter if it's available.
The SmbDetector will make windows src path with // or \\ prefixes a smb:// URL.

This PR also adds the necessary config for running the tests. This includes:

• A new step to the CircleCi that starts two containers, one for the smb server and the other for the go-getter (client), and run the tests inside the go-getter container;
• Dockerfile and docker-compose files to configure both containers and add shared folders with mock files and directories used on go_smb_test.go and client_test.go;
• Makefile to help running the containers and tests locally;
• Readme information about the tests and the SMB protocol.

This is motivated by issues hashicorp/packer#8729 and hashicorp/packer#8552 caused by
hashicorp/packer#7627

[azr]

Nice one, LGTM

[SwampDragons]

@schmichael any chance we can get a Nomad review on this?

[mildwonkey]

This looks fine to me as far as terraform core is concerned! Terraform configures a specific list of getters for exactly this reason - no surprises!
(for ex: https://github.com/hashicorp/terraform/blob/807267d1b56aa6298fecf5edba98c088c2866e83/internal/initwd/getter.go#L50)

I'm not necessarily qualified to review the contents of the PR, but I can confirm that this shouldn't impact core.

[schmichael]

Apologies for not fully considering the security ramifications of this getter initially.

If the provided URL can be evaluated by bash to run arbitrary commands, I think that should block merging this PR as a user could be tricked to copy and paste a dangerous URL to go-getter without realizing it.

Even with that fixed I think the use of FileGetter means anyone who runs go-getter on behalf of untrusted users (eg Nomad, any SaaS tools that use go-getter on user input) will need to avoid using the smb getter.

That part is not a blocker for merging! Just because it's not usable by Nomad* doesn't mean it's not useful for other users of go-getter! Overall the PR looks good!

* unless Nomad makes lots of changes to how we use go-getter

[schmichael]

Do you need to use os.PathSeparator instead of literal slashes ("/") and filepath.Join(...) instead of string concatenation with plusses (host + path)?

[schmichael]

[nitpick] err is the idiomatic variable name to use here

[schmichael]

I hadn't thought about this before, sorry, but this approach presents a problem for embedders of go-getter like Nomad.

Nomad has an explicit allow-list of methods and excludes the file getter because the Nomad agent itself runs go-getter on behalf of users.

If Nomad would enable the smb getter, we would implicitly be allowing the file getter as well. Previously we decided not to do that, but if you look at the last comment on hashicorp/nomad#1897 there's a valid reason for figuring out how to allow it.

[SwampDragons]

@schmichael as the nomad code currently lives, it appears that this would not be an issue because you're not enabling the SMB getter either, correct?

[schmichael]

Correct! That being said other users of go-getter might be surprised that the file getter is implicitly enabled if they enable the smb getter.

Perhaps this code path could check if the file getter is enabled before deferring to it?

[sylviamoss]

I've changed a bit based on your suggestions. Now for Windows only, if a FileGetter is available, the SmbGetter will use it to get shared files via the Windows file system.
You're right, It doesn't make sense to allow using the FileGetter for Unix users cuz this can make people get unwanted files. This is the Nomad concern, right?
For Windows, this still has to be done because as I know the file system is the most common way to access network shared folders.

[schmichael]

For users of go-getter like Nomad which do not trust the URLs they're asked to fetch, I don't think this is a safe approach. It effectively lets an untrusted user run any command inside go-getter by embedding it in the URL.

I think to prevent this you need to avoid using bash -c, always pass "smbclient" as the first argument, and pass a string slice of arguments as subsequent arguments to ensure they're always treated as arguments to smbclients and no shell evaluation is applied. The git getter uses this approach (git is always the first argument so users cannot have input evaluated by bash).

The alternative is trying to clean or validate incoming URLs to ensure they don't include characters bash might interpret, but this seems extremely difficult to do safely.

[sylviamoss]

Nice! I was struggling to use it as a first argument but then. I was able to do it with your help on Slack. Thanks!

[sylviamoss]

I'm opening another PR with an updated version of this one.