review suggestions#3
Closed
thaJeztah wants to merge 1 commit intoakerouanton:endpoint-specific-mac-addressfrom
Closed
review suggestions#3thaJeztah wants to merge 1 commit intoakerouanton:endpoint-specific-mac-addressfrom
thaJeztah wants to merge 1 commit intoakerouanton:endpoint-specific-mac-addressfrom
Conversation
thaJeztah
commented
Jul 29, 2023
- related to Add missing opts to --network advanced syntax docker/cli#4419
- make sure we don't reset the macAddress on older API versions
- on API >= 1.44 return a warning if the top-level macAddress was set
- on API < 1.44, restore the top-level macAddress from the default network (networkMode)
- add a client-error when trying to use the top-level macAddress on API >= 1.44
- added a TODO for migrating in the client (?)
- probably other changes may still be needed ':-)
- make sure we don't reset the macAddress on older API versions - on API >= 1.44 return a warning if the top-level macAddress was set - on API < 1.44, restore the top-level macAddress from the default network (networkMode) - add a client-error when trying to use the top-level macAddress on API >= 1.44 - added a TODO for migrating in the client (?) - probably other changes may still be needed ':-) Signed-off-by: Sebastiaan van Stijn <[email protected]>
This was referenced Jul 29, 2023
Author
|
akerouanton
reviewed
Aug 4, 2023
| nwName := hostConfig.NetworkMode.NetworkName() | ||
| if _, ok := networkingConfig.EndpointsConfig[nwName]; !ok { | ||
| networkingConfig.EndpointsConfig[nwName] = &network.EndpointSettings{} | ||
| if endPointConfig, ok := networkingConfig.EndpointsConfig[nwName]; !ok { |
Owner
There was a problem hiding this comment.
Few things I'm gonna fix:
networkingConfigcould be nil ;- We should invariably set the endpoint's
MacAddress, as it'll be either the container-wide MacAddress, or an empty string.
akerouanton
reviewed
Aug 4, 2023
| if err := cli.NewVersionError("1.44", "specify mac-address per network"); config != nil && config.MacAddress != "" && err != nil { | ||
| return response, err | ||
| } | ||
| // TODO{thaJeztah): should we convert --mac-address to NetworkingConfig[<container network "mode">].MacAddress on API < 1.44 ?? |
Owner
There was a problem hiding this comment.
I think we shouldn't, otherwise we'd break compatibility with older daemons.
Owner
|
I cherry-picked your suggestions into my branch, so let me close this PR. |
akerouanton
pushed a commit
that referenced
this pull request
Mar 28, 2024
…f v1.5.4 full diffs: - protocolbuffers/protobuf-go@v1.31.0...v1.33.0 - golang/protobuf@v1.5.3...v1.5.4 From the Go security announcement list; > Version v1.33.0 of the google.golang.org/protobuf module fixes a bug in > the google.golang.org/protobuf/encoding/protojson package which could cause > the Unmarshal function to enter an infinite loop when handling some invalid > inputs. > > This condition could only occur when unmarshaling into a message which contains > a google.protobuf.Any value, or when the UnmarshalOptions.UnmarshalUnknown > option is set. Unmarshal now correctly returns an error when handling these > inputs. > > This is CVE-2024-24786. In a follow-up post; > A small correction: This vulnerability applies when the UnmarshalOptions.DiscardUnknown > option is set (as well as when unmarshaling into any message which contains a > google.protobuf.Any). There is no UnmarshalUnknown option. > > In addition, version 1.33.0 of google.golang.org/protobuf inadvertently > introduced an incompatibility with the older github.com/golang/protobuf > module. (golang/protobuf#1596) Users of the older > module should update to github.com/golang/[email protected]. govulncheck results in our code: govulncheck ./... Scanning your code and 1221 packages across 204 dependent modules for known vulnerabilities... === Symbol Results === Vulnerability #1: GO-2024-2611 Infinite loop in JSON unmarshaling in google.golang.org/protobuf More info: https://pkg.go.dev/vuln/GO-2024-2611 Module: google.golang.org/protobuf Found in: google.golang.org/[email protected] Fixed in: google.golang.org/[email protected] Example traces found: #1: daemon/logger/gcplogs/gcplogging.go:154:18: gcplogs.New calls logging.Client.Ping, which eventually calls json.Decoder.Peek #2: daemon/logger/gcplogs/gcplogging.go:154:18: gcplogs.New calls logging.Client.Ping, which eventually calls json.Decoder.Read #3: daemon/logger/gcplogs/gcplogging.go:154:18: gcplogs.New calls logging.Client.Ping, which eventually calls protojson.Unmarshal Your code is affected by 1 vulnerability from 1 module. This scan found no other vulnerabilities in packages you import or modules you require. Signed-off-by: Sebastiaan van Stijn <[email protected]>
akerouanton
added a commit
that referenced
this pull request
Oct 22, 2024
When a port mapping is created with a HostIP specified, we insert a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. Then, in filter chains, we allow access to the container port for any packet not coming from the container's network itself (if hairpinning is disabled), nor from another host bridge. (see below) However we don't set any rule that prevent a rogue neighbor located in another L2 segment / subnet from sending packets destined to that HostIP. For instance, if a port binding is created with HostIP == '127.0.0.1', this port should not be accessible from anything but the lo interface. That's currently not the case and this provides a false sense of security. Since nat-DOCKER mangles the dest addr, and the nat table rejects DROP rules, this change adds rules into raw-PREROUTING to filter ingress packets destined to mapped ports based on the input interface, the dest addr and the dest port. Interfaces are resolved by making a fib lookup of the HostIP (if it's not unspecified). In most cases, this should be fine - even in HA scenarios where multiple links might be aggregated, the HostIP would match the address assigned to a bond / bridge interface. However, the env var `DOCKER_DISABLE_FILTER_BY_INPUT_IFACE` can be set to skip this input filtering rule. ``` $ iptables-tracer -skip-modprobe -filter 'udp port 5000' INFO[0000] Waiting for trace events... IN=eth-test OUT= SRC=192.168.123.3 DST=172.17.0.2 LEN=35 TOS=00 TTL=64 ID=63941 PROTO=UDP SPT=48601 DPT=5000 CSUM=e7df nat DOCKER NFMARK=0x0 MATCH RULE (#3): -d 172.17.0.2/32 ! -i br-fb571312de21 -p udp -m udp --dport 5000 -j DNAT --to-destination 172.19.0.2:5000 => DNAT: --to-destination 172.19.0.2:5000 filter DOCKER NFMARK=0x0 MATCH RULE (#1): -d 172.19.0.2/32 ! -i br-fb571312de21 -o br-fb571312de21 -p udp -m udp --dport 5000 -j ACCEPT => ACCEPT ``` Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
added a commit
that referenced
this pull request
Oct 22, 2024
When a port mapping is created with a HostIP specified, we insert a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. Then, in filter chains, we allow access to the container port for any packet not coming from the container's network itself (if hairpinning is disabled), nor from another host bridge. (see below) However we don't set any rule that prevent a rogue neighbor located in another L2 segment / subnet from sending packets destined to that HostIP. For instance, if a port binding is created with HostIP == '127.0.0.1', this port should not be accessible from anything but the lo interface. That's currently not the case and this provides a false sense of security. Since nat-DOCKER mangles the dest addr, and the nat table rejects DROP rules, this change adds rules into raw-PREROUTING to filter ingress packets destined to mapped ports based on the input interface, the dest addr and the dest port. Interfaces are resolved by making a fib lookup of the HostIP (if it's not unspecified). In most cases, this should be fine - even in HA scenarios where multiple links might be aggregated, the HostIP would match the address assigned to a bond / bridge interface. However, the env var `DOCKER_DISABLE_FILTER_BY_INPUT_IFACE` can be set to skip this input filtering rule. ``` $ iptables-tracer -skip-modprobe -filter 'udp port 5000' INFO[0000] Waiting for trace events... IN=eth-test OUT= SRC=192.168.123.3 DST=172.17.0.2 LEN=35 TOS=00 TTL=64 ID=63941 PROTO=UDP SPT=48601 DPT=5000 CSUM=e7df nat DOCKER NFMARK=0x0 MATCH RULE (#3): -d 172.17.0.2/32 ! -i br-fb571312de21 -p udp -m udp --dport 5000 -j DNAT --to-destination 172.19.0.2:5000 => DNAT: --to-destination 172.19.0.2:5000 filter DOCKER NFMARK=0x0 MATCH RULE (#1): -d 172.19.0.2/32 ! -i br-fb571312de21 -o br-fb571312de21 -p udp -m udp --dport 5000 -j ACCEPT => ACCEPT ``` Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
added a commit
that referenced
this pull request
Oct 22, 2024
When a port mapping is created with a HostIP specified, we insert a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. Then, in filter chains, we allow access to the container port for any packet not coming from the container's network itself (if hairpinning is disabled), nor from another host bridge. (see below) However we don't set any rule that prevent a rogue neighbor located in another L2 segment / subnet from sending packets destined to that HostIP. For instance, if a port binding is created with HostIP == '127.0.0.1', this port should not be accessible from anything but the lo interface. That's currently not the case and this provides a false sense of security. Since nat-DOCKER mangles the dest addr, and the nat table rejects DROP rules, this change adds rules into raw-PREROUTING to filter ingress packets destined to mapped ports based on the input interface, the dest addr and the dest port. Interfaces are resolved by making a fib lookup of the HostIP (if it's not unspecified). In most cases, this should be fine - even in HA scenarios where multiple links might be aggregated, the HostIP would match the address assigned to a bond / bridge interface. However, the env var `DOCKER_DISABLE_FILTER_BY_INPUT_IFACE` can be set to skip this input filtering rule. ``` $ iptables-tracer -skip-modprobe -filter 'udp port 5000' INFO[0000] Waiting for trace events... IN=eth-test OUT= SRC=192.168.123.3 DST=172.17.0.2 LEN=35 TOS=00 TTL=64 ID=63941 PROTO=UDP SPT=48601 DPT=5000 CSUM=e7df nat DOCKER NFMARK=0x0 MATCH RULE (#3): -d 172.17.0.2/32 ! -i br-fb571312de21 -p udp -m udp --dport 5000 -j DNAT --to-destination 172.19.0.2:5000 => DNAT: --to-destination 172.19.0.2:5000 filter DOCKER NFMARK=0x0 MATCH RULE (#1): -d 172.19.0.2/32 ! -i br-fb571312de21 -o br-fb571312de21 -p udp -m udp --dport 5000 -j ACCEPT => ACCEPT ``` Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
added a commit
that referenced
this pull request
Oct 22, 2024
When a port mapping is created with a HostIP specified, we insert a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. Then, in filter chains, we allow access to the container port for any packet not coming from the container's network itself (if hairpinning is disabled), nor from another host bridge. (see below) However we don't set any rule that prevent a rogue neighbor located in another L2 segment / subnet from sending packets destined to that HostIP. For instance, if a port binding is created with HostIP == '127.0.0.1', this port should not be accessible from anything but the lo interface. That's currently not the case and this provides a false sense of security. Since nat-DOCKER mangles the dest addr, and the nat table rejects DROP rules, this change adds rules into raw-PREROUTING to filter ingress packets destined to mapped ports based on the input interface, the dest addr and the dest port. Interfaces are resolved by making a fib lookup of the HostIP (if it's not unspecified). In most cases, this should be fine - even in HA scenarios where multiple links might be aggregated, the HostIP would match the address assigned to a bond / bridge interface. However, the env var `DOCKER_DISABLE_FILTER_BY_INPUT_IFACE` can be set to skip this input filtering rule. ``` $ iptables-tracer -skip-modprobe -filter 'udp port 5000' INFO[0000] Waiting for trace events... IN=eth-test OUT= SRC=192.168.123.3 DST=172.17.0.2 LEN=35 TOS=00 TTL=64 ID=63941 PROTO=UDP SPT=48601 DPT=5000 CSUM=e7df nat DOCKER NFMARK=0x0 MATCH RULE (#3): -d 172.17.0.2/32 ! -i br-fb571312de21 -p udp -m udp --dport 5000 -j DNAT --to-destination 172.19.0.2:5000 => DNAT: --to-destination 172.19.0.2:5000 filter DOCKER NFMARK=0x0 MATCH RULE (#1): -d 172.19.0.2/32 ! -i br-fb571312de21 -o br-fb571312de21 -p udp -m udp --dport 5000 -j ACCEPT => ACCEPT ``` Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
added a commit
that referenced
this pull request
Oct 22, 2024
When a port mapping is created with a HostIP specified, we insert a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. Then, in filter chains, we allow access to the container port for any packet not coming from the container's network itself (if hairpinning is disabled), nor from another host bridge. (see below) However we don't set any rule that prevent a rogue neighbor located in another L2 segment / subnet from sending packets destined to that HostIP. For instance, if a port binding is created with HostIP == '127.0.0.1', this port should not be accessible from anything but the lo interface. That's currently not the case and this provides a false sense of security. Since nat-DOCKER mangles the dest addr, and the nat table rejects DROP rules, this change adds rules into raw-PREROUTING to filter ingress packets destined to mapped ports based on the input interface, the dest addr and the dest port. Interfaces are resolved by making a fib lookup of the HostIP (if it's not unspecified). In most cases, this should be fine - even in HA scenarios where multiple links might be aggregated, the HostIP would match the address assigned to a bond / bridge interface. However, the env var `DOCKER_DISABLE_FILTER_BY_INPUT_IFACE` can be set to skip this input filtering rule. ``` $ iptables-tracer -skip-modprobe -filter 'udp port 5000' INFO[0000] Waiting for trace events... IN=eth-test OUT= SRC=192.168.123.3 DST=172.17.0.2 LEN=35 TOS=00 TTL=64 ID=63941 PROTO=UDP SPT=48601 DPT=5000 CSUM=e7df nat DOCKER NFMARK=0x0 MATCH RULE (#3): -d 172.17.0.2/32 ! -i br-fb571312de21 -p udp -m udp --dport 5000 -j DNAT --to-destination 172.19.0.2:5000 => DNAT: --to-destination 172.19.0.2:5000 filter DOCKER NFMARK=0x0 MATCH RULE (#1): -d 172.19.0.2/32 ! -i br-fb571312de21 -o br-fb571312de21 -p udp -m udp --dport 5000 -j ACCEPT => ACCEPT ``` Signed-off-by: Albin Kerouanton <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.