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

Split PR #12 into separate commits #14

Merged
merged 8 commits into from May 30, 2015
Merged

Conversation

konstruktoid
Copy link
Collaborator

PR #12 split into seperate commits.
Replacement for earlier requests which was one PR per commit.
Commits individually signed.

@diogomonica, do you want me to clean up this PR mess I've made by closing my previous single-change PRs?

On the millionth PR I might finally figure out this GitHub voodoo...

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@diogomonica
Copy link
Contributor

@konstruktoid looks great. Thank you for the PR.

@diogomonica diogomonica mentioned this pull request May 30, 2015
@@ -19,6 +19,7 @@ dir_name=`dirname ${this_path}` ## Dir where this file is
myname=`basename ${this_path}` ## file name of this script.
logger="${myname}.log"

export PATH=/bin:/sbin:/usr/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

This line makes this not work on my mac. The reason is that command -v docker now files since my docker binary lives in /usr/local/bin/docker

➜ sh docker-bench-security.sh
docker command not found.

Why do we need this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A simple way to reduce risk of the script misbehaving. Some sort of response to Issue #2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it removes the ability of running it on my laptop, which is obviously a big no-no ;)

How set are you on this exact directory list? Can we add /usr/local/bin to it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mac users you. ;)

Absolutely, the PATH change is just there to add some sort of basic limitation. As /usr/local/bin is required, it should be added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, lets add it and I'll merge it!

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid
Copy link
Collaborator Author

Signed-off-by: Thomas Sjögren <konstruktoid@users.noreply.github.com>
@konstruktoid
Copy link
Collaborator Author

No worries, i'm just glad to help.
4fcac56

diogomonica added a commit that referenced this pull request May 30, 2015
Split PR #12 into separate commits
@diogomonica diogomonica merged commit 54202b3 into docker:master May 30, 2015
@konstruktoid konstruktoid deleted the split_pr branch May 30, 2015 23:45
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.

None yet

2 participants