Allow websocket authentication via protocol header #14716
Conversation
|
[test] |
| if !utf8.Valid(decodedToken) { | ||
| return nil, false, errors.New("invalid token bytes") | ||
| } | ||
| token = string(decodedToken) |
There was a problem hiding this comment.
Seems like getting to this point more than once should be a failure.
| // auth is the token authenticator to use to validate the token | ||
| auth authenticator.Token | ||
| // remove indicates whether the protocol with the token should be stripped from the incoming request | ||
| remove bool |
There was a problem hiding this comment.
Why would anyone set this to false?
|
|
||
| user, ok, err := a.auth.AuthenticateToken(token) | ||
| if ok && a.remove { | ||
| req.Header.Set(protocolHeader, strings.Join(filteredProtocols, ",")) |
There was a problem hiding this comment.
Link to the spec that indicates that repeating the header is equivalent to having the header once with a comma delimited list?
| } | ||
|
|
||
| var ( | ||
| // connectionUpgradeRegex matches any Connection header value that includes upgrade |
There was a problem hiding this comment.
Don't we already have an isUpgrade function somewhere?
|
open upstream so we only have to choose the protocol prefix once and don't end up with backward compatibility issues? |
|
[test] |
|
cc @openshift/security |
| tokenAuthenticators, | ||
| bearertoken.New(serviceAccountTokenAuthenticator), | ||
| websocket.NewProtocolAuthenticator(serviceAccountTokenAuthenticator), | ||
| paramtoken.New("access_token", serviceAccountTokenAuthenticator, true), |
There was a problem hiding this comment.
paramtoken.New has duplicate isWebSocketRequest logic.
|
continuous-integration/openshift-jenkins/test Waiting: Determining build queue position |
|
|
||
| var protocolHeader = textproto.CanonicalMIMEHeaderKey("Sec-WebSocket-Protocol") | ||
|
|
||
| var invalidToken = errors.New("invalid bearer token") |
There was a problem hiding this comment.
nit: errInvalidToken by convention
There was a problem hiding this comment.
will open follow-up upstream, but I don't plan to pick it here... we'll get both in the 1.8 rebase
|
Surprisingly small change, I like it. |
| "k8s.io/apiserver/pkg/util/wsstream" | ||
| ) | ||
|
|
||
| const bearerProtocolPrefix = "base64url.bearer.authorization.k8s.io." |
There was a problem hiding this comment.
A little weird for this to be a prefix, rather than the suffix
There was a problem hiding this comment.
I wanted to make it base64url.bearer.authorization.k8s.io/... but all reasonable separator characters are disallowed. Already merged upstream... feel strongly enough to revisit it?
There was a problem hiding this comment.
I mean I have a mild preference for ordering and we will never change it. Up to you, don't feel that strongly.
I'm going to leave it as-is. It's more readable with it as a prefix, and since the point of namespacing is to avoid conflicts, and the rest of the data contains enough entropy to authenticate, conflicts are a non-issue.
|
I mean I have a mild preference for ordering and we will never change it.
Up to you, don't feel that strongly.
On Jun 26, 2017, at 10:28 PM, Jordan Liggitt <[email protected]> wrote:
*@liggitt* commented on this pull request.
------------------------------
In
vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/authentication/request/websocket/protocol.go
<#14716 (comment)>:
+package websocket
+
+import (
+ "encoding/base64"
+ "errors"
+ "net/http"
+ "net/textproto"
+ "strings"
+ "unicode/utf8"
+
+ "k8s.io/apiserver/pkg/authentication/authenticator"
+ "k8s.io/apiserver/pkg/authentication/user"
+ "k8s.io/apiserver/pkg/util/wsstream"
+)
+
+const bearerProtocolPrefix = "base64url.bearer.authorization.k8s.io."
I wanted to make it base64url.bearer.authorization.k8s.io/... but all
reasonable separator characters are disallowed. Already merged upstream...
feel strongly enough to revisit it?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#14716 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p-adXdMCJ8bjSvwMn23MeJ080h4dks5sIGivgaJpZM4N9Gx9>
.
|
|
[merge] |
|
continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 3 |
|
Evaluated for origin merge up to 2c2e649 |
|
re[test] since the API move happened |
|
Evaluated for origin test up to 2c2e649 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2701/) (Base Commit: c7d9fb8) (PR Branch Commit: 2c2e649) |
|
flake on #14857, green otherwise |
|
No conflicts since base, lgtm. merging to get a good pick level |
Browsers do not have the ability to set headers programatically on websocket requests, all they have control over is the URL and the websocket protocols sent. Previously, we passed a bearer token as a query parameter. This PR adds support for specifying a bearer token via a websocket protocol instead, which moves it into the request headers.
Format is
base64url.bearer.authorization.k8s.io.<base64url-encoded-token>Base64-encoding the token is required, since bearer tokens can contain characters a websocket protocol may not (
/and=)fixes https://bugzilla.redhat.com/show_bug.cgi?id=1458283
docs PR is openshift/openshift-docs#4659