-
Notifications
You must be signed in to change notification settings - Fork 40.9k
docker credential JSON: validate non UTF-8 auth #132104
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
base: master
Are you sure you want to change the base?
Conversation
Fixes: - kubernetes#131982 Signed-off-by: Tomasz Janiszewski <[email protected]>
Welcome @janisz! |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
Hi @janisz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
/release-note-edit
|
/ok-to-test |
Signed-off-by: Tomasz Janiszewski <[email protected]>
Adding sig node: /sig node |
/assign @sreeram-venkitesh |
Since this may potentially break things, let's add a feature gate around it which is enabled by default. Please see the |
Signed-off-by: Tomasz Janiszewski <[email protected]>
@SergeyKanzhelev good point. I added feature gate |
please fix linter by properly adding the feature gates everywhere it needs to be declared |
Signed-off-by: Tomasz Janiszewski <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: janisz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold Is this fixing a real or theoretical issue? A remote system that is vulnerable to non-utf8 characters presented in authentication headers by untrusted users doesn't seem like Kubernetes' issue to protect against. I'm not convinced restricting the character set of the password is a reasonable thing to do. |
@liggitt Good point! I do not have any exploit for this so it's a theoretical issue although since all other strings that k8s operates on are UTF-8 (validated) this can cause problems (e.g. stackrox/stackrox#15270). HTTP does not specify encoding (https://stackoverflow.com/a/703341/1387612) so in theory it may work with any bytes but I guess in most cases it will be a configuration error not a planned behaviour.
|
We do not have a lot of places where the non UTF-8 string will be accepted by k8s. This seems to be one of the few places. I think it is not a big deal to limit this to avoid any possible issues with handling extra characters. If we are not limiting the character set, I think it may be a good thing to test this end-to-end with some really nasty characters. |
@janisz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
PR needs rebase. 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-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #131982
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: