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.

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

Small improvement of user experience #467

Merged
merged 48 commits into from
Apr 27, 2021
Merged

Conversation

razvanstoica89
Copy link
Contributor

Add details about remediation measures for host configuration tests

@konstruktoid
Copy link
Collaborator

Thanks @razvanstoica89, and it was a humble title you picked, calling this PR "small".

@konstruktoid
Copy link
Collaborator

Loads of updates 👍
Will test as soon as possible.

@konstruktoid
Copy link
Collaborator

Can you poke me @razvanstoica89 when you believe you are finished with this PR so I can start testing?
No need to test if it's a work in progress.

@razvanstoica89
Copy link
Contributor Author

Yes of course. Thanks

@konstruktoid
Copy link
Collaborator

konstruktoid commented Mar 25, 2021

the json file doesnt seem to be valid.

$ jq log/docker-bench-security.sh.log.json 
jq: error: docker/0 is not defined at <top-level>, line 1:
log/docker-bench-security.sh.log.json    
jq: error: bench/0 is not defined at <top-level>, line 1:
log/docker-bench-security.sh.log.json           
jq: error: security/0 is not defined at <top-level>, line 1:
log/docker-bench-security.sh.log.json                 
jq: 3 compile errors

@konstruktoid
Copy link
Collaborator

Since you've created the log directory, perhaps rename without the .sh extension

@razvanstoica89
Copy link
Contributor Author

the json file doesnt seem to be valid.

$ jq log/docker-bench-security.sh.log.json 
jq: error: docker/0 is not defined at <top-level>, line 1:
log/docker-bench-security.sh.log.json    
jq: error: bench/0 is not defined at <top-level>, line 1:
log/docker-bench-security.sh.log.json           
jq: error: security/0 is not defined at <top-level>, line 1:
log/docker-bench-security.sh.log.json                 
jq: 3 compile errors

Looks like you're trying to validate that the string "log/docker-bench-security.sh.log.json" is a valid json.
I think a better approach is cat log/docker-bench-security.log.json | jq empty.

As far as I know the only way you can get an invalid json log is to stop the script manually while it is running. Even if you run it again, you will be left with an invalid json log.

@konstruktoid
Copy link
Collaborator

Running a standrd installation without any containers:

Section A - Check results

[INFO] 1 - Host Configuration
[INFO] 1.1 - General Configuration
[NOTE] 1.1.1  - Ensure the container host has been Hardened (Not Scored)
[INFO] -c
[INFO]        * Using 20.10.5, verify is it up to date as deemed necessary
[INFO]        * Your operating system vendor may provide support and security maintenance for Docker
[INFO] 1.2 - Linux Hosts Specific Configuration
WARNING: No swap limit support
[WARN] -s
[INFO] -c
[INFO]       * Users: vagrant
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[INFO] -c
[INFO]        * File not found
[INFO] -c
[INFO]         * File not found
[WARN] -s
[INFO] -c
[INFO]         * File not found

[INFO] 2 - Docker daemon configuration 
[WARN] -s
[PASS] -s
[PASS] -s
[PASS] -s
[PASS] -s
[INFO] -c
[INFO]      * Docker daemon not listening on TCP
[INFO] -c
[INFO]      * Default ulimit doesn't appear to be set
[WARN] -s
[PASS] -s

@konstruktoid
Copy link
Collaborator

Looks like you're trying to validate that the string "log/docker-bench-security.sh.log.json" is a valid json.
I think a better approach is cat log/docker-bench-security.log.json | jq empty.

You are correct, cat log/docker-bench-security.log.json | jq -r works.

@razvanstoica89
Copy link
Contributor Author

Did you manage to test the changes I made?

@konstruktoid
Copy link
Collaborator

Did you manage to test the changes I made?

Yeah, but #467 (comment) still exists.

@razvanstoica89
Copy link
Contributor Author

Did you manage to test the changes I made?

Yeah, but #467 (comment) still exists.

Yes. You are right. By d0443cc I solved the problem reported in #467 (comment) but I didn't let you know.

@konstruktoid
Copy link
Collaborator

~$ git clone https://github.com/docker/docker-bench-security
Cloning into 'docker-bench-security'...
[...]
~$ cd docker-bench-security/
~$ git checkout -b razvanstoica89-master master
[...]
~$ git pull git://github.com/razvanstoica89/docker-bench-security.git master
[...]
~$ git log | grep -A3 d0443c
commit d0443cc817cf17452f41510954e450a320b59c6a
Author: Razvan Stoica <razvan.stoica89@gmail.com>
Date:   Mon Mar 29 15:22:14 2021 +0300
~$ curl -sSL get.docker.com | sh
[...]
$ sudo bash ./docker-bench-security.sh
# --------------------------------------------------------------------------------------------
# Docker Bench for Security v1.3.6
#
# Docker, Inc. (c) 2015-2021
#
# Checks for dozens of common best-practices around deploying Docker containers in production.
# Inspired by the CIS Docker Benchmark v1.2.0.
# --------------------------------------------------------------------------------------------

