get: Handle symlink creation and sanitize paths on Windows

[djdv]

This is my current work in progress for handling the issue of geting symlinks on Windows.
It relies on extending the tar-utils API whyrusleeping/tar-utils#2.
Giving us a method to sanitize paths and handle link creation however we like.
Note that the API extensions are not finalized themselves yet, see: whyrusleeping/tar-utils#2 (comment).

Given the restrictions Windows imposes on symlinks, I've opted for a compromise where we will create symlinks if we have permission to, but skip them and warn the user if we don't (instead of failing out).
A similar approach will need to be taken in gx later, with its own set of rules.

I'm looking for general criticism of the approach and implementation.
Some of my own thoughts on this:

Positive

• Enables unprivileged users to partially get hashes that contain links
  • We want this for build dependencies that only use symlinks for testing and are not actually required for building <-This applies to go-ipfs-api not go-ipfs
• Handles Vista+ though both the Windows authorization API and Windows 10's Developer Mode feature

Negative

• Warning is currently too verbose
  • The warning links to the troubleshooting section of windows.md if it encounters link issues. Telling users how they can enable the permission for their version of the OS.
• Directory tree is modified so hashes will not match if you re-add
• Windows has 2 link types(file, directory) instead of 1(generic link), we create file links by default unless the target directory already exists (this is Golang's behaviour) . This may result in unexpected behaviour with native programs but should cause no issue with IPFS.
  • The only option to determine the type I can think of, is scanning our tar contents ahead of time and switching the link type based on the tar contents. If the tar does not contain the link target we can only default to one or the other. This is a problem Microsoft themselves are struggling with in WSL currently...
• I'm unsure if the tar-utils patch seems good or if it's just a convenient hack
  • The patch has been refactored into something nicer
• /core/commands/get_sanitize_windows.go should probably live in another repo. It checks and attempts to enable symlink creation privileges for the current process.
  • Maybe in https://github.com/Kubuxu/go-os-helper as CanCreateLinks()
  • Probably more appropriate in /x/sys/windows

[djdv]

This should be a direct improvement, we're adding in 2 methods for regular users to create links, skipping them when creation is impossible, and sanitizing output to be platform legal.
This should solve the problem of ungetable hashes on Windows, caused by both filenames/paths (#2013) and hashes that include symlinks.

The only thing that needs a firm decision is, should we require users hold symlink-creation-privilege or not?
That is to say, do we skip links with a warning (current patch behaviour) or fail on links with the same warning; directing users to the Windows.md troubleshooting section

user does not have symlink creation privileges (see: https://git.io/vpHKV)

The functions in /core/commands/get_sanitize_windows.go should likely be refactored and upstreamed to /x/sys/windows but this can come later.

[djdv]

I should also mention that W10 support will only work in Go 1.11+
golang/go#22874
Edit: We could try and backport this though.

[djdv]

The only thing that needs a firm decision is, should we require users hold symlink-creation-privilege or not?

I have given this some thought. Originally, I was leaning towards skipping links on Windows to match (bugged) behaviour of previous gx releases (see: https://discuss.ipfs.io/t/issue-build-from-source/2445).

However, since we can now handle link creation with the right privileges, I'm leaning towards erroring out and telling users how to alter their privileges.
Having special exceptions here and in go-ipfs-api seems like the wrong choice.
Having consistent behaviour across platforms seems better.

[whyrusleeping]

Would be great to pull this windows specific logic out into its own function

[whyrusleeping]

also, if not too difficult, making these anonymous functions into real functions would improve readability here a good deal

[whyrusleeping]

actually, just doing this should clean everything up well enough

[whyrusleeping]

Cool, this LGTM. I'd like one more quick review before merge, either @kevina or @schomatis would be good.

[schomatis]

Sure, I can take a look at it next week.

[Stebalien]

However, since we can now handle link creation with the right privileges, I'm leaning towards erroring out and telling users how to alter their privileges.

I'd second this but don't have a strong opinion.

The only option to determine the type I can think of, is scanning our tar contents ahead of time and switching the link type based on the tar contents. If the tar does not contain the link target we can only default to one or the other. This is a problem Microsoft themselves are struggling with in WSL currently...

We could set links with non-existing targets aside and fill them in at the end. However, we can do this later.

[schomatis]

This PR has mostly specific Win stuff, I don't think I can review much here, I'll take a look at the (already merged) tar-utils PR that actually does the sanitizing against path traversal attacks (which the winSanitize is levering).

(Wow, I never imagined Win symlinking was this involved, great work @djdv!)

[Stebalien]

To the extent that I can review this (it has a lot of low-level windows code that I'm unfamiliar with), LGTM.

However:

1. I'd still like to be smarter about directory versus file symlinks.
2. We should make a final decision as to what to do by default.

[whyrusleeping]

@Stebalien i'm assuming that those are blocking comments since you didnt give an explicit approval.

[djdv]

@Stebalien (#5159 (comment))

However, IIRC, you were thinking about erroring out by default? We should probably come to a decision on that before merging.

Yes, the current patch will fail with a less-ambiguous error (previously: "Access denied"), pointing users to documentation about the issue. We always attempt to create the link, but if we encounter an error, we try to distinguish the reason based on the environment.

Wall of text recap:
There's also special handling for various platform features.
Pre-Vista users get a warning that Symlinks are not supported at the platform level.
Vista+ users have the option to enable a UAC privilege for link creation which we look for and use.
W10+ users have the Vista option, as well as enabling "Developer mode", but this will not work until Go 1.11. (We check for this and fail in lower versions).

The original alternative was to skip creating links, with a warning message, if we didn't have the ability to create them (as opposed to failing outright). This was proposed because a previous release version of gx created 0 byte files instead of links, on Windows. That behaviour was a bug, so mimicking it, even with a warning, seemed less than ideal.

The only other option I can think of, is adding a flag to get that allows us to continue, even if an error has occurred for an item. Warning with the get switch, but halting by default. This would place the decision in the users hands. That seems unnecessary imo.

Also, I'd like to be smarter about file versus directory symlinks if possible.

Agreed, this is something I would like to look into further. The current idea is to determine the type from the tar contents if we can, but ultimately, if the link points to a target outside of the hash/tar, I don't think we can determine which type is intended.

[Stebalien]

Also, I'd like to be smarter about file versus directory symlinks if possible.

Agreed, this is something I would like to look into further. The current idea is to determine the type from the tar contents if we can, but ultimately, if the link points to a target outside of the hash/tar, I don't think we can determine which type is intended.

I'd actually like to ban that outright for security reasons. Unfortunately, that would break valid use cases. I'd like to at least have a flag.

Yes, the current patch will fail with a less-ambiguous error (previously: "Access denied"), pointing users to documentation about the issue. We always attempt to create the link, but if we encounter an error, we try to distinguish the reason based on the environment.

Got it. SGTM.

---

So, I'd like to punt all the symlink stuff to: #5161. We can merge this once the tests pass (looks like a go fmt issue).

[schomatis]

@djdv Should this be labeled blocked by #5161?

[djdv]

@schomatis
I suppose so.

A lot of this was discussed in a separate PR whyrusleeping/tar-utils#2 (unfortunately a lot over private email as well)
and summed up a little here: #4808 (comment) and here https://blog.ipfs.io/36-a-look-at-windows/#file-output-names
Important to note the security problem in the blog post too.
While it's very impractical, I think you can still do this.

We need hard decisions on what the default behaviour of symlinks should be and how to configure that behaviour for go-ipfs and through api calls.
Some users may want to error if a hash contains bad paths, others may want them translated, or skipped.

The default sanitizer implemented in the tar reader can be changed to support whatever symlink or path policies we want, either in that package or in go-ipfs, by changing the function pointers and cascading filter functions.
https://github.com/whyrusleeping/tar-utils/pull/2/files#diff-51c74fb3d0d2d84f5f9010169d07b6beR87
https://github.com/ipfs/go-ipfs/pull/4956/files#diff-08ea28298b807f006bd45d13c13188a8R272

Outside of some refactoring of the Windows specific systemcalls, this can be ready to go as soon as we decide how we wish symlinks to behave.

[djdv]

I'll add too, at least 1 person was interested in also modifying symlinks during add
#5225

Some users are avoiding the issue by just resolving symlinks on add, but this won't fix all cases, and is only a solution for providers, not downloaders.

This was implemented here but the functionality was split into 2 arguments
ipfs/go-ipfs-cmds#96
1 to dereference arguments only, and the other to dereference all links.
The latter is blocked on go-ipfs-cmds needing functional options.
ipfs/go-ipfs-cmds#96 (comment)