-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat(adr): Docker container guidelines #220
Conversation
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
c600139
to
89f6e45
Compare
|
||
## containers for the edge | ||
|
||
what's the standard for edgex containers |
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.
Add a references section. Include the following URLs:
- OWASP Docker Recommendations https://cheatsheetseries.owasp.org/cheatsheets/Docker_Security_Cheat_Sheet.html
- CIS Docker Benchmark https://workbench.cisecurity.org/files/2433/download/2786 (registration required)
- Docker-compose reference https://docs.docker.com/compose/compose-file
Also provide cross-references for each recommendation above.
|
||
When deploying Docker images, the following flags should be set for heightened security. | ||
|
||
- To avoid escalation of privileges each docker container should use the `--security-opt=no-new-privileges` flag. More details about this flag can be found [here](https://docs.docker.com/engine/reference/run/#security-configuration) |
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.
EdgeX has no control over the user's docker daemon. We are only allowed to exercise control via Dockerfile or docker-compose. In this case, we want
security_opt:
- "no-new-privileges:true"
|
||
- To avoid escalation of privileges each docker container should use the `--security-opt=no-new-privileges` flag. More details about this flag can be found [here](https://docs.docker.com/engine/reference/run/#security-configuration) | ||
|
||
- To further prevent privilege escalation attacks the user should be set for the docker container using the `--user=<userid>` or `-u=<userid>` flag. More details about this flag can be found [here](https://docs.docker.com/engine/reference/run/#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.
EdgeX does not launch containers via docker run CLI. Rewrite to focus on what is available in the docker-compose.
Please also provide guidance on what user ID's will be used in the docker-compose.
|
||
- To avoid a faulty or compromised containers from consuming excess amounts of the host of its resources `resource limits` should be set for each container. more details about `resource limits` can be found [here](https://docs.docker.com/config/containers/resource_constraints/) | ||
|
||
- To avoid attackers from writing data to the containers and modifying their files the `--read_only` flag should be set. More details about this flag can be found [here](https://docs.docker.com/compose/compose-file/#domainname-hostname-ipc-mac_address-privileged-read_only-shm_size-stdin_open-tty-user-working_dir) |
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.
Apply to docker-compose not docker run.
- To avoid attackers from writing data to the containers and modifying their files the `--read_only` flag should be set. More details about this flag can be found [here](https://docs.docker.com/compose/compose-file/#domainname-hostname-ipc-mac_address-privileged-read_only-shm_size-stdin_open-tty-user-working_dir) | ||
|
||
> NOTE: exception | ||
If a container is required to have write permission to function, then this flag will not work. For example, the vault requires write permission to write secrets. In this case the `--read_only` flag will not be used. |
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.
Update technical explanation. Vault needs to run setcap in order to lock pages in memory, but will otherwise run fine if memory page locking is disabled. In many cases, mounting a writable docker volume in a strategic place will resolve the issue.
89f6e45
to
af99bbd
Compare
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
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.
Some minor changes needed
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
docs_src/design/adr/system-management/001-docker-image-guidelines.md
Outdated
Show resolved
Hide resolved
af99bbd
to
a7a777d
Compare
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.
LGTM
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.
LGTM
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.
LGTM - we should be able to validate the configuration within a running Jenkins Pipeline using Docker Security Bench. Just wondering if this was already looked at during the work completed 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.
I think this document is good - but to clarify, this is just guidance (correct??). Is there any of this that we plan to incorporate into our Docker Compose files?
With regard to the no-new-privileges options, I think we should note that this may be more difficult when dealing with a device-service that may require privilege access in order to get connected to drivers/etc for sensors/devices.
The intent is to use this ADR to justify a number of the items in the security backlog that will implement these recommendations. So yes, there are active, in-process plans to incorporate this guidance.
No-new-privileges is used to block things like "sudo" in a container. If a container starts with privilege, then no-new-privileges won't prevent the container from retaining privilege. So if there was a device driver that needed root privilege to talk, it could still specify no-new-privilege and start as root. |
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.
Overall, the recommendations seem fine, but I did have a few comments/questions.
Two other global comments, it seems odd that this ADR falls under system-management, as it's more about deployment than management. Second, maybe I misunderstood, but I thought we sequentially number all ADRs, is it really the case that we number them in series per domain? This doesn't seem to be the case with Core, which only has a single ADR which is 0003:
services: | ||
device-virtual: | ||
image: ${REPOSITORY}/docker-device-virtual-go${ARCH}:${DEVICE_VIRTUAL_VERSION} | ||
user: 4000:4000 # user option using an unprivileged 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.
Are there any guidelines as to what UID ranges should be used? On many distros system user UIDs (i.e. used by services) range from 100-999, and regular user UIDs 1000+.
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.
Docker user NS remapping, when enabled, usually uses a very high user range.
I am a proponent of giving each service its own UID (maybe == its port number) so that the secret store tokens can be individually ACLed.
> NOTE: exception | ||
Sometimes containers will require root access to perform their fuctions. For example the System Management Agent requires root access to control other Docker containers. In this case you would allow it run as default root user. | ||
|
||
- To avoid a faulty or compromised containers from consuming excess amounts of the host of its resources `resource limits` should be set for each container. More details about `resource limits` can be found [here](https://docs.docker.com/config/containers/resource_constraints/). This follows Rule #7 for Docker security found [here](https://cheatsheetseries.owasp.org/cheatsheets/Docker_Security_Cheat_Sheet.html#rule-7-limit-resources-memory-cpu-file-descriptors-processes-restarts). |
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.
Should this ADR give some recommendations as to what these resource limitations should be? I also imagine the limits might be different for some of our runtime dependencies (e.g. kong, redis, ...).
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.
Don't think we can as they will be system and use case dependent.
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 we're not going to provide any guidance (e.g. minimum memory requirement), then we should add some verbiage that this is an exercise left to the end-user base on system resources and use cases.
Also I'll note that the docker resource constraints link above is for the docker command itself, not docker-compose. Does compose support YAML specified resource constraints?
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.
Does compose support YAML specified resource constraints?
If a container is required to have write permission to function, then this flag will not work. For example, the vault needs to run setcap in order to lock pages in memory. In this case the `--read_only` flag will not be used. | ||
|
||
> NOTE: alternatives | ||
If writing non-persistent data is required (ex. a config file) then a temporary filesystem mount can be used to accomplish this goal while still enforcing `--read_only`. Mounting a `tmpfs` in Docker gives the container a temporary location in the host systems memory to modify files. This location will be removed once the container is stopped. More details about `tmpfs` can be found [here](https://docs.docker.com/storage/tmpfs/) |
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 about our persistence later (e.g. redis)?
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.
Agree with Tony. Should also mention plain 'old volumes for normal persistence needs.
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.
Interestingly, Kubernetes might be an issue here. It just doesn't support volumes in the normal way that Docker does.
|
||
When deploying Docker images, the following flags should be set for heightened security. | ||
|
||
- To avoid escalation of privileges each docker container should use the `no-new-privileges` option in their Docker compose file (example below). More details about this flag can be found [here](https://docs.docker.com/engine/reference/run/#security-configuration). This follows Rule #4 for Docker security found [here](https://cheatsheetseries.owasp.org/cheatsheets/Docker_Security_Cheat_Sheet.html#rule-4-add-no-new-privileges-flag). |
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've also had instances of device services using the --privileged
flag in order to access DBus and/or actual hardware devices. It'd be nice if this was called out explicitly as not allowed. In addition, we should provide some guidelines as to how to properly allow hardware access (e.g. writing custom AppArmor rules) from within a 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.
@tonyespy Jim White I believe commented on this earlier. I will repeat what I said before.
no-new-privileges doesn't prevent one from running privileged containers. What it prevents is tools like "sudo" from running in a container thus a going from user mode to root mode. It also prevents capabilities defined on extended file attributes from being effected. (For example, the capability to bind to a low-numbered port.).
Docker-compose v3 doesn't allow one to customize cgroups like the v2 did. For example, you can no longer pass in device_cgroup_rules. So if you are accessing a device that isn't in docker's built-in allow list, privileged mode is one way to allow device access to continue to function. (Curious myself if this is the only way, or maybe we should downrev the docker-compose to v2... hmmm.)
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 @bnevis-i. Two additional points...
- The device-bluetooth-c (still in holding) device service recommends running the service using
docker run --privileged
, and on systems where AppArmor is supported, includes an example of how to pass an AppArmor profile to docker run. Do you know if compose v2 also allowed AppArmor profiles to be specified? - Frankly, if we're going to suggest that someone run a service with
--privileged
, I'm not sure why we're even bothering to run the service in a container in the first place.
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.
Are you up to adding a recommendation to prefer
security_opt: [ "apparmor:unconfined" ]
or a custom apparmor profile vs running --privileged
This doesn't negate the guidance regarding no-new-privileges, but it does add additional (and worthy) guidance to not use --privileged at all.
77c74b6
e38661a
to
77c74b6
Compare
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.
LGTM
Thanks, this is helpful
Specific Docker Security Bench checks as referenced in this ADR are: 2.17 Ensure containers are restricted from acquiring new privileges (Scored) 4 - Container Images and Build File 5 - Container Runtime 5.12 Ensure that the container's root filesystem is mounted as read only |
I think we should start with the following checks and known exceptions:
We have to to exclude all of section 2 because EdgeX doesn't install or configure the users's docker daemon.
We can't check this at daemon level, but we can code it in using security_opt (no checker for it).
Suggest we run check 4.1 (the only scored checker in the section) and add exceptions for the containers requiring root.
Suggest hold off adding the checker until we have syntax in the docker-compose to set limits.
Good check. One exception so far. |
Current output of the above proposal.
|
Thank you @bnevis-i |
@brian-intel Please change the commit message to "feat(adr):" instead of "ADR:" |
77c74b6
to
2511d89
Compare
close edgexfoundry#206 Signed-off-by: Brian McGinn <[email protected]>
a40994d
to
bb6cc09
Compare
Signed-off-by: edgex-jenkins <[email protected]>
ADR: Docker container guidelines
close #206
Signed-off-by: Brian McGinn [email protected]
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
New ADR for docker security guidelines
What is the current behavior?
Current docker container deployments lack updated security guidelines
Issue Number: 206
What has been updated?
Are there any specific instructions or things that should be known prior to reviewing?
Other information