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

Check for port before setting Origin header for watch requests. #1669

Merged
merged 3 commits into from Aug 1, 2019

Conversation

@kjcjohnson
Copy link
Contributor

commented Jul 27, 2019

When the Kubernetes URL used doesn't specify a port
(e.g., https://example.com/api/v1/...), the origin header for
watch requests ends up with a port of -1 (e.g., https://example.com:-1).
This happens because calling getPort() on a java.net.URL object that
does not have a port explicitly specified will always return -1. The
return value was always just inserted into the origin header.

Now, we check for this and only append a port to the origin header if
getPort() returns something other than -1. We make this change in both
the WatchConnectionManager and WatchHTTPManager classes.

I wasn't able to come up with a way to make a test for this, because the
mock Kubernetes server doesn't support specifying the port, and especially not
the defaults of 80/443.

Fixes #1667

Check for port before setting Origin header for watch requests.
 When the Kubernetes URL used doesn't specify a port
(e.g., https://example.com/api/v1/...), the origin header for
watch requests ends up with a port of -1 (e.g. https://example.com:-1).
This happens because calling `getPort()` on a java.net.URL object that
does not have a port explicitly specified will always return -1. The
return value was always just inserted into the origin header.

Now, we check for this and only append a port to the origin header if
`getPort()` returns something other than -1. We make this change in both
the WatchConnectionManager and WatchHTTPManager classes.
@centos-ci

This comment has been minimized.

Copy link

commented Jul 27, 2019

Can one of the admins verify this patch?

@oscerd
oscerd approved these changes Jul 28, 2019
@gastaldi

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

Ok to test

@rohanKanojia

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

ok to test

@cjellick

This comment has been minimized.

Copy link

commented Aug 1, 2019

Do we need an origin header at all? That’s really just useful for protecting browsers from cross site scripting.

Could we simply drop the header all together?

@rohanKanojia

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

@kjcjohnson: Could you please test your patch without header? If it's not breaking anything then let's drop it.

@kjcjohnson

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

It looks like dropping the origin header won't work. I took some Wireshark captures of the packets, and they're identical other than the Origin header, but only the request with the header succeeds:

Without Origin header:

GET /k8s/clusters/c-scffx/api/v1/namespaces/test-ns/pods?fieldSelector=metadata.name%3Dtestpod-1sntr&watch=true HTTP/1.1
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Key: 1Ass6sr+wPqYyEddMR7FPQ==
Sec-WebSocket-Version: 13
Authorization: Bearer <token>
Host: rancher.example.com
Accept-Encoding: gzip
User-Agent: okhttp/3.12.0

HTTP/1.1 403 Forbidden
X-Powered-By: ARR/3.0
X-Powered-By: ASP.NET
Date: Thu, 01 Aug 2019 19:00:43 GMT
Content-Length: 0

With Origin header:

GET /k8s/clusters/c-scffx/api/v1/namespaces/test-ns/pods?fieldSelector=metadata.name%3Dtestpod-rfw80&watch=true HTTP/1.1
Origin: http://rancher.example.com
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Key: zmA6ry8uoV5uumoYhh8rCg==
Sec-WebSocket-Version: 13
Authorization: Bearer <token>
Host: rancher.example.com
Accept-Encoding: gzip
User-Agent: okhttp/3.12.0

HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Sec-WebSocket-Accept: WcnYoYGI9IQ0piiOUMVaDwgV1OU=
X-Powered-By: ARR/3.0
Connection: Upgrade
X-Powered-By: ASP.NET
Date: Thu, 01 Aug 2019 19:25:45 GMT
CHANGELOG.md Outdated Show resolved Hide resolved
Whitespace cleanup in CHANGELOG.md
Co-Authored-By: George Gastaldi <gegastaldi@gmail.com>
@gastaldi

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

[merge]

@fusesource-ci fusesource-ci merged commit aaeb24f into fabric8io:master Aug 1, 2019

5 of 6 checks passed

merge Build started for merge commit.
Details
ci/circleci: OPENSHIFT_3.10.0 Your tests passed on CircleCI!
Details
ci/circleci: OPENSHIFT_3.11.0 Your tests passed on CircleCI!
Details
ci/circleci: OPENSHIFT_3.9.0 Your tests passed on CircleCI!
Details
license-check License headers all applied properly - thanks!
Details
test All tests passed!
Details
@fusesource-ci

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Pull request is not mergeable.

@kjcjohnson kjcjohnson deleted the kjcjohnson:iss_1667 branch Aug 3, 2019

carlossg added a commit to jenkinsci/kubernetes-plugin that referenced this pull request Aug 24, 2019
[JENKINS-59000] Upgrade kubernetes-client to 4.4.2
To get the fix in fabric8io/kubernetes-client#1669

Origin header is set with port -1 when no port is present in the Kubernetes API url
carlossg added a commit to jenkinsci/kubernetes-plugin that referenced this pull request Aug 24, 2019
[JENKINS-59000] Upgrade kubernetes-client to 4.4.2
To get the fix in fabric8io/kubernetes-client#1669

Origin header is set with port -1 when no port is present in the Kubernetes API url

Causing an error on exec

```
java.net.ProtocolException: Expected HTTP 101 response but was '403 Forbidden'
```
daxmc99 added a commit to daxmc99/pipeline-jenkins-server that referenced this pull request Aug 26, 2019
Update plugins.txt
Update plugins to pickup fix for origin headers found here fabric8io/kubernetes-client#1669
daxmc99 added a commit to daxmc99/pipeline-jenkins-server that referenced this pull request Aug 29, 2019
cjellick added a commit to rancher/pipeline-jenkins-server that referenced this pull request Aug 29, 2019
Update plugins.txt & update dockerfile for kubernetes plugin
Update plugins to pickup fix for origin headers found here fabric8io/kubernetes-client#1669
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.