X-Stream-Protocol-Version header is not parsed correctly

[admilazz]

What happened:

I'm trying to establish a SPDY connection to exec into a pod. I sent this header:

X-Stream-Protocol-Version: v4.channel.k8s.io, v3.channel.k8s.io, v2.channel.k8s.io, channel.k8s.io

The request failed with Forbidden - unable to upgrade: unable to negotiate protocol: client supports [v4.channel.k8s.io, v3.channel.k8s.io, v2.channel.k8s.io, channel.k8s.io], server accepts [v4.channel.k8s.io v3.channel.k8s.io v2.channel.k8s.io channel.k8s.io]

It appears that the server is not parsing the list out of the HTTP header and is treating it as a single value.

What you expected to happen:

Protocol negotiation should succeed. RFC2616 says:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.

I can't simply send a multiple headers, because the HTTP library I'm using has no mechanism for sending separate headers as opposed to combining them. Kubernetes should parse the header whether it's given as separate headers or a list.

Anything else we need to know?:
I'm having to use SPDY because the web sockets implementation of exec seems to be broken.

Environment:

• Kubernetes version (use kubectl version):
  Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.3", GitCommit:"06ad960bfd03b39c8310aaf92d1e7c12ce618213", GitTreeState:"clean", BuildDate:"2020-02-11T18:14:22Z", GoVersion:"go1.13.6", Compiler:"gc", Platform:"linux/amd64"}
  Server Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.2", GitCommit:"66049e3b21efe110454d67df4fa62b08ea79a19b", GitTreeState:"clean", BuildDate:"2019-05-16T16:14:56Z", GoVersion:"go1.12.5", Compiler:"gc", Platform:"linux/amd64"}

• Cloud provider or hardware configuration: Azure VM

• OS (e.g: cat /etc/os-release):
  NAME="Ubuntu"
  VERSION="16.04.6 LTS (Xenial Xerus)"
  ID=ubuntu
  ID_LIKE=debian
  PRETTY_NAME="Ubuntu 16.04.6 LTS"
  VERSION_ID="16.04"
  HOME_URL="http://www.ubuntu.com/"
  SUPPORT_URL="http://help.ubuntu.com/"
  BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"
  VERSION_CODENAME=xenial
  UBUNTU_CODENAME=xenial

• Kernel (e.g. uname -a): Linux AdamArisDev 4.15.0-1063-azure e2e-test: expose minion 8080 port #68-Ubuntu SMP Fri Nov 8 09:30:20 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

• Others:
  My cluster is running in minikube v1.1.0.

[admilazz]

/sig api-machinery

[tedyu]

Thinking of the following change:

diff --git a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/httpstream.go b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/httpstream.go
index 9d5fdeeced5..65a2de32ffd 100644
--- a/staging/src/k8s.io/apimachinery/pkg/util/httpstream/httpstream.go
+++ b/staging/src/k8s.io/apimachinery/pkg/util/httpstream/httpstream.go
@@ -130,6 +130,13 @@ func Handshake(req *http.Request, w http.ResponseWriter, serverProtocols []strin
                panic(fmt.Errorf("unable to upgrade: serverProtocols is required"))
        }

+       if len(clientProtocols) == 1 && strings.Contains(clientProtocols[0], ",") {
+               cProtocols := strings.Split(clientProtocols[0], ",")
+               for i := range cProtocols {
+                       cProtocols[i] = strings.Trim(cProtocols[i], " ")
+               }
+               clientProtocols = cProtocols
+       }
        negotiatedProtocol := negotiateProtocol(clientProtocols, serverProtocols)
        if len(negotiatedProtocol) == 0 {
                for i := range serverProtocols {

[liggitt]

Since none of the accepted protocols include a comma, we should probably just iterate over all the headers, split by ",", trim each resulting item, omitting empty strings when computing clientProtocols, regardless of the number of headers.

[liggitt]

Iterate over req.Header[http.CanonicalHeaderKey(HeaderProtocolVersion)]

[admilazz]

Thanks for the quick update. I don't know what the expected HTTP behavior is when you use multiple headers and also have lists in each header, but if the same rule applies that multiple headers should be treated the same as one header with the items separated by commas, then presumably:

Foo: a, b
Foo: c, d

should be the same as

Foo: a, b, c, d

in which case @liggitt's suggestion of basically moving the split into the inner loop would be more correct.

In any case, I suppose a bug fix of this nature would only affect newer versions of Kubernetes? (I don't know if you do bug-fix-only patches to older versions.) I guess as a workaround I could try to catch the Forbidden error, look for the "unable to negotiate protocol" text, and switch to using only a single header value for older Kubernetes versions. Or is there a better approach?

[admilazz]

That said, presumably this could also be handled in a lower layer - in the HTTP layer where headers are parsed - which might fix similar bugs with other headers that might exist elsewhere. (I think the only general exception to the equivalency rule is the Set-Cookie header because in practice cookies can contain commas but aren't quoted.)

[liggitt]

I suppose a bug fix of this nature would only affect newer versions of Kubernetes? (I don't know if you do bug-fix-only patches to older versions.)

That's correct.

I guess as a workaround I could try to catch the Forbidden error, look for the "unable to negotiate protocol" text, and switch to using only a single header value for older Kubernetes versions. Or is there a better approach?

Given all the currently supported protocols are available in all supported kubernetes versions (the most recent protocols were added in #13885 in v1.2.0 and in #26541 in v1.4.0), I'd probably just pick the specific protocol you want. If you wanted to detect a protocol failure, the X-Accepted-Stream-Protocol-Versions header is sent on a forbidden response due to protocol negotiation failure.

[admilazz]

That's good to know. I can just stick to v4, then, since I don't expect to need anything that old. :-) Thanks.

[zhouya0]

/reopen

That said, presumably this could also be handled in a lower layer - in the HTTP layer where headers are parsed - which might fix similar bugs with other headers that might exist elsewhere. (I think the only general exception to the equivalency rule is the Set-Cookie header because in practice cookies can contain commas but aren't quoted.)

I agree, should this fix be more general? Some other http headers have also the comma problems.

[k8s-ci-robot]

@zhouya0: Reopened this issue.

In response to this:

/reopen

That said, presumably this could also be handled in a lower layer - in the HTTP layer where headers are parsed - which might fix similar bugs with other headers that might exist elsewhere. (I think the only general exception to the equivalency rule is the Set-Cookie header because in practice cookies can contain commas but aren't quoted.)

I agree, should this fix be more general? Some other http headers have also the comma problems.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[liggitt]

That said, presumably this could also be handled in a lower layer - in the HTTP layer where headers are parsed - which might fix similar bugs with other headers that might exist elsewhere. (I think the only general exception to the equivalency rule is the Set-Cookie header because in practice cookies can contain commas but aren't quoted.)

I agree, should this fix be more general? Some other http headers have also the comma problems.

I would not recommend that, as not all headers value handling can safely assume commas are value delimiters.

[liggitt]

I would keep this issue scoped to this particular header

[fedebongio]

/assign @liggitt
(since you are already working on this one, thanks!)

[liggitt]

/close

fixed in #89857

[k8s-ci-robot]

@liggitt: Closing this issue.

In response to this:

/close

fixed in #89857

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.