Allow almost all printable ASCII characters in environment variables

[HirazawaUi]

• One-line PR description: Allow special characters in environment variables

• Issue link: Allow almost all printable ASCII characters in environment variables #4369

• Other comments:

[bart0sh]

/cc

[sftim]

Is this relevant to SIG Security?

[HirazawaUi]

Based on the description, I assume this has nothing to do with SIG Security?https://github.com/kubernetes/community/tree/master/sig-security#security-special-interest-group

[thockin]

Could be good to get a consultation - security holes are rarely where reasonable people expect them to be. :)

@IanColdwater @tabbysable ?

[thockin]

ping @IanColdwater @tabbysable

[thockin]

Overall I am OK with this proposal. Caution is warranted, but reasonable caution, IMO.

@liggitt or @SergeyKanzhelev objections or further consultations you want?

[thockin]

Can you detail what the new validation will be? Is it "anything but '=' or is it something more precise?

[liggitt]

+1 to being explicit here. The summary says Allow all printable ASCII except '='. Is that well-defined?

[HirazawaUi]

The summary says Allow all printable ASCII except '='. Is that well-defined?

I wrote a demo to verify the ASCII characters that can be set as environment variables

package main

import (
	"fmt"
	"os"
)

func setEnvVars() {
	for i := 0; i < 128; i++ {
		envKey := "a" + string(i)
		if err := os.Setenv(envKey, "abc"); err != nil {
			fmt.Println(i)
			fmt.Printf("set character %s to env name failed \n", envKey)
		}
	}
}

func main() {
	setEnvVars()
}

it results is

➜  ~ ./envvar
0
set character a to env name failed
61
set character a= to env name failed

It looks like all ASCII characters can be set as environment variables except for NUL with serial number 0 and = with serial number 61.... But I'd like to check if the CRI API restricts them first

[thockin]

OK, this is the main open issue, IMO.

[HirazawaUi]

According to the test results in #4373 (comment), I think we can constrain the characters to characters 33-126 in ASCII. I will add some regex to KEP, and elaborate it in more detail.

[thockin]

Could be good to get a consultation - security holes are rarely where reasonable people expect them to be. :)

@IanColdwater @tabbysable ?

[thockin]

@SergeyKanzhelev or someone else from sig-node may want to look at it.

[liggitt]

Feature gates aren't intended to be a long-term configuration option. How is the risk managed as the feature gate graduates to GA and is enabled by default without user intervention, or is locked on and can no longer be disabled?

[thockin]

I presume it's like any other - locked once GA. Any reason not to follow this?

[HirazawaUi]

My opinion is the same as @thockin, it should be the same as other feature gates, if we think it has risks when upgrade and rollback scenarios, we can let the beta time duration be long enough

[liggitt]

+1 to being explicit here. The summary says Allow all printable ASCII except '='. Is that well-defined?

[liggitt]

add details about what this will exercise and that it checks all the way into the container (e.g. from kubernetes/kubernetes#53201 (comment))

e2e tests demonstrating we can actually construct an environment with envvar names at the edges of what we support and it makes it into the container successfully (envvar names containing newlines/whitespace, unprintable characters, multi-byte characters, "difficult" characters like '"*?;/\()&$, etc)

[liggitt]

are there any CRI-specific (containerd vs crio vs docker vs ...) or OS-specific (linux or windows) oddities about support for envvars with weird characters in the names? do CRI implementations have to make adjustments to support this? does the CRI API specify allowed envvar names or is it arbitrary / opaque

[HirazawaUi]

do CRI implementations have to make adjustments to support this? does the CRI API specify allowed envvar names or is it arbitrary / opaque

Let me do some research...

[HirazawaUi]

I wrote a demo to test envvar name support for different container runtimes that allow all printable ASCII characters as envvar names, even "=" is allowed, They have more relaxed restrictions on envvar than k8s.
ref: https://github.com/HirazawaUi/verfiy-container-env

[sftim]

Not sure these are right for a provisional KEP.

[HirazawaUi]

I will modify it if it is not implemented in 1.30.

[BenTheElder]

Provisional KEPs are not supposed to have code merging, versus implementable
The idea being, provisional KEPs may have part of an idea merged to iterate on but aren't agreed to implement yet. An implementable KEP would be expected to have stronger approval and would have a target release for implementing. https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/0000-kep-process/README.md#kep-workflow

[HirazawaUi]

Ping when updated, please

@thockin Sorry for the delay, has already been updated, my initial thought was to update after all the comments have been processed over the weekend

[thockin]

/lgtm
/approve

[thockin]

Maybe describe this (or implement it) as unicode.IsPrint(r) && r < unicode.MaxASCII

[HirazawaUi]

We also need to filter out "=" from all printable ASCII characters, it will be easier to use regex.

[thockin]

The reason I prefer unicode.IsPrint(r) && r < unicode.MaxASCII to a range or regex is that it's not reasonable to expect people to keep the ASCII range in their heads.

[HirazawaUi]

all right, I agree with your opinion.

[thockin]

ping @IanColdwater @tabbysable

[HirazawaUi]

@thockin I updated the documentation to weaken "printable ASCII characters" and emphasize using the range of decimal numbers in ASCII to describe it, may you take a look at it again?

[HirazawaUi]

Before, I mistakenly thought that the space character was not a printable ASCII character, but it turned out that I was wrong.
ref: https://en.wikipedia.org/wiki/ASCII#Printable_characters
And space character can also be set as an envvar name, so I re-adjusted the ASCII character range to 32-126.

[thockin]

I'm still LGTM to proceed on this.

/lgtm

[HirazawaUi]

Since it's close to the enhancement freeze, I'll contact the maintainers of sig node & sig security on slack to review it (if they're interested).

[HirazawaUi]

@liggitt Do you have any other thoughts on it?

[thockin]

This PR also needs to add a file in keps/prod-readiness to trigger PRR.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, mrunalp, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/sig-node/OWNERS [mrunalp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thockin]

@HirazawaUi You still need to trigger PRR or this will not move forward

[HirazawaUi]

@HirazawaUi You still need to trigger PRR or this will not move forward

all right.

[BenTheElder]

not N/A when going beta, GA. ok for alpha

[BenTheElder]

Provisional KEPs are not supposed to have code merging, versus implementable
The idea being, provisional KEPs may have part of an idea merged to iterate on but aren't agreed to implement yet. An implementable KEP would be expected to have stronger approval and would have a target release for implementing. https://github.com/kubernetes/enhancements/blob/master/keps/sig-architecture/0000-kep-process/README.md#kep-workflow