add basic ip centric access control on routes #442

aaronhurt · 2018-02-13T01:45:10Z

I've verified and tested this locally with good results. Running it in our development environment for testing as well.

aaronhurt · 2018-02-13T01:45:44Z

Oops, forgot to link this to #439

aaronhurt · 2018-02-13T02:53:18Z

route/access_rules.go

+
+	// currently only one function - more may be added in the future
+	return t.denyByIP(net.ParseIP(host))
+}


I have mixed feelings about this section. In the event that fabio is deployed behind another load balancer you would want to check the XFF header but then what about the actual source? Would it be better to check both the XFF (if present and different) as well as the actual RemoteAddr and return true (deny) if either of those checks return true on their own?

Yes, I definitely don't like this. It's an open exploit as is since there is no validation on who is allowed to pass an XFF header to fabio.

Example with allow=ip:172.16.0.0/12 and a client that is not sourcing from that network.

$ curl -H 'X-Forwarded-For: 172.16.1.1' https://test-site/foo {} $ curl https://test-site/foo access denied

I think it needs to check both XFF and RemoteAddr or it's basically useless.

aaronhurt · 2018-02-13T03:00:38Z

It seems the tests are randomly failing with a timeout around some consul interaction. I've seen the different go versions succeed and fail at different times with the same code by just amending the commit and doing force push.

aaronhurt · 2018-02-13T04:33:22Z

I've fixed the security hole in the latest commit.

aaronhurt · 2018-02-13T05:24:41Z

@magiconair I think that's it unless you have other suggestions or requests.

magiconair · 2018-02-13T08:32:29Z

I haven't fully nailed down the test failures. I'm punting by running on travis and codeship and if one of them is green I'm ok with that :-/ I'll have a look at the PR later.

aaronhurt · 2018-02-13T18:14:13Z

@microadam are you able to test and verify?

microadam · 2018-02-13T20:17:01Z

I will see if I can give it a go! Not a go developer, but will give compiling it a shot

aaronhurt · 2018-02-13T20:18:40Z

@microadam I could post a binary at my fork of the repo if that makes it easier. Not sure how comfortable you would be with that though ... and I completely understand.

microadam · 2018-02-13T20:27:06Z

@leprechau yeah that would definitely work! Hitting lots of errors trying to compile (probably just me not knowing how to do it!)

aaronhurt · 2018-02-13T20:35:34Z

Linux amd64?

microadam · 2018-02-13T20:39:43Z

Perfect!

microadam · 2018-02-13T20:50:21Z

Have managed to compile it! will try and test it out

aaronhurt · 2018-02-13T20:53:20Z

Okay, awesome ... let me know if you need anything else.

microadam · 2018-02-13T21:07:31Z

I assume I must be doing something wrong, but with options of allow=172.14.0.0/12 and accessing it from an address of 192.168.0.x, I don't get denied, which is what I was expecting

I am on Mac OSX running fabio on a Vagrant box, so there are a few networks to contend with

aaronhurt · 2018-02-13T21:37:53Z

Interesting ... can you turn on access logging just to see what IP fabio sees your connection coming from? There is probably a NAT between you and the vbox host. To be really sure you could switch it to deny=0.0.0.0/0 that would block everything ... or almost the same an allow=127.0.0.100/32 should pretty much block everything.

microadam · 2018-02-14T07:19:24Z

No joy with the deny all either. I can only assume I have compiled it wrong. Perhaps if you could provide a binary after all?

Steps to compile were:

git clone git@github.com:myENA/fabio.git
git checkout feature/route-acl
go get ./...
make linux

I assume something in there is not quite right! (first time actually trying to compile a GO project...)

microadam · 2018-02-14T07:57:36Z

Think I have it compiled correctly now. Getting this error:

[ERROR] failed to process access rules: invalid access item, expected <type>:<data>, got [0.0.0.0/0]

My urlprefix looks like: urlprefix-my-site.com/ deny=0.0.0.0/0

EDIT: Im a muppet, should have been deny=ip:0.0.0.0/0 my bad! Will get back to you once I have run some tests, apologies for the newbie questions :)

