Container net host remove #15176

ycoheNvidia · 2023-05-21T10:53:52Z

submodule change to support snmp container hardening: sonic-net/sonic-snmpagent#281

Why I did it

Net host removal for snmp and database as part of container hardening

How I did it

How to verify it

Modified snmp related templates to support net host removal:

SNMP Docker listening-address:port forwarding
snmpd.conf listening address to include all addresses - since docker port forwarding is the actual routing table - and blocks all other addresses.
$NET modified to bridge for snmp and database container
snmp docker stop to rm container - since port forwarding can only happen on newly created image

Description for the changelog

snmp and database host net removal

A picture of a cute animal (not mandatory but encouraged)

   .
  ":"
___:____     |"\/"|

,' `. \ /
| O ___/ |
^^^^^^^^^^^^~

qiluo-msft · 2023-06-27T20:53:05Z

dockers/docker-database/Dockerfile.j2

@@ -32,6 +32,8 @@ RUN apt-get clean -y                                  && \
             s/^# unixsocket/unixsocket/;                \
             s/redis-server.sock/redis.sock/g;           \
             s/^client-output-buffer-limit pubsub [0-9]+mb [0-9]+mb [0-9]+/client-output-buffer-limit pubsub 0 0 0/; \
+             s/^bind 127.0.0.1 ::1$/# bind 127.0.0.1 ::1/;       \
+             s/^protected-mode yes/protected-mode no/;    \


no

This is not safe, right? #Closed

Redis protected mode only prevents the Redis server from responding to queries outside the loopback interfaces.
Docker network mode was changed from host to bridged. This requires to be able to connect from host IP to Redis docker IP.

qiluo-msft · 2023-06-27T20:56:45Z

dockers/docker-snmp/snmpd.conf.j2

 #
-{% if SNMP_AGENT_ADDRESS_CONFIG %}
-{% for (agentip, port, vrf) in SNMP_AGENT_ADDRESS_CONFIG %}


SNMP_AGENT_ADDRESS_CONFIG

This feature is still useful, and there are some other PRs trying to extend it. #Closed

I am sorry, I wasn't aware of the other PR's for this feature.
The logic of address/port/vrf was moved to the docker create port and address forwarding. Since only user defined ports and addresses will be forwarded, this logic will no longer be relevant here

I have updated the code to include the latest merged PR

Could you explain "The logic of address/port/vrf was moved to the docker create port and address forwarding"?

In the current implementation, snmpd has its configuration in snmpd.conf which defines which addresses and ports can be used for query, how and if vrf is being used, etc.
Removing snmp container from host network means that it is not exposed to any of these addresses by default,
What we did here was to use docker address:port forwarding as the method to implement the logic for it. When a user configures address and port to be used for snmp queries - this address:port tuple will be used when snmp docker is created, similar to a firewall. In what we offer here, the snmp demon inside the container will listen to all packets, and the docker networking logic (address:port forwarding) will forward only the relevant packets, as configured by user

qiluo-msft · 2023-06-27T21:00:23Z

files/build_templates/docker_image_ctl.j2

-        NET="host"
+	{%- if docker_container_name == "database" %}
+        NET="bridge"
+	PORT_MAP="-p 127.0.0.1:6379:6379"


6379

multi-DB usage may require more ports. #Closed

If you mean multi-asic case. Its working. Each asic db docker has its own IP with mapped 6379 listening port and shared redis.sock socket file.
If there are more use cases, that may require different ports please elaborate.

You may need to fetch the ports for all redis instances from conf files: "/var/run/redis/sonic-db/database_config.json".

With multi-DB feature, there will be multiple-namespace, and multiple instance per namespace. ref: https://github.com/sonic-net/SONiC/blob/master/doc/database/multi_namespace_db_instances.md

still applicable.

Thanks, checking

still applicable

qiluo-msft · 2023-06-27T22:18:18Z

