OCPBUGS-20410: CRI-O: Use 127.0.0.1 for stream server with random port

[saschagrunert]

This should work since it's the default in CRI-O with the merge of cri-o/cri-o#1714. It slightly increases security of the default configuration.

- Description for the changelog

Updated CRI-O streaming server configuration to use 127.0.0.1 and a random port.

[saschagrunert]

cc @mrunalp @haircommander @rphillips @umohnani8 @harche @QiWang19

What are your thoughts on that change?

[haircommander]

makes sense to me!

/lgtm

[QiWang19]

assign @sinnykumari for approval

[rphillips]

Had a chat with Mrunal. This will break IPv6 only systems. Instead of defaulting to "" (which in golang is all interfaces), we can listen on 127.0.0.1 and ::1.

[saschagrunert]

The golang server is already listening on IPv4 or IPv6 per: https://github.com/cri-o/cri-o/blob/5951d4935f00bd1501dc510b68da1aa631c5ed72/vendor/k8s.io/kubelet/pkg/cri/streaming/server.go#L240

Can we just use localhost here?

[saschagrunert]

/retest

[saschagrunert]

/test okd-scos-e2e-aws-ovn

[saschagrunert]

/test okd-scos-e2e-aws-ovn

[saschagrunert]

/test okd-scos-e2e-aws-ovn

[saschagrunert]

/test okd-scos-e2e-aws-ovn

[saschagrunert]

/retest

[saschagrunert]

/retest

[kwilczynski]

@saschagrunert, do you reckon

The golang server is already listening on IPv4 or IPv6 per: https://github.com/cri-o/cri-o/blob/5951d4935f00bd1501dc510b68da1aa631c5ed72/vendor/k8s.io/kubelet/pkg/cri/streaming/server.go#L240

Can we just use localhost here?

Using (as an example) ":8080" or "0.0.0.0:8080" (to denote any interface) would result in a Go listener binding to all available interfaces - let it be IPv4 or IPv6. However, if we make it "localhost:8080" or "127.0.0.1:8080", then only IPv4 will be used.

Why do we want to listen on IPv6 on localhost? I don't know how practical this would be. I think leaving it to be just "localhost" or "127.0.0.1" (which is probably better than "localhost" if we want to keep it to IPv4 only) would be fine.

[saschagrunert]

Why do we want to listen on IPv6 on localhost? I don't know how practical this would be. I think leaving it to be just "localhost" or "127.0.0.1" (which is probably better than "localhost" if we want to keep it to IPv4 only) would be fine.

We have to stay compatible with IPv6-only systems. That's why 127.0.0.1 would not work. The listener in golang would then automatically pick IPv6 if we only specify "localhost".

[kwilczynski]

Why do we want to listen on IPv6 on localhost? I don't know how practical this would be. I think leaving it to be just "localhost" or "127.0.0.1" (which is probably better than "localhost" if we want to keep it to IPv4 only) would be fine.

We have to stay compatible with IPv6-only systems. That's why 127.0.0.1 would not work. The listener in golang would then automatically pick IPv6 if we only specify "localhost".

You mean it wouldn't? The only cases where IPv6 will be used would be ":1234" or "0.0.0.0:1234" (here to denote any explicitly). See a test below. I wish there were an option to enable dual-stack in Go's listener or use a custom listener type to provide configuration (similarly to what one can do when creating a custom client), but that's not the case.

Some systems hardcode the notion of "localhost", but this goes through a normal resolver on most, so it might resolve to something other than 127.0.0.1, albeit unlikely. There is also the "ipv6-localhost" that some systems also offer via the /etc/hosts entry, and that resolves to ::1, but it often does not work - and it would not help here anyway. This is why I suggested 127.0.0.1 for loopback, as it does not depend on any resolvers.

Using localhost or 127.0.0.1 does not disable IPv6 support, so we are still compatible with it and support it - as long as the OS supports it (kernel supports it and loopback has ::1 configured). If we ought to be locked down to a local-only listener and connection for security, then it does not matter that we aren't listening on IPv6, does it?

Am I missing something?

Otherwise, if we insist on this, then if we detect "localhost" or "127.0.0.1" as a value, then we need to have another listener to explicitly listen on ::1, which seems like overkill - more error checking, more code, more tests are needed, etc.

---

Test:

$ ./test ":1234" 1
tcp6       0      0 :::1234                 :::*                    LISTEN      78318/test          

$ nc -n 0 1234 -v -z -w 1
Connection to 0 1234 port [tcp/*] succeeded!
$ nc -n -6 ::1 1234 -v -z -w 1
Connection to ::1 1234 port [tcp/*] succeeded!

$ ./test "localhost:1234" 1
tcp        0      0 127.0.0.1:1234          0.0.0.0:*               LISTEN      78375/test          

$ nc -n 0 1234 -v -z -w 1
Connection to 0 1234 port [tcp/*] succeeded!
$ nc -n -6 ::1 1234 -v -z -w 1
nc: connect to ::1 port 1234 (tcp) failed: Connection refused

Using:

package main

import (
	"fmt"
	"net"
	"os"
	"os/exec"
	"strings"
)

func main() {
	go func() {
		_, err := net.Listen("tcp", os.Args[1])
		if err != nil {
			panic(err)
		}
	}()
	s, err := exec.Command("netstat", "-ntlp").Output()
	if err != nil {
		panic(err)
	}
	for _, l := range strings.Split(string(s), "\n") {
		if strings.Contains(l, fmt.Sprint(os.Getpid())) {
			fmt.Println(l)
		}
	}
	if len(os.Args) > 2 && os.Args[2] != "" {
		select {}
	}
}

[saschagrunert]

@kwilczynski im not actually sure how to test that, but how does it behave on systems supporting only IPv6, no IPv4?

The listen on tcp uses either tcp4 or tcp6, right?

Would 127.0.0.1 still work in that case?

[kwilczynski]

@kwilczynski im not actually sure how to test that, but how does it behave on systems supporting only IPv6, no IPv4?

We could reason about two things: disabling OS support and not configuring IPv4.

The former, for Linux, is challenging, if not impossible [1][2]. The *BSD (FreeBSD) is the only other OS I know that can disable IPv4 entirely if you wish (very purist of them, isn't it? 😄 ). I can't speak for Windows. For Linux, the CONFIG_NET enables loopback and dummy drivers - this is usually coupled with CONFIG_INET, which enables IPv4 shebang. There is no CONFIG_INET4 or CONFIG_INET6 that can be toggled depending on which stack we would want. Things would also break without some resemblance of IPv4 support - for example, many things use simple TCP (with IPv4) IPC over loopback.

The latter, you can choose not to configure IPv4 on loopback (most likely things would break) or on other interfaces (more legitimate case). This would work. To add, there is no sysctl toggle to disable IPv4 support as what is available for IPv6 currently.

1. https://lkml.org/lkml/2019/4/25/257
2. https://lkml.org/lkml/2019/4/25/751

For reference:

1. net: Listen("tcp", "0.0.0.0:1234") binds to IPv6 addresses golang/go#48723
2. proposal: net/v2: Listen is unfriendly to multiple address families, endpoints and subflows golang/go#9334
3. net: Dial is not safe to use in namespaces with dual-stack enabled golang/go#44922

The listen on tcp uses either tcp4 or tcp6, right?

Correct, but which addresses the listener will bind in due process is limited to whether the address portion is empty or set to any or the 0.0.0.0 (like the examples from prior comments). For anything else, depending on the provided address portion (not the port) and what it is or what it resolves to, the listener would bind to specific IPv4 or IPv6 addresses only.

Would 127.0.0.1 still work in that case?

The 127.0.0.1 should probably always work on Linux. Otherwise, there will be issues, I suppose.

I think a possible solution would be to make the stream_address attribute so that it can be repeated multiple times or take a list of addresses. This approach is not uncommon. For example:

Using a list in TOML:

[[crio.api]]
stream_address = "127.0.0.1"
stream_port = "1234"

[[crio.api]]
stream_address = "::1"
stream_port = "0"

Using the stream_address attribute with a list of addresses (an array):

[crio.api]
stream_address = ["127.0.0.1", "::1"]
stream_port = "0"

This approach also allows people to enable a streaming endpoint to listen on multiple addresses (or interfaces, if you wish), such as the localhost (loopback) or perhaps some internal network address (which might be some dedicated management VLAN, etc.).

[saschagrunert]

Would 127.0.0.1 still work in that case?

The 127.0.0.1 should probably always work on Linux. Otherwise, there will be issues, I suppose.

Thanks, excellent explanation by the way!

Using a list in TOML:

[[crio.api]]
stream_address = "127.0.0.1"
stream_port = "1234"

[[crio.api]]
stream_address = "::1"
stream_port = "0"

This would be cool, but a breaking change. We would have to spawn multiple streaming servers because the current one does not support that. Or we modify the existing implementation to support multiple endpoints. I don't know if that's really worth the effort. The CRI-O config would have to go through a deprecation policy as well.

[saschagrunert]

/test okd-scos-e2e-aws-ovn

[rphillips]

/lgtm

[rphillips]

/approve

[rphillips]

/assign @sinnykumari

[sinnykumari]

I don't have much domain specific context here, will go with already review done by the node team. Also required ci tests are passing.
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, rphillips, saschagrunert, sinnykumari

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:

• OWNERS [sinnykumari]

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

[kwilczynski]

/cherry-pick release-4.14

[openshift-cherrypick-robot]

@kwilczynski: new pull request created: #3984

In response to this:

/cherry-pick release-4.14

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/test-infra repository.

[kwilczynski]

/cherry-pick release-4.15

[openshift-cherrypick-robot]

@kwilczynski: new pull request could not be created: failed to create pull request against openshift/machine-config-operator#release-4.15 from head openshift-cherrypick-robot:cherry-pick-3853-to-release-4.15: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between openshift:release-4.15 and openshift-cherrypick-robot:cherry-pick-3853-to-release-4.15"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request"}

In response to this:

/cherry-pick release-4.15

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/test-infra repository.

[kwilczynski]

/cherry-pick release-4.16

[openshift-cherrypick-robot]

@kwilczynski: new pull request could not be created: failed to create pull request against openshift/machine-config-operator#release-4.16 from head openshift-cherrypick-robot:cherry-pick-3853-to-release-4.16: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between openshift:release-4.16 and openshift-cherrypick-robot:cherry-pick-3853-to-release-4.16"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request"}

In response to this:

/cherry-pick release-4.16

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/test-infra repository.

[kwilczynski]

OK. Change already on release-4.15 and release-4.16 branches.

[openshift-ci-robot]

@saschagrunert: Jira Issue OCPBUGS-20410: Some pull requests linked via external trackers have merged:

• openshift/machine-config-operator#3853

The following pull requests linked via external trackers have not merged:

• openshift/machine-config-operator#3984 is open

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-20410 has not been moved to the MODIFIED state.

In response to this:

This should work since it's the default in CRI-O with the merge of cri-o/cri-o#1714. It slightly increases security of the default configuration.

- Description for the changelog

Updated CRI-O streaming server configuration to use 127.0.0.1 and a random port.

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/test-infra repository.

[wking]

/jira refresh

[openshift-ci-robot]

@wking: Jira Issue OCPBUGS-20410: All pull requests linked via external trackers have merged:

• openshift/machine-config-operator#3853

Jira Issue OCPBUGS-20410 has been moved to the MODIFIED state.

In response to this:

/jira refresh

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/test-infra repository.