magiconair · 2018-02-14T08:14:51Z

@microadam and @leprechau thanks to both of you for working on this!

aaronhurt · 2018-02-14T13:00:17Z

@microadam np, thanks for the testing! There are some markdown docs in the commit but I think you have the syntax right now.

aaronhurt · 2018-02-14T13:02:32Z

Also, just realized my own example up there was wrong ... sorry about that. At least the error message works 😂

microadam · 2018-02-14T20:54:51Z

Finally got around to doing some tests, apologies for the delay!

A few things I have spotted, some are possibly edge cases and not worth worrying about (possibly all).

with deny=ip:0.0.0.0/0 I can still access it from localhost. Request comes through like:

[::1] - - [14/Feb/2018:20:38:10 +0000] "GET / HTTP/1.1" 200 68744

If it put in allow=ip:192.168.0.252 I correctly get:

[ERROR] failed to process access rules: failed to parse CIDR ip:192.168.0.252 with error: invalid CIDR address: 192.168.0.252

However it then defaults to allowing all traffic through. As we don't have any concept of "validation" before the server runs, perhaps this should prevent the server from running, or at least, if its an allow rule, it should block EVERYTHING, at least you then know something is wrong? Not sure how to handle the case of where its a deny rule though, as you wouldn't want to default to ALLOWING everything as you wouldn't know there was an issue. Just trying to think of the case where this is all automated and people aren't paying attention to the logs.

Seems to work in all the other trivial tests I have done! I haven't managed to test it behind another proxy yet, as I don't have that easily setup, but from the looks of the code it should handle that!

Thanks a lot for doing this :)

aaronhurt · 2018-02-14T21:33:14Z

Thanks for the feedback. Internally the code is using the golang core net package. Specifically it's using the ParseCIDR and Contains functions.

https://golang.org/pkg/net/#ParseCIDR
https://golang.org/pkg/net/#IPNet.Contains

They support both IPv4 and IPv6 but will not transit boundaries. So, for the first example with deny=ip:0.0.0.0/0 that will match ALL IPv4 addresses but will not match IPv6 addresses. The [::1] request source is IPv6 localhost.

The second part is also a side affect of the ParseCIDR function. 192.168.0.252 is not in proper CIDR notation. To specify a single IP address in CIDR form it would be 192.168.0.252/32 (a 32 bit mask denotes a single address).

To stopping server operation and/or ignoring the route I have mixed feelings. The current behavior for invalid options adds the route but just ignores the bad options. I kept his same behavior for the allow and deny options. It is completely possible to either skip the route (not add it) or stop the server if route parsing fails. However, since routes may be added dynamically I don't think stopping the server for an invalid route option makes any sense.

microadam · 2018-02-14T21:38:43Z

Sure. That all makes sense. Just trying to think if there is a way to make it really obvious that it has failed without checking the logs. There isn’t really a clean way of doing it I suppose. I guess if you do mess it up and don’t notice the logs, you will just not see the change you were expecting I.e person still won’t have access or the traffic you are trying to block will still come through. I guess that is the best that can be done! Definitely a nice addition to this project, certainly covers my use case :)

…

On 13 Feb 2018, at 18:14, Aaron Hurt ***@***.***> wrote: @microadam are you able to test and verify? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aaronhurt · 2018-02-14T21:48:30Z

Awesome, thank you for the feedback and testing.

@magiconair any other thoughts or concerns? What do you think about skipping the route on failure for allow/deny processing? Just putting a continue in the loop if the process function returns error?

magiconair · 2018-02-15T06:07:22Z

I’ll have a look at the code later after coffee. One thought I had about the addresses is whether to automatically change 1.2.3.4 to 1.2.3.4/32.

Also, could we add an all alias which will just match any address?

Can you do allow=ip:1.2.3.4/32 deny=all or is that implicit?

aaronhurt · 2018-02-15T13:01:09Z

You could check for a string contains and add the /32 prefix when there isn’t a match. I can see this possibly confusing some people.

