haproxy Cookie id leaks information about software #8615

pecameron · 2016-04-22T21:09:12Z

Resolves: bz1318796
haproxy now md5 hashes the project_routename to generate the Cookie ID.

Signed-off-by: Phil Cameron [email protected]

pecameron · 2016-04-22T21:11:55Z

@openshift/networking PTAL
This is the same as the previous PR(8611) without the unwanted merge commit at the head.

pecameron · 2016-04-22T21:15:53Z

@openshift/networking The previous PR (8611) comments by @marun (and ramr) concerned whether or not we need the PREFIX/SUFFIX at all. This is part of an on-going email chain. Also, there needs to be a test case and doc changes.

marun · 2016-04-22T21:19:25Z

@pecameron It would have been preferable to edit the previous branch/PR rather that creating a new one. I recommend reading up on git's 'interactive rebase' and 'cherry picking' capabilities the next time you run into this kind of problem.

smarterclayton · 2016-04-22T22:21:45Z

pkg/router/template/router.go

+	}
+
+	// Possibly hash the name for later use in obsuring cookie ID
+	if os.Getenv("COOKIE_PREFIX_MAIN") + os.Getenv("COOKIE_PREFIX_EDGE") + os.Getenv("COOKIE_PREFIX_REENCRYPT") != "" {


Code used from inside the router needs to be parameterized from the router cmd and passed as members. Only values that are solely used within the template should be environment variables (basically, no os.Getenv except in templates or in top level CLI).

Can we just do it with a flag (in lieu of 3 env vars) that does this equivalent of "obfuscating" the cookie name. Or maybe even just do it by default?
Maybe hash it based on the tls.Termination + backendKey as it would have the type of route as well. Something like:

Hash: fmt.Sprintf("%s_%s", tls.Termination, backendKey), } // ... if r.TheObsfucatorFlagName { // replace flag name w/ the real one. config.Hash = fmt.Sprintf("%x", md5.Sum([]byte(config.Hash))) }

+1 I don't think we need infinite flexibility.

On 04/22/2016 06:21 PM, Clayton Coleman wrote:

In pkg/router/template/router.go
#8615 (comment):

@@ -398,6 +399,12 @@ func (r *templateRouter) AddRoute(id string, route *routeapi.Route, host string)
config := ServiceAliasConfig{
Host: host,
Path: route.Spec.Path,

Hash: backendKey,

}

// Possibly hash the name for later use in obsuring cookie ID

if os.Getenv("COOKIE_PREFIX_MAIN") + os.Getenv("COOKIE_PREFIX_EDGE") + os.Getenv("COOKIE_PREFIX_REENCRYPT") != "" {

Code used from inside the router needs to be parameterized from the
router cmd and passed as members. Only values that are solely used
within the template should be environment variables (basically, no
os.Getenv except in templates or in top level CLI).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8615/files/efbfa149db72f9378c1378793e9db0263996d6f0#r60810832

Thanks for taking a look. Is it OK to pass the environment variables
through the dc?I will make the changes.

@pecameron as the @eparis commented and based on the later comments, just set the hash to something like:
config.Hash = fmt.Sprintf("%x", md5.Sum([]byte(fmt.Sprintf("%s_%s", tls.Termination, backendKey)))) unconditionally and then use it `{{cfg.Hash}}`` as the cookie name in the template.
That way you don't need any environment variables.
And maybe use

On 04/25/2016 02:20 PM, Ram Ranganathan wrote:

In pkg/router/template/router.go
#8615 (comment):

@@ -398,6 +399,12 @@ func (r *templateRouter) AddRoute(id string, route *routeapi.Route, host string)
config := ServiceAliasConfig{
Host: host,
Path: route.Spec.Path,

Hash: backendKey,

}

// Possibly hash the name for later use in obsuring cookie ID

if os.Getenv("COOKIE_PREFIX_MAIN") + os.Getenv("COOKIE_PREFIX_EDGE") + os.Getenv("COOKIE_PREFIX_REENCRYPT") != "" {

@pecameron https://github.com/pecameron as the @eparis
https://github.com/eparis commented and based on the later comments,
just set the hash to something like:
|config.Hash = fmt.Sprintf("%x", md5.Sum([]byte(fmt.Sprintf("%s_%s",
tls.Termination, backendKey))))| unconditionally and then use it
|{{cfg.Hash}}|` as the cookie name in the template.

That way you don't need any environment variables.
And maybe use

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8615/files/efbfa149db72f9378c1378793e9db0263996d6f0#r60961740

Hi Ram,
Thanks! How do I test this new feature in
pkg/router/template/router_test.go?
Thanks,
phil

Look at the integration tests in test/integration/router_test.go - what you need to do is probably similar to TestRouterPathSpecificity - check that the response back from the client has a cookie + its name is "mangled".

Forgot to add - you have to check that the HTTP response has a cookie by looking at the HTTP headers, so you will also need some change to the getRouter helper function used in the test - as that currently returns only the content (not the headers).

eparis · 2016-04-23T01:55:10Z

I argue that we don't need any of this stuff. Just make the entire thing be an opaque hash. I do not believe there is any point to having any human readable words in here.

smarterclayton · 2016-04-23T15:44:36Z

Agreed on that as well

pecameron · 2016-04-25T11:43:55Z

On 04/22/2016 05:19 PM, Maru Newby wrote:

@pecameron https://github.com/pecameron It would have been
preferable to edit the previous branch/PR rather that creating a new
one. I recommend reading up on git's 'interactive rebase' and 'cherry
picking' capabilities the next time you run into this kind of problem.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8615 (comment)

I asked for help on how to do this, none was forthcoming so I figured
something out. Sorry I got it wrong.

pecameron · 2016-05-06T18:10:56Z

I mad the discussed changes, PTAL
@openshift/networking

eparis · 2016-05-06T18:41:23Z

pkg/router/template/router.go

@@ -450,6 +451,8 @@ func (r *templateRouter) AddRoute(id string, route *routeapi.Route, host string)
 		}
 	}