dockers/docker-database/Dockerfile.j2

@@ -32,6 +32,8 @@ RUN apt-get clean -y                                  && \
             s/^# unixsocket/unixsocket/;                \
             s/redis-server.sock/redis.sock/g;           \
             s/^client-output-buffer-limit pubsub [0-9]+mb [0-9]+mb [0-9]+/client-output-buffer-limit pubsub 0 0 0/; \
+             s/^bind 127.0.0.1 ::1$/# bind 127.0.0.1 ::1/;       \


bind 127.0.0.1 ::1

It will bind to all the interfaces visibile to database container. To be super cautious, could you add a sonic-mgmt testcase to ensure redis is not expose external port on sonic? #Closed

SuvarnaMeenakshi · 2023-06-28T23:32:09Z

files/build_templates/docker_image_ctl.j2

+    fi
+{%- if docker_container_name == "snmp" %}
+# get snmp listening address and port list from redis
+addr_port_values=$(python3 -c 'from swsscommon.swsscommon import ConfigDBConnector; cfg_db = ConfigDBConnector();  cfg_db.connect(wait_for_init=True, retry_on=True); [print(k[0] + "|" + k[1]) for k in cfg_db.get_keys("SNMP_AGENT_ADDRESS_CONFIG|*")]')


why is this logic moved here instead of doing in snmpd.conf.j2 file?
What is the value add we get?
This is done during docker create. If we are just restarting snmp server and there is a change in the SNMP_AGENT_ADDRESS_CONFIG in config_db, will the new change get picked up if we just restart running docker?

This was moved here because port and address forwarding can only be done during docker create. What it means that in order to harden this container's network - each address/port change for this service will require docker removal and creation

This PR will kill the feature of SNMP_AGENT_ADDRESS_CONFIG/port. Do you want to implement the same feature here or in another PR?

I am not sure I understand your question. This PR and sonic-net/sonic-snmpagent#281 moves the logic of snmp networking management from the container to the host. The host will forward the relevant packets only (SNMP_AGENT_ADDRESS_CONFIG for example) and the container will take all packets sent to port 161

maipbui · 2023-07-05T21:21:11Z

files/build_templates/docker_image_ctl.j2

