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.
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
SONiC Container Hardening #1364
SONiC Container Hardening #1364
Changes from all commits
e971302
67afbf0
80c9557
2a3964d
29e3da4
edac201
9469782
e4e5c05
4ce3232
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There is similar work done with Multi ASIC design, it is better to converge before making any decision changing net=host -> bridge etc.
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.
Not familiar with it, what is the work being done there?
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 believe, BGP container needs full network access in order to function properly, do we have the list of containers that can be migrated without net=host? Some of the container (DHCP relay, SNMP..etc) directly work on the Linux networking, removing net=host, might require additional changes at the application level.
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.
On the SNMP container we have managed to remove the net=host.
For BGP - we also have a PR up with @maipbui changes that we can review.
For DHCP - we'll need to check this.
I have a composed list of containers and the suggestions on whether we can remove the flags or not, I'll add it to the HLD as an appendix.
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.
Please help review code PRs which we included in this HLD PR's description. @venkatmahalingam Yes, BGP container needs host network mode.
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.
Will this design cover SDK change? SONiC containers are massively interactive with Redis database, it is required to let our SDK adapt bridge mode container
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.
Why does this require SDK changes?
Regarding the SDK - since it resides in a single container, we have decided against reducing the privileges and capabilities of this container, as this will require massive SDK re-design (and this is not our purpose).
Also - the SDK container is not at a high risk of compromise since it does not interact with the outside network.
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.
@Blueve Which SDK? Is it switch ASIC vendor SDK or swss-common library?
Could you give specific sample function in the SDK you are worrying about?
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.
@qiluo-msft swss-common, do we still have a unified way to interact with Redis database with this design?
Current we can use two approaches to interact Redis in sdk:
With this design, both of them are not available I think.
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.
Both are still applicable with this design (tested).
UDS should not be affected, since reducing the privileges of the redis container does not affect the Unix socket (on a different level of permissions - file access permissions).
Local IP might require a slight change, but you can check it in the relevant PR and it is currently working.
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 feedback in sonic community review meeting, please considering first unifying single-asic to multi-asic namespace design, and then leverage the current bridge net mode already implemented on multi-asic system.
ref: sonic-net/sonic-buildimage#3856 #Pending
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! Will review this and apply the relevant fixes
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.
What is the user experience with this change with the current container capabilities? What is the plan to mitigate this?
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.
User should not experience any changes.
If there are, then one of the following should occur:
We should go with option 1 first, and only after we have decided option 1 is not feasible - option 2 is on the table.
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 wonder whether syslog(especially send syslog message to remote rsyslog server) can work well with this change?
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.
Syslog can function with this.
Also, the syslog is currently running on top of the host - where nothing is changed there.
So I'm going to split my answer:
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.
It should be clarified the syslog I mean is generated inside container. I just tried image built by this snmp network mode related PR: sonic-net/sonic-buildimage#15176 (SONiC.master-15176.360116-6bb24c135). And cannot find any syslog generated by snmp container (contains 'snmp#') in local host syslog file or our syslog server, and process to dynamically generate rsyslog inside container
containercfgd
doesn't startup. Not sure whether you have verified this, could you confirm that?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.
Can you please refer this item to the specific PR?
If there is some implementation concern that you have noticed - I'd appreciate it if you could direct it to the relevant PR.
This is a good item and I do not recall if this was reviewed thoroughly (might have missed something)
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.
Is there a tool that knows what containers are running privileged mode?
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.
There might be, requires a bit of research.
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.
maybe you can check by
docker inspect container_name | grep Privileged
?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.
This might be applicable for the --privileged flag, but this HLD is also discussing host network.
So while this might work for one of the HLD item (removing containers from privilege mode) it does not scope the whole feature list.
So this command might be a good start, but we should look at a more encompassing approach to check this (and not stop 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.
Is there any impact w.r.t memory and cpu as it is running in different network name spaces?
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.
Negligible, as it is part of the infra.
I'll be more precise - the impact is as much of how much the Linux ACL takes, but this should not be visible to any user.
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.
What if SNMP container wants to listen on some particular interface on the host? container hardening will be disabled by default for now? can this be driven based on FEATURE table config?
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.
If the SNMP container would like to listen to a specific interface - you can check this:
https://github.com/sonic-net/sonic-buildimage/pull/15176/files
I think that this PR will showcase your specific use-case.
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.
how do we know if the appliation does not use SYS_TIME?
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.
We don't, but if a specific container requires this - it can add it as a specific flag only for this container.