Use specified ServerName in aggregator TLS validation #49353

liggitt · 2017-07-21T04:46:36Z

The aggregator sets a ServerName in the proxier tlsConfig, but the code path handling websocket upgrade requests did not honor it, and instead tried to verify TLS using the dialed host

Honors ServerName if already set in tls.Config
Adds unit tests for upgrade functionality via the aggregator
Fixes mutation of shared tlsConfig.ServerName in spdy roundtripper

Websocket requests to aggregated APIs now perform TLS verification using the service DNS name instead of the backend server's IP address, consistent with non-websocket requests.

liggitt · 2017-07-21T04:52:46Z

cc @kubernetes/sig-api-machinery-pr-reviews @kubernetes/sig-api-machinery-bugs

liggitt · 2017-07-21T06:08:01Z

/retest

deads2k · 2017-07-21T13:04:19Z

@ncdc

liggitt · 2017-07-21T13:07:06Z

staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.go

@@ -158,27 +158,28 @@ func (s *SpdyRoundTripper) dial(req *http.Request) (net.Conn, error) {
 		return nil, err
 	}



this bit fixes two existing bugs:

mutation of the roundtripper

mutation of the original tlsConfig (which the round tripper should not assume it is free to modify)

ncdc · 2017-07-21T13:14:41Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go

+		path := "/apis/" + tc.APIService.Spec.Group + "/" + tc.APIService.Spec.Version + "/foo"
+		called := false
+
+		func() { // Cleanup after each test case.


Can you use subtests?

tried it, it triggered the race detector on references to the iterators, didn't feel like messing with it

ncdc · 2017-07-21T13:14:47Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go

@@ -218,3 +226,189 @@ func TestProxyHandler(t *testing.T) {
 		}()
 	}
 }
+
+func TestProxyUpgrade(t *testing.T) {
+


ncdc · 2017-07-21T13:24:13Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy_test.go

-					Service:               &apiregistration.ServiceReference{},
-					Group:                 "foo",
-					Version:               "v1",
-					InsecureSkipTLSVerify: true,


I take it this is now part of TestProxyUpgrade?

actually, I can keep this one intact and add a caBundle test... I wanted a TLS test for normal connections

liggitt · 2017-07-21T15:12:42Z

test nits addressed

ncdc · 2017-07-21T16:02:05Z

/lgtm

liggitt · 2017-07-21T16:20:46Z

/test pull-kubernetes-federation-e2e-gce

deads2k · 2017-07-21T16:21:20Z

/approve

k8s-github-robot · 2017-07-21T16:22:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, ncdc

Associated issue: 49354

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k,liggitt]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-21T17:45:00Z

Automatic merge from submit-queue

janetkuo · 2017-07-21T18:26:32Z

@liggitt

k8s.io/kubernetes/vendor/k8s.io/kube-aggregator/pkg/apiserver TestProxyUpgrade seems to be flaky:

go test -v k8s.io/kubernetes/vendor/k8s.io/kube-aggregator/pkg/apiserver -run TestProxyUpgrade$
handler_proxy_test.go:328: invalid hostname + insecure: expected called=true, got false
testing.go:610: race detected during execution of test

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/48746/pull-kubernetes-unit/42230/

liggitt · 2017-07-21T19:02:53Z

@janetkuo looking...

liggitt · 2017-07-21T19:32:40Z

@janetkuo fixed in #49404

Automatic merge from submit-queue fix race in proxy unit test fixes #49413, follow up to #49353 to fix a race in the unit test: ``` go test -v k8s.io/kubernetes/vendor/k8s.io/kube-aggregator/pkg/apiserver -run TestProxyUpgrade$ handler_proxy_test.go:328: invalid hostname + insecure: expected called=true, got false testing.go:610: race detected during execution of test ``` https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/48746/pull-kubernetes-unit/42230/

wojtek-t · 2017-07-25T11:21:17Z

Cherrypick in #49560

liggitt · 2017-07-25T11:39:25Z

Was picked in #49403

liggitt · 2017-07-25T11:51:36Z

Pick in #49403 includes the unit test race fix

#49353-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #49212 upstream release 1.7 Cherry pick of #49212 on release-1.7. #49212: Bump rescheduler version to v0.3.1

…3-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #49353 Cherry pick of #49353 on release-1.7. #49353: Use specified ServerName in aggregator TLS validation