I’m not sure an all is needed with the current behavior. The code currently only allows one of allow or deny to be specified. When using the allow option all sources not matched by the allow are denied (default deny). When using the deny option all sources not matched by the deny are allowed (default allow).

I tried to explain that in the added documentation as well.

aaronhurt · 2018-02-15T13:22:41Z

I added some code to add a /32 prefix to value strings that do not contain a / ... invalid or incomplete addresses will still error out in the ParseCIDR function.

magiconair · 2018-02-15T15:58:06Z

Should we do this for v6 addresses without netmask as well? — Frank Schröder

…

On 15. Feb 2018, at 14:22, Aaron Hurt ***@***.***> wrote: I added some code to add a /32 prefix to value strings that do not contain a / ... invalid or incomplete addresses will still error out in the ParseCIDR function. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aaronhurt · 2018-02-15T16:25:17Z

That's a good question ... would require a bit more pre-parsing but I think it's doable and probably good to be consistent.

aaronhurt · 2018-02-15T17:46:26Z

Latest commit should properly add a /32 prefix to plain IPv4 addresses and a /128 prefix to plain IPv6 addresses.

aaronhurt · 2018-02-15T21:58:29Z

Last commit added equal test coverage for IPv4 and IPv6 addresses.

magiconair

Can we add a simple integration test as well which sets the appropriate header to spoof the source ip?

magiconair · 2018-02-16T07:02:45Z

docs/content/cfg/_index.md

-`register=name`      | Register fabio as new service `name`. Useful for registering hostnames for host specific routes.
+Option                                     | Description
+------------------------------------------ | -----------
+`allow=ip:10.0.0.0/8,ip:192.168.0.0/24`    | Restrict access to source addresses within the `10.0.0.0/8` or `192.168.0.0/24` CIDR mask.  All other requests will be denied.


Pls add an IPv6 example as well

magiconair · 2018-02-16T07:03:42Z

docs/content/feature/access-control.md

+
+fabio supports basic ip centric access control per route.  You may
+specify one of `allow` or `deny` options per route to control access.
+Currently only source ip control is available.


Pls add an ipv6 example and explain how the source ip is determined. Also add a comment that /32 and /128 is added automatically.

magiconair · 2018-02-16T07:04:20Z

proxy/http_proxy.go

@@ -84,6 +84,11 @@ func (p *HTTPProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	if t.AccessDeniedHTTP(r) {
+		http.Error(w, "access denied", http.StatusForbidden)


I'm pretty sure that someone will ask for a different status code here but we can leave it for now.

I can see that being a request but not sure there is much more appropraite than forbidden for a denied request. Would you want that to be a config option at some point?

yeah, if someone asks for this.

magiconair · 2018-02-16T07:05:38Z

proxy/tcp/sni_proxy.go

@@ -78,6 +78,11 @@ func (p *SNIProxy) ServeTCP(in net.Conn) error {
 	}
 	addr := t.URL.Host

+	if t.AccessDeniedTCP(in) {
+		log.Print("[INFO] route rules denied access to ", t.URL.String())


This should also log who was denied access.

magiconair · 2018-02-16T07:05:49Z

proxy/tcp/tcp_proxy.go

@@ -48,6 +48,11 @@ func (p *Proxy) ServeTCP(in net.Conn) error {
 	}
 	addr := t.URL.Host

+	if t.AccessDeniedTCP(in) {
+		log.Print("[INFO] route rules denied access to ", t.URL.String())


aaronhurt · 2018-02-16T14:12:58Z

I had to export the ProcessAccessRules function that hangs off of route.Target in access_rules.go in order to process the rules for the integration test.

I also moved the deny logging into the denyByIP function in access_rules.go to make it easier to alter if needed in the future.

magiconair · 2018-02-18T21:11:51Z

proxy/http_proxy.go

@@ -84,6 +84,11 @@ func (p *HTTPProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	if t.AccessDeniedHTTP(r) {
+		http.Error(w, "access denied", http.StatusForbidden)


yeah, if someone asks for this.

magiconair · 2018-02-18T21:13:02Z

docs/content/feature/access-control.md

+The source ip used for validation against the defined ruleset is
+taken from information available in the request.
+
+For `HTTP` requests the client `RemoteAddr` is always validated


I think if the inbound connection uses the PROXY protocol then this is mapped into the RemoteAddr field of the underlying connection. This should be the case for both TCP and HTTP since this is on the connection itself.

https://github.com/fabiolb/fabio/blob/master/vendor/github.com/armon/go-proxyproto/protocol.go#L96-L113

I'll update the docs before merging.

Fixes #439

magiconair · 2018-02-18T21:34:13Z

hrmpf, I've merged this but put the additional commit into the branch. This way github won't auto-close the PR ... This is on master now.

Thanks a lot for @leprechau and @microadam for working on this!

aaronhurt force-pushed the feature/route-acl branch 2 times, most recently from 79d7e03 to c4469f9 Compare February 13, 2018 02:49

aaronhurt commented Feb 13, 2018

View reviewed changes

aaronhurt force-pushed the feature/route-acl branch from c4469f9 to 02dd9a1 Compare February 13, 2018 02:55

aaronhurt force-pushed the feature/route-acl branch 2 times, most recently from 55f1011 to 6d757dd Compare February 13, 2018 03:18

add basic ip centric access control on routes

eb57449

aaronhurt force-pushed the feature/route-acl branch from 6d757dd to eb57449 Compare February 13, 2018 03:24

prevent access control bypass via xff header

aa3073a

aaronhurt added 2 commits February 12, 2018 23:16

add test cases for http request parsing

1778892

cleanup test descriptions

1ed8a56

assume /32 prefix if not passed

25ba3ba

handle adding a prefix for plain v4 and v6 addresses

6fc06c6

add ipv6 tests

b00bfdc

magiconair reviewed Feb 16, 2018

View reviewed changes

address review comments

b5b9563

add extra documentation around source determination

e23d450

magiconair approved these changes Feb 18, 2018

View reviewed changes

magiconair added this to the 1.5.8 milestone Feb 18, 2018

magiconair added a commit that referenced this pull request Feb 18, 2018

Merge pull request #442 from myENA-feature/route-acl

4ab67c2

Fixes #439

magiconair closed this Feb 18, 2018

aaronhurt deleted the feature/route-acl branch January 22, 2020 22:30

djenriquez mentioned this pull request Feb 19, 2020

Must Access Control require RemoteAddr matching? #754

Closed

add basic ip centric access control on routes #442

add basic ip centric access control on routes #442

Conversation

aaronhurt commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronhurt commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

magiconair commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

microadam commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

microadam commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

microadam commented Feb 13, 2018

microadam commented Feb 13, 2018

aaronhurt commented Feb 13, 2018

microadam commented Feb 13, 2018

aaronhurt commented Feb 13, 2018 • edited Loading

microadam commented Feb 14, 2018 • edited Loading

microadam commented Feb 14, 2018 • edited Loading

magiconair commented Feb 14, 2018

aaronhurt commented Feb 14, 2018

aaronhurt commented Feb 14, 2018 • edited Loading

microadam commented Feb 14, 2018

aaronhurt commented Feb 14, 2018 • edited Loading

microadam commented Feb 14, 2018 via email

aaronhurt commented Feb 14, 2018

magiconair commented Feb 15, 2018

aaronhurt commented Feb 15, 2018 • edited Loading

aaronhurt commented Feb 15, 2018

magiconair commented Feb 15, 2018 via email

aaronhurt commented Feb 15, 2018

aaronhurt commented Feb 15, 2018

aaronhurt commented Feb 15, 2018

magiconair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronhurt commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magiconair commented Feb 18, 2018

aaronhurt commented Feb 13, 2018 •

edited

Loading

microadam commented Feb 14, 2018 •

edited

Loading

microadam commented Feb 14, 2018 •

edited

Loading

aaronhurt commented Feb 14, 2018 •

edited

Loading

aaronhurt commented Feb 14, 2018 •

edited

Loading

aaronhurt commented Feb 15, 2018 •

edited

Loading