-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow for '/' in context name #37
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it |
CLAs look good, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for catching this. It's indeed an edge case. Do you manually maintain your kubeconfig entries? I'm curious what generates a context name with /
in it. :)
echo "${KUBENS_DIR}/${ctx}" | ||
} | ||
|
||
read_namespace() { | ||
local f | ||
f="$(namespace_file "${1}")" | ||
[[ -f "${f}" ]] && cat "${f}" | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand why this exists. Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said in 6610d70, the https://github.com/koalaman/shellcheck/wiki/SC2155 prevents masking the return value. In case $f does not exists the return value is 1, and since you're using set -eou
this aborts the script.
A non-existent file in this case is just fine, so I again mask the return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it now when I executed it myself with DEBUG=1 ./kubens
. Thanks so much for noticing this.
I guess this works, because in both places read_namespace
is called, empty output will do the job.
kubens
Outdated
@@ -54,14 +54,15 @@ get_namespaces() { | |||
} | |||
|
|||
namespace_file() { | |||
local ctx="${1}" | |||
local ctx="${1//\//-}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind creating a
escape_context_name() {
echo "${1//\//-}"
}
and using it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, added it.
Also, fix `kubens -` which fails to save previous namespace after 6610d70
I am renaming the contexts generated by a tool :) There are just too-many-dashes-in-the-generated-name. I needed a delimiter other than - or _ and one that implies hierarchy. Slashes won the contest :) |
echo "${KUBENS_DIR}/${ctx}" | ||
} | ||
|
||
read_namespace() { | ||
local f | ||
f="$(namespace_file "${1}")" | ||
[[ -f "${f}" ]] && cat "${f}" | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it now when I executed it myself with DEBUG=1 ./kubens
. Thanks so much for noticing this.
I guess this works, because in both places read_namespace
is called, empty output will do the job.
@jvassev Thank you for your patch and debugging the subtle issue with the return value. If you're interested in development of the project, please hit "Watch" so you can participate in issue discussions and feature proposals. |
@jvassev Now that v0.5.0 is released a couple of days ago, I'd like to thank you again. Your fix highlighted what was a security vulnerability in kubens. Summarykubens was susceptible to an attack where a crafted context with a name like This is because kubens uses the name of the current-context to save the previous namespace (for ExploitabilityUnless the user picks up an untrusted kubeconfig file and starts using that, OR they let a program/user to change their existing kubeconfig (through Normally, Kubernetes validates the format for namespaces. However an attacker can also bypass this by setting an arbitrary "namespace" field on the context (in kubeconfig file, or through PoC Exploit
|
Also, fix
kubens -
which fails to save previous namespaceafter 6610d70