@@ -523,6 +531,8 @@ start() {
        {%- if docker_container_name == "database" %}
        NET="bridge"
        DB_OPT=$DB_OPT" -v /var/run/redis$DEV:/var/run/redis:rw "
+	{%- elif docker_container_name == "snmp" %}
+        NET="bridge"


NET="bridge"

Checking best practice (https://docs.docker.com/network/network-tutorial-standalone/), default bridge network is not best choice for production. User-defined bridge networks is best choice for production. Can we use user-defined bridge network here?

I agree, defining a user-defined bridge is the best case solution for this item.
Problem is - this FR wasn't scoped to define this user-bridge and therefor remained in the general bridge domain.
If such a bridge will be defined - then we can add it to the SNMP container and redis container (and any future container) as well.

ycoheNvidia · 2023-08-01T12:55:11Z

@qiluo-msft we addressed the comments and I adjusted the code to the recent merged snmp code. Can we proceed please?

dockers/docker-snmp/snmpd.conf.j2

qiluo-msft · 2023-08-11T23:24:18Z

Please resolove conflict #Closed

… container_net_host_remove

SuvarnaMeenakshi · 2023-08-16T20:21:15Z

files/build_templates/docker_image_ctl.j2

+NAMESPACE_COUNT=$NUM_ASIC
+if [ -z $addr_port_values ]; then
+if [ -z $NAMESPACE_COUNT ] || [ $NAMESPACE_COUNT -lt 2 ]; then
+addr_port_values=$(python3 -c 'from swsscommon.swsscommon import ConfigDBConnector; cfg_db = ConfigDBConnector();  cfg_db.connect(wait_for_init=True, retry_on=True); [print(k[1].split("/")[0].lower() + "%" + k[0]) if len(k) == 2 and k[1].split('/')[0].lower().startswith("fe80") else print(k[1].split("/")[0].lower()) for k in cfg_db.get_keys("LOOPBACK_INTERFACE|*")+cfg_db.get_keys("MGMT_INTERFACE|*") if len(k) == 2]')


here currently only Loopback and Mgmt IP addresses are used.
There is SNMP_AGENT_ADDRESS_CONFIG table which can be configured with the IPs for snmpd to listen on.
This change does not include using that.

@SuvarnaMeenakshi you either missed line 556 which addresses the specific SNMP_AGENT_ADDRESS_CONFIG table, or I miss something. I followed the logic that was found in snmpd.conf.j2 and translated it here

This is throwing a syntax error.
python3 -c 'from swsscommon.swsscommon import ConfigDBConnector; cfg_db = ConfigDBConnector(); cfg_db.connect(wait_for_init=True, retry_on=True); [print(k[1].split("/")[0].lower() + "%" + k[0]) if len(k) == 2 and k[1].split('/')[0].lower().startswith("fe80") else print(k[1].split("/")[0].lower()) for k in cfg_db.get_keys("LOOPBACK_INTERFACE|")+cfg_db.get_keys("MGMT_INTERFACE|") if len(k) == 2]'

Why do we need the "fe80" check?
We can keep the fe80 condition in the snmpd.conf.j2 file?

yaqiangz · 2023-09-14T07:50:05Z

Hi @ycoheNvidia, I wonder whether syslog generated inside bridge mode container can be written to host machine and update to remote syslog server?

arlakshm · 2023-08-15T21:30:18Z

dockers/docker-database/supervisord.conf.j2

@@ -36,7 +36,7 @@ dependent_startup=true
 {%- else -%}
 {%- set LOOPBACK_IP = '' -%}
 {%- endif -%}
-command=/bin/bash -c "{ [[ -s /var/lib/{{ redis_inst }}/dump.rdb ]] || rm -f /var/lib/{{ redis_inst }}/dump.rdb; } && mkdir -p /var/lib/{{ redis_inst }} && exec /usr/bin/redis-server /etc/redis/redis.conf --bind {{ LOOPBACK_IP }} {{ redis_items['hostname'] }} --port {{ redis_items['port'] }} --unixsocket {{ redis_items['unix_socket_path'] }} --pidfile /var/run/redis/{{ redis_inst }}.pid --dir /var/lib/{{ redis_inst }}"
+command=/bin/bash -c "{ [[ -s /var/lib/{{ redis_inst }}/dump.rdb ]] || rm -f /var/lib/{{ redis_inst }}/dump.rdb; } && mkdir -p /var/lib/{{ redis_inst }} && exec /usr/bin/redis-server /etc/redis/redis.conf --port {{ redis_items['port'] }} --unixsocket {{ redis_items['unix_socket_path'] }} --pidfile /var/run/redis/{{ redis_inst }}.pid --dir /var/lib/{{ redis_inst }}"


In SONiC Chassis, the SWSS container on the linecard connect to local database as well as the database_chassis instance on supervisor. This info is got from database_global.json. Will this be possible after this change

snippet from database_global.json on linecard

INSTANCES": { "redis": { "hostname": "127.0.0.1", "port": 6379, "unix_socket_path": "/var/run/redis/redis.sock", "persistence_for_warm_boot": "yes" }, "redis_chassis": { "hostname": "redis_chassis.server", "port": 6380, "unix_socket_path": "/var/run/redis-chassis/redis_chassis.sock", "persistence_for_warm_boot": "yes" } },

qiluo-msft · 2023-09-18T06:39:18Z

files/build_templates/docker_image_ctl.j2

-        NET="host"
+	{%- if docker_container_name == "database" %}
+        NET="bridge"
+	PORT_MAP="-p 127.0.0.1:6379:6379"


mix tabs and spaces. #Closed

files/build_templates/docker_image_ctl.j2

qiluo-msft · 2023-09-18T07:03:36Z

files/build_templates/docker_image_ctl.j2

@@ -646,6 +705,9 @@ stop() {
    /usr/local/bin/container stop -t 60 $DOCKERNAME
    {%- else %}
    /usr/local/bin/container stop $DOCKERNAME
+    {%- if docker_container_name == "snmp" %}
+    docker rm $DOCKERNAME


docker rm

Why remove docker container?

Docker container behavior requires that port forwarding is done during "docker run", and cannot be changed afterwards. Using port forwarding as a method of support net host remove will require removing docker stop, as it is called as part of service restart (docker stop + docker run).
An example for flow:

snmp being configured -> snmp service established -> snmp docker created with "docker run" with relevant port forwarding arguments

snmp configuration changes -> snmp service restart -> snmp old docker deleted + calling "docker run" with new port forwarding values

If there is hot patches inside docker container, original behavior is that the patches will survive config reload or reboot. But this PR lose the capabilities.

Are there real use-cases where a docker is being loaded with patches inside it?
There might also be some local files (such as temp config and such in every container), is this a real use-case?

Yes, it is possible in production environment.

Still applicable.

yaqiangz · 2023-09-19T02:40:04Z

files/build_templates/docker_image_ctl.j2

+        NET="bridge"
+        PORT_MAP="-p 127.0.0.1:6379:6379"
+    {%- elif docker_container_name == "snmp" %}
+    NET="bridge"


I just tried image built by this PR yesterday, version: SONiC.master-15176.360116-6bb24c135. Looks like rsyslog inside snmp container cannot work well. I cannot find any syslog generated by snmp container (contains 'snmp#') in local host syslog file or our syslog server, and process containercfgd which is to dynamically generate rsyslog configuration file inside container doesn't startup. Not sure whether you have verified this, could you confirm that?
@Yarden-Z FYI

@ycoheNvidia could check if change like this is needed. #4738

dockers/docker-database/supervisord.conf.j2

… container_net_host_remove

files/build_templates/docker_image_ctl.j2

SuvarnaMeenakshi · 2023-10-10T18:04:40Z

@ycoheNvidia 2 issues that I observed when using this diff with latest master branch image:

redis-connection is not working from snmp_subagent even with the PR change in snmp submodule, I added a comment on that PR. SNMP hardening - redis socket connect sonic-snmpagent#281
SNMP walk is failing for single-asic and multi-asic device, I tried adding iptable NAT rule to modify destination address as eth0 IP address inside snmp docker and modify the SRC while sending out response to docker0 IP, with this I see that snmp query works fine on both single and multi-asic Wondering if you verified the snmp query with these changes?
sudo ip6tables -t nat -A PREROUTING -p udp -m udp --dport 161 -j DNAT --to-destination fd00::242:f07f:102 (eth0 IP of snmp docker )
sudo ip6tables -t nat -A POSTROUTING -p tcp -m tcp --dport 161 -j SNAT --to-source fd00::1 (docker0 Ipv6)
sudo iptables -t nat -A PREROUTING -p udp -m udp --dport 161 -j DNAT --to-destination 240.127.1.2 (eth0 IP of snmp docker)
sudo iptables -t nat -A POSTROUTING -p udp -m udp --dport 161 -j SNAT --to-source 240.127.1.1 (docker0 Ipv4)
We need to check if these rules have to added as postStartAction in /usr/bin/snmp.sh after starting docker or if they should be added in caclmgrd. Something similar is done for multi-asic.

ycoheNvidia and others added 2 commits May 21, 2023 13:45

SNMP container hardening

b9bf960

Remove net flag in redis docker

5ee5af7

ycoheNvidia requested review from qiluo-msft and lguohan as code owners May 21, 2023 10:53

ycoheNvidia mentioned this pull request May 21, 2023

SNMP hardening - redis socket connect sonic-net/sonic-snmpagent#281

Closed

Merge branch 'sonic-net:master' into container_net_host_remove

8e537a1

qiluo-msft reviewed Jun 27, 2023

View reviewed changes

qiluo-msft requested a review from SuvarnaMeenakshi June 27, 2023 20:55

qiluo-msft reviewed Jun 27, 2023

View reviewed changes

qiluo-msft requested review from judyjoseph and abdosi June 27, 2023 21:00

qiluo-msft reviewed Jun 27, 2023

View reviewed changes

SuvarnaMeenakshi reviewed Jun 28, 2023

View reviewed changes

maipbui mentioned this pull request Jul 5, 2023

SONiC Container Hardening sonic-net/SONiC#1364

Merged

maipbui reviewed Jul 5, 2023

View reviewed changes

Merge branch 'master' into container_net_host_remove

238a198

ycoheNvidia force-pushed the container_net_host_remove branch from 734584b to 238a198 Compare August 1, 2023 12:35

qiluo-msft reviewed Aug 3, 2023

View reviewed changes

dockers/docker-snmp/snmpd.conf.j2 Outdated Show resolved Hide resolved

maipbui mentioned this pull request Aug 11, 2023

[database] enable userns-remap and make redis process runs as non-root user #15972

Closed

11 tasks

Merge branch 'master' of github.com:ycoheNvidia/sonic-buildimage into…

5296455

… container_net_host_remove

ycoheNvidia force-pushed the container_net_host_remove branch from 94258fa to 5296455 Compare August 14, 2023 13:25

SuvarnaMeenakshi reviewed Aug 16, 2023

View reviewed changes

Removed one space in file header

621b54a

arlakshm reviewed Sep 14, 2023

View reviewed changes

qiluo-msft reviewed Sep 18, 2023

View reviewed changes

files/build_templates/docker_image_ctl.j2 Show resolved Hide resolved

qiluo-msft reviewed Sep 18, 2023

View reviewed changes

Removed tabs and irrelevant lines

9169746

yaqiangz reviewed Sep 19, 2023

View reviewed changes

qiluo-msft reviewed Sep 24, 2023

View reviewed changes

dockers/docker-database/supervisord.conf.j2 Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:ycoheNvidia/sonic-buildimage into…

f568e64

… container_net_host_remove

ycoheNvidia force-pushed the container_net_host_remove branch from e52c538 to f568e64 Compare September 28, 2023 13:21

qiluo-msft reviewed Oct 2, 2023

View reviewed changes

files/build_templates/docker_image_ctl.j2 Outdated Show resolved Hide resolved

Indentation

10c37ae

ycoheNvidia closed this Apr 10, 2024

Container net host remove #15176

Container net host remove #15176

Conversation

ycoheNvidia commented May 21, 2023 • edited Loading

Why I did it

How I did it

How to verify it

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

qiluo-msft Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

bind 127.0.0.1 ::1

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycoheNvidia commented Aug 1, 2023

qiluo-msft commented Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaqiangz commented Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaqiangz Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuvarnaMeenakshi commented Oct 10, 2023 • edited Loading

ycoheNvidia commented May 21, 2023 •

edited

Loading

qiluo-msft Jun 27, 2023 •

edited

Loading

qiluo-msft Jun 27, 2023 •

edited

Loading

qiluo-msft Jun 27, 2023 •

edited

Loading

qiluo-msft Jun 27, 2023 •

edited

Loading

qiluo-msft commented Aug 11, 2023 •

edited

Loading

yaqiangz commented Sep 14, 2023 •

edited

Loading

qiluo-msft Sep 18, 2023 •

edited

Loading

yaqiangz Sep 19, 2023 •

edited

Loading

SuvarnaMeenakshi commented Oct 10, 2023 •

edited

Loading