Repository navigation
Fix websocket proxy auth truncating passwords that contain a colon - #2738
Conversation
websocket_proxycare() decoded the proxy-authorization header and split it on every ':', then indexed auth[0] and auth[1]. RFC 7617 allows ':' in the password (only the user-id may not contain one), so any proxy password containing a colon was silently truncated to the text before the first colon, sending the wrong credentials to the proxy. A header with no colon at all raised an unhandled IndexError. Split on the first colon only and only set http_proxy_auth when a password half is actually present.
|
Welcome @Akshaay1! |
|
/release-note-none |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Akshaay1, yliaog The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
c8c4804
into
kubernetes-client:master
| if len(auth) == 2: | ||
| connect_opt.update({'http_proxy_auth': (auth[0], auth[1]) }) |
There was a problem hiding this comment.
With a credential that has no colon, this now silently skips http_proxy_auth: the CONNECT goes out without Proxy-Authorization, and the user gets failed CONNECT via proxy status: 407 instead of an error about the header. Could we raise an ApiValueError here (bad base64 or non-UTF-8 credentials already raise in this block), and have the useronly test case expect it?
| @@ -599,8 +599,9 @@ def websocket_proxycare(connect_opt, configuration, url, headers): | |||
| for key,value in configuration.proxy_headers.items(): | |||
| if key == 'proxy-authorization' and value.startswith('Basic'): | |||
| b64value = value.split()[1] | |||
There was a problem hiding this comment.
The description also mentions a bare Basic header, but that still raises IndexError here. Could it get the same error?
|
@Akshaay1 thanks for the fix. This merged before my review went out. Could you send a small follow-up PR for the two inline comments (raise a clear error for colon-less or bare |
Follow-up to kubernetes-client#2738. A proxy-authorization header whose decoded Basic credentials contain no colon was silently skipped, so the CONNECT went out without Proxy-Authorization and failed with a confusing 407. A bare "Basic" header with no credentials still raised IndexError. Both cases now raise ApiValueError with a clear message, and the "useronly" test case expects it.
Problem
websocket_proxycare()decodes theproxy-authorizationheader for websocket calls (exec/attach/port-forward through an authenticated HTTP proxy) and splits the decodeduser:passstring on every::Per RFC 7617, the password half of Basic auth credentials may contain
:— only the user-id may not. A proxy password containing a colon (e.g.pa:ss:word) is silently truncated to the text before the first colon, so the websocket client authenticates to the proxy with the wrong password. A header with no colon at all (or justBasicwith no credentials) raises an unhandledIndexError.Root Cause
str.split(':')with no limit splits on every colon, and the code unconditionally indexesauth[0]/auth[1]assuming exactly two parts.Solution
split(':', 1)), so the rest of the password is preserved.http_proxy_authwhen the split actually produced both halves, instead of indexing unconditionally — this turns the previousIndexErroron a colon-less/malformed header into a no-op (same as if noproxy-authorizationheader were present).Testing
test_websocket_proxycareinkubernetes/stream/ws_client_test.pywith two cases: a password containing colons (user:pa:ss:word), and a header with no colon at all (useronly).pytest kubernetes/stream/ws_client_test.py(24 passed) andpytest kubernetes/stream/(full module, all passing).master(AssertionError: ('user', 'pa') == ('user', 'pa:ss:word')for the colon case,IndexError: list index out of rangefor the no-colon case) and pass with this fix.Related Issue
Closes #2734