Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

haproxy obfuscated internal IP in routing cookie #8334

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
6 participants
@pecameron
Copy link
Contributor

commented Apr 1, 2016

The cookie currently returns the openshift internal pod IP address.
This is a security issue as an attacker can develop a map of the pods
in the cluster just by observing the returned cookie.

This change returns a hash of the internal address and internal service
name to obfuscate the internal information. The service name is configured
when the service is created and is not visible to outside users. This
in combination with the internal ip:port is hashed and presented in the
cookie.

addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1318796

return svc.EndpointTable
}
endpoints := make([]Endpoint, 0, len(svc.EndpointTable))
for i := range svc.EndpointTable {
endpoint := svc.EndpointTable[i]
if endpoint.PortName == alias.PreferPort || endpoint.Port == alias.PreferPort {
s := endpoint.ID+endpoint.TargetName+endpoint.PortName
endpoint.IDhash = fmt.Sprintf("%x", md5.Sum([]byte(s)))

This comment has been minimized.

Copy link
@knobunc

knobunc Apr 4, 2016

Contributor

Why is this inconsistent with the call a few lines above. I'd lose hash above.

@@ -172,12 +173,20 @@ func newTemplateRouter(cfg templateRouterCfg) (*templateRouter, error) {

func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
if len(alias.PreferPort) == 0 {
for i := range svc.EndpointTable {
endpoint := svc.EndpointTable[i]
s := endpoint.ID+endpoint.TargetName+endpoint.PortName

This comment has been minimized.

Copy link
@knobunc

knobunc Apr 4, 2016

Contributor

Please put spaces around the +s to be consistent with the rest of the code.

Also, please document what we are hashing in a comment, and why we are adding the name (as protection against rainbow tables).

@pecameron pecameron force-pushed the pecameron:bz1318796 branch 2 times, most recently from 1be8c21 to 3e5d176 Apr 4, 2016

@@ -172,12 +173,20 @@ func newTemplateRouter(cfg templateRouterCfg) (*templateRouter, error) {

func endpointsForAlias(alias ServiceAliasConfig, svc ServiceUnit) []Endpoint {
if len(alias.PreferPort) == 0 {
for i := range svc.EndpointTable {
endpoint := svc.EndpointTable[i]

This comment has been minimized.

Copy link
@eparis

eparis Apr 4, 2016

Member

Assuming this is necessary (it's weird to me you are walking this table twice, but I haven't actually looked at it) I think the idiomatic go way to update the values stored in items in a slice would be:

for i := range svc.EndpointTable {
    endpoint := &svc.EndpointTable[i]
    hash := $WHATEVER
    endpoint.IDHash = fmt.Sprintf(....hash)
}

Notice endpoint is a pointer to the value.

Now if I were going to write it your way, because I didn't really understand or want to use pointers but did want to make use of the return values of range it should look like:

for i, endpoint := range svc.EndpointTable {
    hash := $WHATEVER
    svc.EndpointTable[i].IDhash = fmt.Sprintf("%x", hash)
}

@pecameron pecameron force-pushed the pecameron:bz1318796 branch from 3e5d176 to d5a28c7 Apr 4, 2016

if len(alias.PreferPort) == 0 {
for i := range svc.EndpointTable {
endpoint := svc.EndpointTable[i]

This comment has been minimized.

Copy link
@knobunc

knobunc Apr 4, 2016

Contributor

I think you missed @eparis' comment about the go idioms.

It's ugly that we have to do the same thing in two places though. Perhaps make a private function that computes a hash value from an Endpoints structure?

This comment has been minimized.

Copy link
@pecameron

pecameron Apr 4, 2016

Author Contributor

Didn't miss it, misunderstood it. I will make a private function as you
suggest.
On 04/04/2016 01:54 PM, Ben Bennett wrote:

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

if len(alias.PreferPort) == 0 {
  •   for i := range svc.EndpointTable {
    
  •       endpoint := svc.EndpointTable[i]
    

I think you missed @eparis https://github.com/eparis' comment about
the go idioms.

It's ugly that we have to do the same thing in two places though.
Perhaps make a private function that computes a hash value from an
Endpoints structure?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8334/files/d5a28c792427c49fed6676cc043d4a13b621d62c#r58417984

This comment has been minimized.

Copy link
@ramr

ramr Apr 4, 2016

Contributor

Better to do this when we add an endpoint - see: https://github.com/pecameron/origin/blob/bz1318796/pkg/router/template/router.go#L507 rather than in here endpointsForAlias.

Reason is endpointsForAlias is called every time we handle the template (router config) and there would be multiple calls in the lifetime of a router (as endpoints/routes get added/removed/modified). It's not performant to calculate the IDHash for all the endpoints on every single call to endpointsForAlias.
Just do it once when we add the endpoint.

That way, you can also inline the function as it will be only used in a single place.

@@ -214,7 +214,7 @@ backend be_edge_http_{{$cfgIdx}}
{{ end }}
http-request set-header Forwarded for=%[src];host=%[req.hdr(host)];proto=%[req.hdr(X-Forwarded-Proto)]
{{ range $idx, $endpoint := endpointsForAlias $cfg $serviceUnit }}
server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} check inter 5000ms cookie {{$endpoint.ID}}
server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} check inter 5000ms cookie {{$endpoint.IDhash}}

This comment has been minimized.

Copy link
@ramr

ramr Apr 4, 2016

Contributor

Needs to be server {{$endpoint.IDHash}} {{$endpoint.IP}} ...
The server id is set to {{$endpoint.ID}} - it should match the IDHash - that value from the cookie is used to pick a specific server.

Also IDHash seems more consistent as a name than IDhash.

This comment has been minimized.

Copy link
@pecameron

pecameron Apr 4, 2016

Author Contributor

Ram,
Thanks, I will make those changes.
phil
On 04/04/2016 02:29 PM, Ram Ranganathan wrote:

In images/router/haproxy/conf/haproxy-config.template
#8334 (comment):

@@ -214,7 +214,7 @@ backend be_edge_http_{{$cfgIdx}}
{{ end }}
http-request set-header Forwarded for=%[src];host=%[req.hdr(host)];proto=%[req.hdr(X-Forwarded-Proto)]
{{ range $idx, $endpoint := endpointsForAlias $cfg $serviceUnit }}

  • server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} check inter 5000ms cookie {{$endpoint.ID}}
  • server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} check inter 5000ms cookie {{$endpoint.IDhash}}

Needs to be |server {{$endpoint.IDHash}} {{$endpoint.IP}} ...|
The server |id| is set to |{{$endpoint.ID}}| - it should match the
IDHash - that value from the cookie is used to pick a specific server.

Also |IDHash| seems more consistent as a name than |IDhash|.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8334/files/d5a28c792427c49fed6676cc043d4a13b621d62c#r58423905

@@ -236,7 +236,7 @@ backend be_secure_{{$cfgIdx}}
timeout check 5000ms
cookie OPENSHIFT_REENCRYPT_{{$cfgIdx}}_SERVERID insert indirect nocache httponly secure
{{ range $idx, $endpoint := endpointsForAlias $cfg $serviceUnit }}
server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} ssl check inter 5000ms verify required ca-file {{ $workingDir }}/cacerts/{{$cfgIdx}}.pem cookie {{$endpoint.ID}}
server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} ssl check inter 5000ms verify required ca-file {{ $workingDir }}/cacerts/{{$cfgIdx}}.pem cookie {{$endpoint.IDhash}}

This comment has been minimized.

Copy link
@ramr

ramr Apr 4, 2016

Contributor

Same change as above needed here - needs to be server {{$endpoint.IDHash}} {{$endpoint.IP}} ... to match up to the cookie.

@@ -62,6 +62,7 @@ type Endpoint struct {
Port string
TargetName string
PortName string
IDhash string

This comment has been minimized.

Copy link
@ramr

ramr Apr 4, 2016

Contributor

IdHash for consistency with the other variable names. And that affects the prior comments re: using IDHash for the server id.

This comment has been minimized.

Copy link
@pecameron

pecameron Apr 4, 2016

Author Contributor

I will make the change.
phil
On 04/04/2016 02:38 PM, Ram Ranganathan wrote:

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

@@ -62,6 +62,7 @@ type Endpoint struct {
Port string
TargetName string
PortName string

  • IDhash string

|IdHash| for consistency with the other variable names. And that
affects the prior comments re: using |IDHash| for the server id.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/8334/files/d5a28c792427c49fed6676cc043d4a13b621d62c#r58425446

@pecameron pecameron force-pushed the pecameron:bz1318796 branch from d5a28c7 to 58aca5b Apr 4, 2016

@knobunc

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

This looks much better. LGTM.

@ramr

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

Code LGTM - If you could also add to the AddEndpoints test here: https://github.com/ramr/origin/blob/master/pkg/router/template/router_test.go#L68
just check that the actualEp.IdHash is set and matches expectations.

@ramr

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

As discussed over email - testing that the IdHash is valid looks fine but as re: the test failures for:

--- FAIL: TestAddEndpointDuplicates (0.00s)  
    router_test.go:123: add same endpoints expected to return false but got true  
W0405 15:21:34.318716   26055 router.go:696] a edge terminated route with host edgetermfalse does   not have the required certificates. The route will still be created but no certificates will be written  
W0405 15:21:34.319183   26055 router.go:696] a reencrypt terminated route with host  reencrypttermfalse does not have the required certificates.  The route will still be created but no   certificates will be written  
FAIL  

There is a check above the added code to ensure that if we add the same endpoints, we don't reload the router (return false from here basically since the config is unchanged). But since we now set the IdHash below this check, the 2 objects will never match up. So better to just move setting the IdHash to plugin.go#createRouterEndpoints.

@pecameron pecameron force-pushed the pecameron:bz1318796 branch from 58aca5b to c78ed19 Apr 6, 2016

@pecameron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2016

Above failure seems to be a flake:

I0406 16:14:51.933815 23883 client.go:320] Found registry v2 API at https://registry-1.docker.io/v2/
--- FAIL: TestRegistryClientImageV2-2 (0.12s)
dockerregistryclient_test.go:206: tag "latest" has not been set on repository "openshift/origin"
FAIL

@knobunc

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2016

[test]

@pecameron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

[FLAKE:8466] problem is not in networking.

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

[test]

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

[merge] because looks like the last failure is #8480

@pecameron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2016

[TEST]

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

looks like you messed up the rebase quite badly...

@pecameron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2016

eparis, help! I sent you email

haproxy obfuscated internal IP in routing cookie
The cookie currently returns the openshift internal pod IP address.
This is a security issue as an attacker can develop a map of the pods
in the cluster just by observing the returned cookie.

This change returns a hash of the internal address and internal service
name to obfuscate the internal information. The service name is configured
when the service is created and is not visible to outside users. This
in combination with the internal ip:port is hashed and presented in the
cookie.

addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1318796

@pecameron pecameron force-pushed the pecameron:bz1318796 branch from f2431e3 to 36c680b Apr 18, 2016

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

approved and [merge] failed test looks unrelated.

@openshift-bot

This comment has been minimized.

Copy link

commented Apr 18, 2016

Evaluated for origin test up to 36c680b

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

[merge] again because the failure is unrelated crap.

@pecameron

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

@eparis Still failing. Could you try the merge again?

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

[merge] with extreme prejudice

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

This is [merge] try number 5
@danmcp I'm guessing there is nothing we can do about the aws m4.large failures?

@danmcp

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

@eparis We have done some stuff. Most of the tests are no longer using them (they didn't really need them anyway). We can also switch zones if it continues to be a problem. I need to (or someone does) to do a little research to pick the best one to move to if we are going to switch.

@openshift-bot

This comment has been minimized.

Copy link

commented Apr 19, 2016

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

@openshift-bot

This comment has been minimized.

Copy link

commented Apr 19, 2016

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

@eparis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

[merge] you silly PR. merge!

@openshift-bot

This comment has been minimized.

Copy link

commented Apr 19, 2016

Evaluated for origin merge up to 36c680b

@openshift-bot openshift-bot merged commit 2d4cec7 into openshift:master Apr 19, 2016

2 of 3 checks passed

continuous-integration/openshift-jenkins/test Failed
Details
continuous-integration/openshift-jenkins/merge Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pecameron pecameron deleted the pecameron:bz1318796 branch Apr 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.