Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve controller container security #1112

Closed
philnichol opened this issue Dec 24, 2021 · 3 comments
Closed

Improve controller container security #1112

philnichol opened this issue Dec 24, 2021 · 3 comments
Assignees
@vijtrip2
Labels
kind/enhancement Categorizes issue or PR as related to existing feature enhancements. needs-priority Indicates a PR lacks a `priority/foo` label and requires one.
Milestone
ACK Core (2021-11...

Comments

@philnichol
Copy link

Hi and thanks in advance for reading this 😄

Is your feature request related to a problem?
Currently if I scan the ACK controller images for vulnerabilities and CIS standards it does not pass, this means introducing potential vulnerabilities, and forcing me to raise Risk Acceptance requests with security in order to use this in production

Output from live container image:

$ trivy image public.ecr.aws/aws-controllers-k8s/s3-controller:v0.0.7
2021-12-24T18:47:02.274Z	INFO	Detected OS: amazon
2021-12-24T18:47:02.274Z	INFO	Detecting Amazon Linux vulnerabilities...
2021-12-24T18:47:02.277Z	INFO	Number of language-specific files: 1
2021-12-24T18:47:02.277Z	INFO	Detecting gobinary vulnerabilities...

public.ecr.aws/aws-controllers-k8s/s3-controller:v0.0.7 (amazon 2 (Karoo))
==========================================================================
Total: 20 (UNKNOWN: 0, LOW: 0, MEDIUM: 13, HIGH: 0, CRITICAL: 7)

+--------------------+------------------+----------+--------------------+------------------------+---------------------------------------+
|      LIBRARY       | VULNERABILITY ID | SEVERITY | INSTALLED VERSION  |     FIXED VERSION      |                 TITLE                 |
+--------------------+------------------+----------+--------------------+------------------------+---------------------------------------+
| curl               | CVE-2021-22945   | MEDIUM   | 7.76.1-7.amzn2.0.2 | 7.79.1-1.amzn2.0.1     | curl: use-after-free and              |
|                    |                  |          |                    |                        | double-free in MQTT sending           |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-22945 |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-22946   |          |                    |                        | curl: Requirement to use              |
|                    |                  |          |                    |                        | TLS not properly enforced             |
|                    |                  |          |                    |                        | for IMAP, POP3, and...                |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-22946 |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-22947   |          |                    |                        | curl: Server responses                |
|                    |                  |          |                    |                        | received before STARTTLS              |
|                    |                  |          |                    |                        | processed after TLS handshake         |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-22947 |
+--------------------+------------------+          +                    +                        +---------------------------------------+
| libcurl            | CVE-2021-22945   |          |                    |                        | curl: use-after-free and              |
|                    |                  |          |                    |                        | double-free in MQTT sending           |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-22945 |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-22946   |          |                    |                        | curl: Requirement to use              |
|                    |                  |          |                    |                        | TLS not properly enforced             |
|                    |                  |          |                    |                        | for IMAP, POP3, and...                |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-22946 |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-22947   |          |                    |                        | curl: Server responses                |
|                    |                  |          |                    |                        | received before STARTTLS              |
|                    |                  |          |                    |                        | processed after TLS handshake         |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-22947 |
+--------------------+------------------+----------+--------------------+------------------------+---------------------------------------+
| nspr               | CVE-2021-43527   | CRITICAL | 4.25.0-2.amzn2     | 4.32.0-1.amzn2         | nss: Memory corruption in             |
|                    |                  |          |                    |                        | decodeECorDsaSignature with           |
|                    |                  |          |                    |                        | DSA signatures (and RSA-PSS)          |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-43527 |
+--------------------+                  +          +--------------------+------------------------+                                       +
| nss                |                  |          | 3.53.1-7.amzn2     | 3.67.0-4.amzn2.0.1     |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
+--------------------+                  +          +--------------------+------------------------+                                       +
| nss-softokn        |                  |          | 3.53.1-6.amzn2     | 3.67.0-3.amzn2         |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
+--------------------+                  +          +                    +                        +                                       +
| nss-softokn-freebl |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
+--------------------+                  +          +--------------------+------------------------+                                       +
| nss-sysinit        |                  |          | 3.53.1-7.amzn2     | 3.67.0-4.amzn2.0.1     |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
+--------------------+                  +          +                    +                        +                                       +
| nss-tools          |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
+--------------------+                  +          +--------------------+------------------------+                                       +
| nss-util           |                  |          | 3.53.1-1.amzn2     | 3.67.0-1.amzn2         |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
|                    |                  |          |                    |                        |                                       |
+--------------------+------------------+----------+--------------------+------------------------+---------------------------------------+
| vim-minimal        | CVE-2021-3778    | MEDIUM   | 2:8.1.1602-1.amzn2 | 2:8.2.3642-1.amzn2.0.1 | vim: heap-based buffer overflow       |
|                    |                  |          |                    |                        | in utf_ptr2char() in mbyte.c          |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3778  |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-3796    |          |                    |                        | vim: use-after-free in                |
|                    |                  |          |                    |                        | nv_replace() in normal.c              |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3796  |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-3872    |          |                    |                        | vim: heap-based buffer overflow in    |
|                    |                  |          |                    |                        | win_redr_status() in drawscreen.c     |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3872  |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-3875    |          |                    |                        | vim: heap-based buffer overflow       |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3875  |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-3968    |          |                    |                        | vim: Heap use-after-free              |
|                    |                  |          |                    |                        | in ml_append_int function             |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3968  |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-3973    |          |                    |                        | vim: Heap based buffer                |
|                    |                  |          |                    |                        | overflow in findfile.c                |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3973  |
+                    +------------------+          +                    +                        +---------------------------------------+
|                    | CVE-2021-3974    |          |                    |                        | vim: Use after free in regexp_nfa.c   |
|                    |                  |          |                    |                        | -->avd.aquasec.com/nvd/cve-2021-3974  |
+--------------------+------------------+----------+--------------------+------------------------+---------------------------------------+

usr/bin/controller (gobinary)
=============================
Total: 4 (UNKNOWN: 1, LOW: 0, MEDIUM: 1, HIGH: 2, CRITICAL: 0)

+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
|         LIBRARY          | VULNERABILITY ID | SEVERITY |         INSTALLED VERSION          |           FIXED VERSION            |                 TITLE                 |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
| github.com/gogo/protobuf | CVE-2021-3121    | HIGH     | v1.3.1                             | 1.3.2                              | gogo/protobuf:                        |
|                          |                  |          |                                    |                                    | plugin/unmarshal/unmarshal.go         |
|                          |                  |          |                                    |                                    | lacks certain index validation        |
|                          |                  |          |                                    |                                    | -->avd.aquasec.com/nvd/cve-2021-3121  |
+--------------------------+------------------+          +------------------------------------+------------------------------------+---------------------------------------+
| golang.org/x/crypto      | CVE-2020-29652   |          | v0.0.0-20200622213623-75b288015ac9 | v0.0.0-20201216223049-8b5274cf687f | golang: crypto/ssh: crafted           |
|                          |                  |          |                                    |                                    | authentication request can            |
|                          |                  |          |                                    |                                    | lead to nil pointer dereference       |
|                          |                  |          |                                    |                                    | -->avd.aquasec.com/nvd/cve-2020-29652 |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
| golang.org/x/text        | CVE-2021-38561   | UNKNOWN  | v0.3.3                             | 0.3.7                              | -->avd.aquasec.com/nvd/cve-2021-38561 |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
| k8s.io/client-go         | CVE-2020-8565    | MEDIUM   | v0.18.2                            | 0.20.0-alpha.2                     | kubernetes: Incomplete fix            |
|                          |                  |          |                                    |                                    | for CVE-2019-11250 allows for         |
|                          |                  |          |                                    |                                    | token leak in logs when...            |
|                          |                  |          |                                    |                                    | -->avd.aquasec.com/nvd/cve-2020-8565  |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
$ dockle public.ecr.aws/aws-controllers-k8s/s3-controller:v0.0.7
WARN	- CIS-DI-0001: Create a user for the container
	* Last user should not be root
INFO	- CIS-DI-0005: Enable Content trust for Docker
	* export DOCKER_CONTENT_TRUST=1 before docker pull/build
INFO	- CIS-DI-0006: Add HEALTHCHECK instruction to the container image
	* not found HEALTHCHECK statement

Describe the solution you'd like
I would propose we update the templates in code-generator to do the following:

  • Run as non-root
  • The final image should be "FROM scratch" since most of the vulnerabilities are related to packages that aren't required for the container to run
  • Add more explicit securityContext to pod and container, inc. CAP: DROP ALL to the template deployment yaml manifest, since these containers shouldn't need any special privileges
  • One for a separate ticket, but also could be worth adding a CIS/vuln scan to the CI steps?

Describe alternatives you've considered
I had a go at using a non-root scratch container that ran the s3 e2e tests which removed all CIS warnings and reduced the vulnerabilities (and removed all 7 CRITICAL vulns) to only the ones in the final go binary, let me know if this is something of interest since I'd be happy to tidy up and contribute this.

$ trivy image aws-controllers-k8s:s3-75752b2
2021-12-24T18:46:24.846Z	INFO	Number of language-specific files: 1
2021-12-24T18:46:24.846Z	INFO	Detecting gobinary vulnerabilities...

bin/controller (gobinary)
=========================
Total: 4 (UNKNOWN: 1, LOW: 0, MEDIUM: 1, HIGH: 2, CRITICAL: 0)

+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
|         LIBRARY          | VULNERABILITY ID | SEVERITY |         INSTALLED VERSION          |           FIXED VERSION            |                 TITLE                 |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
| github.com/gogo/protobuf | CVE-2021-3121    | HIGH     | v1.3.1                             | 1.3.2                              | gogo/protobuf:                        |
|                          |                  |          |                                    |                                    | plugin/unmarshal/unmarshal.go         |
|                          |                  |          |                                    |                                    | lacks certain index validation        |
|                          |                  |          |                                    |                                    | -->avd.aquasec.com/nvd/cve-2021-3121  |
+--------------------------+------------------+          +------------------------------------+------------------------------------+---------------------------------------+
| golang.org/x/crypto      | CVE-2020-29652   |          | v0.0.0-20200622213623-75b288015ac9 | v0.0.0-20201216223049-8b5274cf687f | golang: crypto/ssh: crafted           |
|                          |                  |          |                                    |                                    | authentication request can            |
|                          |                  |          |                                    |                                    | lead to nil pointer dereference       |
|                          |                  |          |                                    |                                    | -->avd.aquasec.com/nvd/cve-2020-29652 |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
| golang.org/x/text        | CVE-2021-38561   | UNKNOWN  | v0.3.3                             | 0.3.7                              | -->avd.aquasec.com/nvd/cve-2021-38561 |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
| k8s.io/client-go         | CVE-2020-8565    | MEDIUM   | v0.18.2                            | 0.20.0-alpha.2                     | kubernetes: Incomplete fix            |
|                          |                  |          |                                    |                                    | for CVE-2019-11250 allows for         |
|                          |                  |          |                                    |                                    | token leak in logs when...            |
|                          |                  |          |                                    |                                    | -->avd.aquasec.com/nvd/cve-2020-8565  |
+--------------------------+------------------+----------+------------------------------------+------------------------------------+---------------------------------------+
$ dockle aws-controllers-k8s:s3-75752b2
SKIP	- DKL-LI-0001: Avoid empty password
	* failed to detect etc/shadow,etc/master.passwd
SKIP	- DKL-LI-0002: Be unique UID/GROUP
	* failed to detect etc/group
INFO	- CIS-DI-0005: Enable Content trust for Docker
	* export DOCKER_CONTENT_TRUST=1 before docker pull/build
INFO	- CIS-DI-0006: Add HEALTHCHECK instruction to the container image
	* not found HEALTHCHECK statement
@philnichol philnichol added the kind/enhancement Categorizes issue or PR as related to existing feature enhancements. label Dec 24, 2021
@vijtrip2
Copy link
Contributor

Hi @philnichol, Thank you so much fo bringing this to our attention.

We have plans to enable regular image scan on ACK controller images in near future, and we will work for patching the security vulnerabilities that are currently present ASAP.

@a-hilaly a-hilaly added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 28, 2021
@vijtrip2 vijtrip2 mentioned this issue Dec 28, 2021
Merged
ack-bot pushed a commit to aws-controllers-k8s/test-infra that referenced this issue Dec 28, 2021
@vijtrip2
use Dockerfile from code-generator for building controller image (#167)
8dc93e3 
Issue #, if available: aws-controllers-k8s/community#1112

Description of changes:
* use Dockerfile from code-generator for building controller image
* This helps in maintaining only single Dockerfile for building ACK controller images

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@vijtrip2 vijtrip2 mentioned this issue Dec 30, 2021
Closed
@philnichol philnichol mentioned this issue Dec 30, 2021
Merged
@philnichol
Copy link
Author

I've pushed this draft just in case it helps at all since the work had been done, but no worries if it's not the direction you're after :) aws-controllers-k8s/code-generator#254

ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Jan 3, 2022
@philnichol
Hardening container (#254)
d29e4e8 
Issue #, if available:

Relates aws-controllers-k8s/community#1112

Description of changes:
- ~~No longer runs as root, runs as nobody instead, since runtime is from scratch I've added a "dummy" /etc/shadow file~~
- ~~Runtime image is now "from scratch" since we don't need much other than ca-certs and the binary itself (eg. curl, vim, etc)~~
- Standard principle of least privilege security caps in deployment manifest (drop all plus explicit least privilege deployment/pod settings and capabilities)

This is a draft since there's still stuff missing, and not sure if you would want to go in a different direction

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@vijtrip2 vijtrip2 self-assigned this Jan 6, 2022
@vijtrip2 vijtrip2 mentioned this issue Jan 6, 2022
Merged
ack-bot pushed a commit to aws-controllers-k8s/runtime that referenced this issue Jan 6, 2022
@vijtrip2
changes due to controller-runtime v0.11.0 upgrade (#63)
dff8696 
Issue #, if available: aws-controllers-k8s/community#1112

Description of changes:
* Upgrade of controller-runtime to v0.11.0
* Upgrade of go.mod to 1.17 because that is minimal go version for controller-runtime library
*  Keeping the indirect import in go.mod file because removing them causes the error "go.mod needs changes. Run go mod tidy" during local development
> At go 1.17 and above, the go command adds an indirect requirement for each module that provides any package imported (even indirectly) by a package or test in the main module or passed as an argument to go get. These more comprehensive requirements enable module graph pruning and lazy module loading.
* `Reconcile` method in reconcilers now accepts a `context.Context` parameter
* Replace `ACK RuntimeMetaObject` with `controller-runtime Object`
* Replace `desired.RuntimeObject().DeepCopyObject()` with `desired.DeepCopy().RuntimeObject()` because the former does not return implementation of `controller-runtime Object`
* Remove `RuntimeMetaObject()` method from AWSResource interface
* Use `controller-runtime client Object` instead of `apimachinery runtime Object`

------

* Tested locally by running e2e tests for ecr-controller
* Validated that newly generated image has no golang security vulnerabilities.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@vijtrip2 vijtrip2 mentioned this issue Jan 6, 2022
Merged
ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Jan 7, 2022
@vijtrip2
security hardening of service controllers (#257)
e8d1493 
Issue #, if available: aws-controllers-k8s/community#1112

Description of changes:
* Update base image to `public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nonroot:2021-12-01-1638322424` and golang image to `1.17.5` for building controller images
* Updated the `deployment.yaml` files to runAsUser 1000. This userId was selected as random. 
* Updated ACK runtime to `v0.16.0`

----------------

* validated that controller runs correctly when executed as non root user
* tested locally by running ecr-controller e2e tests
* validated that there were no security vulnerabilities in generated image

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
RedbackThomson pushed a commit to RedbackThomson/ack-test-infra that referenced this issue Jan 8, 2022
@vijtrip2
use Dockerfile from code-generator for building controller image (aws…
9c9b4ee 
…-controllers-k8s#167)

Issue #, if available: aws-controllers-k8s/community#1112

Description of changes:
* use Dockerfile from code-generator for building controller image
* This helps in maintaining only single Dockerfile for building ACK controller images

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@vijtrip2
Copy link
Contributor

All these vulnerabilities are resolved now and ACK controller image now run as non-root. I am closing this issue and creating new issue for aws-sdk-go upgrade . #1128

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vijtrip2 vijtrip2

Labels
kind/enhancement Categorizes issue or PR as related to existing feature enhancements. needs-priority Indicates a PR lacks a `priority/foo` label and requires one.
Projects
None yet
Milestone
ACK Core (2021-11-15 thru 2021-11-26)
Development

No branches or pull requests
3 participants
@a-hilaly @philnichol @vijtrip2