+	config.RoutingKeyName = fmt.Sprintf("%x", md5.Sum([]byte(fmt.Sprintf("%s %s", config.TLSTermination, backendKey))))


3 function calls and a cast on one line? seems a bit much.
I'll leave it to @ramr or @marun to tell me if just config.TLSTermination and backendKey are unique/adequate.

The use of tlsterm and route key (namespace + route name) maintains the current semantics such that if the type or name of a route changes, it's cookie id will differ accordingly. Without a salt brute force recovery would be possible, but I'm not sure that's a problem.

Yeah, I don't think I care about the 'brute force ability'

Yeah, so its just for uniqueness and it would probably become more readable if we just used a temp
variable to store the key and then hash it and set config.RoutingKey ala:

key := fmt.Sprintf("%s %s", config.TLSTermination, backendKey) config.RoutingKeyName = fmt.Sprintf("%x", md5.Sum([]byte(key)))

I will make the change.

Resolves: bz1328030 This obfuscates the cookie name my hashing the original name. Signed-off-by: Phil Cameron <[email protected]>

pecameron · 2016-05-09T12:33:22Z

I made the discussed changes, PTAL
@openshift/networking

eparis · 2016-05-09T13:33:13Z

[merge]

openshift-bot · 2016-05-09T13:35:16Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5848/) (Image: devenv-rhel7_4140)

openshift-bot · 2016-05-09T13:35:16Z

Evaluated for origin merge up to 28ab17d

openshift-bot · 2016-05-09T13:35:16Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-05-09T13:40:19Z

Evaluated for origin test up to 28ab17d

openshift-bot · 2016-05-09T14:40:20Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3678/)

smarterclayton reviewed Apr 22, 2016
View reviewed changes

pecameron force-pushed the bz1318796 branch from efbfa14 to 2b6aaf7 Compare May 6, 2016 14:54

eparis reviewed May 6, 2016
View reviewed changes

haproxy Cookie id leaks information about software

28ab17d

Resolves: bz1328030 This obfuscates the cookie name my hashing the original name. Signed-off-by: Phil Cameron <[email protected]>

pecameron force-pushed the bz1318796 branch from 2b6aaf7 to 28ab17d Compare May 9, 2016 12:30

eparis closed this May 9, 2016

eparis reopened this May 9, 2016

openshift-bot merged commit 71f310b into openshift:master May 9, 2016

haproxy Cookie id leaks information about software #8615

haproxy Cookie id leaks information about software #8615

Uh oh!

Conversation

pecameron commented Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pecameron commented Apr 22, 2016

Uh oh!

pecameron commented Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marun commented Apr 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton Apr 23, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eparis commented Apr 23, 2016

Uh oh!

smarterclayton commented Apr 23, 2016 via email

Uh oh!

pecameron commented Apr 25, 2016

Uh oh!

pecameron commented May 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pecameron commented May 9, 2016

Uh oh!

eparis commented May 9, 2016

Uh oh!

openshift-bot commented May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented May 9, 2016

Uh oh!

openshift-bot commented May 9, 2016

Uh oh!

openshift-bot commented May 9, 2016

Uh oh!

openshift-bot commented May 9, 2016

Uh oh!

Uh oh!

pecameron commented Apr 22, 2016 •

edited

Loading

pecameron commented Apr 22, 2016 •

edited

Loading

openshift-bot commented May 9, 2016 •

edited

Loading