Initializing 2021-04-08T20:42:29+00:00


Section A - Check results

[INFO] 1 - Host Configuration
[INFO] 1.1 - General Configuration
[NOTE] 1.1.1  - Ensure the container host has been Hardened (Not Scored)
[PASS] -c
[INFO]        * Using 20.10.5, verify is it up to date as deemed necessary
[INFO] 1.2 - Linux Hosts Specific Configuration
WARNING: No swap limit support
[WARN] -s
[INFO] -c
[INFO]       * Users:
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[INFO] -c
[INFO]        * File not found
[INFO] -c
[INFO]         * File not found
[WARN] -s
[INFO] -c
[INFO]         * File not found

[INFO] 2 - Docker daemon configuration
[WARN] -s
[PASS] -s
[PASS] -s
[PASS] -s
[PASS] -s
[INFO] -c
[INFO]      * Docker daemon not listening on TCP

@razvanstoica89
Copy link
Contributor Author

Please test it using the sudo sh ./docker-bench-security.sh command as recommended in the original README.md file.

@konstruktoid
Copy link
Collaborator

sh is linked to bash on multiple distributions.

$ sudo sh ./docker-bench-security.sh
# --------------------------------------------------------------------------------------------
# Docker Bench for Security v1.3.6
#
# Docker, Inc. (c) 2015-2021
#
# Checks for dozens of common best-practices around deploying Docker containers in production.
# Inspired by the CIS Docker Benchmark v1.2.0.
# --------------------------------------------------------------------------------------------

Initializing 2021-04-09T09:52:09+00:00


Section A - Check results

[INFO] 1 - Host Configuration
[INFO] 1.1 - General Configuration
[NOTE] 1.1.1  - Ensure the container host has been Hardened (Not Scored)
[PASS] -c
[INFO]        * Using 20.10.5, verify is it up to date as deemed necessary
[INFO] 1.2 - Linux Hosts Specific Configuration
[WARN] -s
[INFO] -c
[INFO]       * Users: 
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[WARN] -s
[INFO] -c
[INFO]        * File not found
[INFO] -c
[INFO]        * File not found
[INFO] -c
[INFO]         * File not found
[WARN] -s
[INFO] -c
[INFO]         * File not found

[INFO] 2 - Docker daemon configuration
[WARN] -s
[PASS] -s
[PASS] -s
[PASS] -s
[PASS] -s
[INFO] -c
[INFO]      * Docker daemon not listening on TCP
^C
$ ls -l $(which sh)
lrwxrwxrwx. 1 root root 4 Jan 12 08:24 /usr/bin/sh -> bash
$ cat /etc/redhat-release 
CentOS Stream release 8
$ 

@razvanstoica89
Copy link
Contributor Author

I think I managed to fix this bug #467 (comment)

@konstruktoid
Copy link
Collaborator

Looks good, but I'm not sure about those impact statements.
E.g. [INFO] 5.31 - You should ensure that no containers mount docker.sock as a volume. Impact: None. Mounting docker.sock is basically server ownership.

@razvanstoica89
Copy link
Contributor Author

razvanstoica89 commented Apr 14, 2021

These impact statements refer to the impact of the implementation of remedial measures.
Not to the impact that the lack of implementation of remedial measures has.

Keep in mind that in very few situations or use cases, you want to mount docker.sock even if it is definitely not recommended.
Example: Portainer.io

@@ -1151,7 +1151,7 @@ check_5_31() {
local id="5.31"
local desc="Ensure that the Docker socket is not mounted inside any containers (Scored)"
local remediation="You should ensure that no containers mount docker.sock as a volume."
local remediationImpact="None."
local remediationImpact="If you really, really have to do this, you should use user namespaces and always ensure that the images you run with this configuration are properly audited and that you trust them in order to avoid potential breaches."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"None." is fine, since it's the remediationImpact we're talking about, why not just change "Impact" to "Remedation Impact"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. It's better None. I'm rolling back my last change. Please excuse my mistake.

fi
globalRemediation="${globalRemediation}\n${bldblu}[INFO]${txtrst} ${id} - ${remediation}"
if [ -n "${remediationImpact}" ]; then
globalRemediation="${globalRemediation} Impact: ${remediationImpact}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to change this Impact to Remediation Impact as in the json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@konstruktoid Thanks for the feedback. I made this change.

@konstruktoid
Copy link
Collaborator

Looks good!
And thanks for this massive, massive contribution.

@konstruktoid konstruktoid merged commit 6a8fdcf into docker:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants