pyed / ipfilter

ipfilter is a middleware for Caddy that blocks or allows requests based on the client's IP
https://caddyserver.com/docs/ipfilter
Apache License 2.0
83 stars 19 forks source link

"Invalid char ':'" error when building with GO111MODULE=on #39

Closed deanishe closed 5 years ago

deanishe commented 5 years ago

Go v1.12.4 on macOS High Sierra.

When trying to build caddy v1.0.0-beta1 or master with GO111MODULE=on, go fails to extract this plugin due to colons in filenames:

go: extracting github.com/pyed/ipfilter v1.1.1
-> unzip /Users/username/pkg/mod/cache/download/github.com/pyed/ipfilter/@v/v1.1.1.zip: malformed file path "testdata/blacklist/1234/abcd/1234:abcd::1": invalid char ':'
build github.com/mholt/caddy/caddy: cannot load github.com/pyed/ipfilter: unzip /Users/username/pkg/mod/cache/download/github.com/pyed/ipfilter/@v/v1.1.1.zip: malformed file path "testdata/blacklist/1234/abcd/1234:abcd::1": invalid char ':'
2019/04/16 18:26:12 exit status 1
exit status 1

It seems the Go team do not plan to allow colons in filenames, as it's a reserved character on Windows.

krader1961 commented 5 years ago

I can appreciate why a colon is a reserved character in MS Windows file names. It also has special meaning on other non-UNIX operating systems (many of which are of historical interest only). But looking at https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file#naming-conventions I'm left shaking my head in disbelief. They also disallow <, >, ?, and * and other characters in file names. WTF? Does Go also disallow those chars in module file names?

Regardless, we should probably rename those unit test support files and modify this module to support both colons and some other separator char for IPv6 addresses.

krader1961 commented 5 years ago

Does anyone have an opinion on the optimal alternative separator char for IPv6 addresses? Underscore? Hyphen? Dollar-sign? Hash(sharp)-sign? Something else? I'd prefer not using periods due to ambiguity with IPv4 addresses.

deanishe commented 5 years ago

Does anyone have an opinion on the optimal alternative separator char for IPv6 addresses?

🍺, tbh.

But seriously, is confusion with IPv4 addresses likely?

That said, :: is common in IPv6 addresses, and .. isn't going to work at all in a filename.

pyed commented 5 years ago

We need to go with an alternative character, hyphens sounds okay .. I guess, any thoughts ?

krader1961 commented 5 years ago

and .. isn't going to work at all in a filename

I can't speak for how MS Windows file APIs work but dots, even two consecutive dots, are valid in a UNIX file name. A double dot only has special significance when that is the entire file name.

is confusion with IPv4 addresses likely?

There isn't any ambiguity in practice but using periods does make it marginally harder for a human to tell at a glance whether it's an IPv4 or IPv6 address.

hyphens sounds okay .. I guess, any thoughts ?

It should be a char that doesn't have special-meaning when used in a shell bare word. So that rules out dollar-sign, hash-mark, etc. I'm leaning towards = for the symmetry with : but - is fine too.

pyed commented 5 years ago

2001=db8=a0b=12f0==1 OR 2001-dbz-a0b-12f0--1, I guess = looks more like : and - makes it look like a Mac address, so = it is. Edit: MAC address uses : too 🤦‍♂️

krader1961 commented 5 years ago

@pyed Do you want to take care of this or would you prefer I do so since I wrote the code in question?

pyed commented 5 years ago

@krader1961 I'd really appreciate it if you write the fix for this as you know your way around this part of the code better than I do.

ghost commented 5 years ago

@krader1961 Any estimate on when you'll open a PR to fix this issue? I can't build Caddy from source because of this problem.

krader1961 commented 5 years ago

@whalehub I will try fix it this weekend; i.e., the next 48 hours. But I'll need to send a P.R. for review and that could take a couple more days before it is merged.

ghost commented 5 years ago

@krader1961 Thank you!

@pyed Can you please push a v1.1.2 release which includes this commit?

deanishe commented 5 years ago

I can't speak for how MS Windows file APIs work but dots, even two consecutive dots, are valid in a UNIX file name

Windows doesn't like filename ending in a dot. Yup, it's technically valid in UNIX filenames, but code to mitigate filesystem traversal vulns doesn't always know that and may reject any path with .. in it.

ghost commented 5 years ago

@pyed Thank you for the v1.1.2 release!