k8s-cherrypick-bot · 2017-07-26T08:48:52Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-github-robot assigned thockin and deads2k Jul 21, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 21, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2017

k8s-github-robot added the release-note-label-needed label Jul 21, 2017

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. labels Jul 21, 2017

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherrypick-candidate and removed release-note-label-needed labels Jul 21, 2017

liggitt added this to the v1.7 milestone Jul 21, 2017

liggitt force-pushed the aggregator-tls branch from 86acff2 to 33c30e6 Compare July 21, 2017 05:05

liggitt commented Jul 21, 2017

View reviewed changes

ncdc reviewed Jul 21, 2017

View reviewed changes

liggitt force-pushed the aggregator-tls branch from 33c30e6 to b5008ef Compare July 21, 2017 15:12

Use specified ServerName in aggregator TLS validation

bd9b463

liggitt force-pushed the aggregator-tls branch from b5008ef to bd9b463 Compare July 21, 2017 15:39

k8s-ci-robot assigned ncdc Jul 21, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2017

k8s-github-robot merged commit 971c247 into kubernetes:master Jul 21, 2017

liggitt deleted the aggregator-tls branch July 21, 2017 17:47

janetkuo mentioned this pull request Jul 21, 2017

Add a new API version apps/v1beta2 #48746

Merged

liggitt mentioned this pull request Jul 21, 2017

Automated cherry pick of #49353 #49403

Merged

liggitt mentioned this pull request Jul 21, 2017

fix race in proxy unit test #49404

Merged

wojtek-t mentioned this pull request Jul 25, 2017

Automated cherry pick of #49212 upstream release 1.7 #49560

Merged

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 25, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use specified ServerName in aggregator TLS validation #49353

Use specified ServerName in aggregator TLS validation #49353

liggitt commented Jul 21, 2017 •

edited

Loading

liggitt commented Jul 21, 2017

liggitt commented Jul 21, 2017

deads2k commented Jul 21, 2017

liggitt Jul 21, 2017

ncdc Jul 21, 2017

liggitt Jul 21, 2017

ncdc Jul 21, 2017

ncdc Jul 21, 2017

liggitt Jul 21, 2017

liggitt commented Jul 21, 2017

ncdc commented Jul 21, 2017

liggitt commented Jul 21, 2017

deads2k commented Jul 21, 2017

k8s-github-robot commented Jul 21, 2017

k8s-github-robot commented Jul 21, 2017

janetkuo commented Jul 21, 2017

liggitt commented Jul 21, 2017

liggitt commented Jul 21, 2017

wojtek-t commented Jul 25, 2017

liggitt commented Jul 25, 2017

liggitt commented Jul 25, 2017

k8s-cherrypick-bot commented Jul 26, 2017

		@@ -158,27 +158,28 @@ func (s SpdyRoundTripper) dial(req http.Request) (net.Conn, error) {
		return nil, err
		}

Use specified ServerName in aggregator TLS validation #49353

Use specified ServerName in aggregator TLS validation #49353

Conversation

liggitt commented Jul 21, 2017 • edited Loading

liggitt commented Jul 21, 2017

liggitt commented Jul 21, 2017

deads2k commented Jul 21, 2017

liggitt Jul 21, 2017

Choose a reason for hiding this comment

ncdc Jul 21, 2017

Choose a reason for hiding this comment

liggitt Jul 21, 2017

Choose a reason for hiding this comment

ncdc Jul 21, 2017

Choose a reason for hiding this comment

ncdc Jul 21, 2017

Choose a reason for hiding this comment

liggitt Jul 21, 2017

Choose a reason for hiding this comment

liggitt commented Jul 21, 2017

ncdc commented Jul 21, 2017

liggitt commented Jul 21, 2017

deads2k commented Jul 21, 2017

k8s-github-robot commented Jul 21, 2017

k8s-github-robot commented Jul 21, 2017

janetkuo commented Jul 21, 2017

liggitt commented Jul 21, 2017

liggitt commented Jul 21, 2017

wojtek-t commented Jul 25, 2017

liggitt commented Jul 25, 2017

liggitt commented Jul 25, 2017

k8s-cherrypick-bot commented Jul 26, 2017

liggitt commented Jul 21, 2017 •

edited

Loading