Hash the value of the sticky cookie

[sylr]

Having private IPs in client cookies is something that have been flagged by external security audits done for my company's software.

[sylr]

Related to traefik/traefik#2299

[atomlab]

Hi, when do you plan merge this commit? It's very necessary improvement for security.

[sylr]

@emilevauge could you take a look a this ?

[pavelmarek77]

Any update here? I am also eager to get it implemented due our recent Penetration testing findings ..

[jbdoumenjou]

Hello, some questions about this PR.
Why not use the standard go lib for hash?
Could we consider a way to avoid hash recalculation at each call for all URLs?
WDYT?

[sylr]

Why not use the standard go lib for hash?

Only for performance reasons, it claims to be more cpu/memory efficient than the standard library.

Could we consider a way to avoid hash recalculation at each call for all URLs?

I guess we could add a map that caches the results of the hashing. Its size should not be greater than the number of backends.

[sylr]

I've added several flavours of isBackendAlive(...) with caching and different lock mechanisms in order to benchmark them.

Here the results on my machine:

$ go test -benchmem -benchtime=15s -run='^$' github.com/vulcand/oxy/roundrobin -bench '^(BenchmarkStickysessions)$'                           11:49:40
goos: darwin
goarch: amd64
pkg: github.com/vulcand/oxy/roundrobin
BenchmarkStickysessions/isBackendAlive-12                     	 2197734	      9010 ns/op	    8071 B/op	     504 allocs/op
BenchmarkStickysessions/isBackendAliveRWMutexRlock-12         	 2055895	      8796 ns/op	    5039 B/op	     251 allocs/op
BenchmarkStickysessions/isBackendAliveRWMutexLock-12          	  657480	     28608 ns/op	    5041 B/op	     252 allocs/op
BenchmarkStickysessions/isBackendAliveMutexLock-12            	  889591	     20345 ns/op	    5041 B/op	     252 allocs/op
PASS
ok  	github.com/vulcand/oxy/roundrobin	92.652s

The benchmark has been written so that it replicates a normal behavior where the list of backends does not mutate much. We can consider that the cache will be appended a few times in order to be filled and then will stay the same until the list of backends changes.

In that scenario isBackendAliveRWMutexRlock seems to be the best choice but its code is not the most readable as we need to juggle with the mutex's read and write lock states.

Let me know what you think.

[jbdoumenjou]

Hi,

Thank you very much for the benchmarks and sorry for the response time.

Considering the small performance gain compared to the complexity introduced and the fact that the StickySession architecture is initially intended to be stateless (it can be used in several roundrobin loadbalancers) it seems more relevant not to introduce a cache.

[sylr]

I reverted the hash cache experiment.

[jspdown]

sameURL use to check only for Path, Host and Scheme equality. If this change is intended could you tell us more?

[sylr]

It's either creating a net.URL out of the needle string to compare it with serverURL with sameURL(...) or Marshaling serverURL into a string to compare it with needle.

I guess both are pretty similar.

[jspdown]

It's really close except that with .String() you will also compare the User if present.

[sylr]

Ok, I did not know it was a thing to discard the user info but ok.

[sylr]

Ok so since in addition to hash I have to clone the serverURL and marshal it, I benchmarked it again and it became clear adding cache for computed values improves things a lot:

go test -benchmem -benchtime=15s -run='^$' github.com/vulcand/oxy/roundrobin -bench '^(BenchmarkStickysessions)$'
goos: darwin
goarch: amd64
pkg: github.com/vulcand/oxy/roundrobin
BenchmarkStickysessions/isBackendAlive-12         	 3489471	      5354 ns/op	       0 B/op	       0 allocs/op
BenchmarkStickysessions/isBackendAliveOld-12      	 1841678	     10067 ns/op	    8073 B/op	     504 allocs/op
PASS
ok  	github.com/vulcand/oxy/roundrobin	52.678s

The cached version is twice faster and use 0 allocs compared to previous version.

[cognusion]

I'll mention #184 again...

[jbdoumenjou]

Hi @sylr,

Sorry for the late answer.
For the sake of simplicity and knowing that the sticky mechanism could be used for several load balancers in different use cases, we prefer to have the first step without cache.
Could you please remove it accordingly?

The cache could be addressed in another PR.

[pavelmarek77]

@sylr Can we please proceed with this? Our Penetration testers are complaining about Internal IP Address Disclosed :-(

[sylr]

The PR in its current state is the most performant version according to the tests I made so I'm not willing to remove the cache.

[emilevauge]

@sylr we love the fact that you are deeply involved in this project, but open source means being able to work together, within a team. If you don't accept any of the feedback from the maintainers, this PR will never move forward and we all think it's too bad.
There are many good reasons to not include the cache in this PR, and if you don't agree with that, we plan to open a PR on our own in order to unlock this situation.
I strongly hope you will reconsider your decision, you are a great contributor :)

[sylr]

If you don't accept any of the feedback from the maintainers, this PR will never move forward and we all think it's too bad.

I believe I responded to all feedbacks except one so this is just an unfair statement.

I'll revert the cash to make this go forward.

[emilevauge]

@sylr It's perfectly fair once you take into account the tone of your answer, which is kind of aggressive.
Thanks a lot for keeping this place peaceful and effective. We are all on the same boat, with the same goal.
And thanks again for removing the cache part, we will merge this one ASAP 🙂

[cognusion]

Or, you know, #184 which allows you to pick how you want to hash it, or if you want to encrypt it.. In use for a few years, billions of requests, numerous external security audits... No cache. Just sayin'. 🕺

[ReillyBrogan]

If the other PR allows for outright encryption of the sticky cookie value (as well as hashing for those who are fine with that) then that would be preferable for us.

[jbdoumenjou]

PR #216 has been opened to use both the flexibility of #184 and the hash mechanism of #203.
WDYT?