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

Run the agent process as non-root user #2

Closed
wants to merge 1 commit into from

Conversation

@akkie
Copy link

commented Aug 9, 2019

In kubernetes clusters is common to enable pod security policies, that disallow containers that run the main process as root, because hackers can gain root access to the Docker host by hacking the application running inside the container.

With this PR the blackfire agent runs as an unprivileged user.

Dockerfile Outdated
COPY blackfire blackfire-agent /usr/local/bin/

# Don't run as root
RUN adduser -D blackfire

This comment has been minimized.

Copy link
@lyrixx

lyrixx Aug 9, 2019

I think the use could be a system user, and this use does not need to have a $HOME folder.
so:

adduser --system -D -H  blackfire

This comment has been minimized.

Copy link
@akkie

akkie Aug 13, 2019

Author

I've opened a question on SO regarding this issue. It was put as on hold. So I'm not sure I'll get an answer.

If I should answer it, I would say that a UID in the container should't match a UID on the host system. The issue is that the UID allocation on the host system isn't really predictable because many systems use different ranges for UID allocations. It seems that on most modern Linux system, users will be randomly allocated in the range between 100 - 999 and normal users will be allocated in an increasing order, on most modern Linux system in the range from 1000 - 60000. So wouldn't it make sense to choose a UID of a normal user in the range between 5000 - 6000 or even higher? Choosing a system user would increase the risk to match an existing system user on the host system. But if I choose a very high UID in the normal user range, the change to match a user on a host system would be very low.

I think there is no general guidance. So let me know what you prefer and I will update the PR.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Aug 13, 2019

I tested with postgres, redis, rabbitmq, mysql, they all use a system user with ID 999

Dockerfile Outdated

# Don't run as root
RUN adduser -D blackfire
RUN chown -R blackfire:blackfire /usr/local/bin

This comment has been minimized.

Copy link
@lyrixx

lyrixx Aug 9, 2019

Why did you change the whole directory? There is no need for that
(Same comment for the next line)

This comment has been minimized.

Copy link
@akkie

akkie Aug 13, 2019

Author

From https://unix.stackexchange.com/questions/8656/usr-bin-vs-usr-local-bin-on-linux

/usr/local/bin is for normal user programs not managed by the distribution package manager, e.g. locally compiled packages. You should not install them into /usr/bin because future distribution upgrades may modify or delete them without warning.

For the sake of consistency, I thought I'll follow this advice. But the same as in my other answer. Let me know what you prefer and I will update the PR.

This comment has been minimized.

Copy link
@iamluc

iamluc Aug 14, 2019

I think chown and chmod commands are not needed at all.

This comment has been minimized.

Copy link
@akkie

akkie Aug 14, 2019

Author

I thought it would be good that the user who runs the main process owns the files and that the permissions are explicitly set, even if the executable bit is set in git.

But, your'e right. It's not really needed. I can remove it if desired.

This comment has been minimized.

Copy link
@iamluc

iamluc Aug 14, 2019

I'm NOT a security expert, but it think a security breach in the agent would allow the hacker to change the binary.

Also, I checked with Debian and Alpine, all binaries are owned by root (with some rare exceptions).

IMHO, we should do the same.

This comment has been minimized.

Copy link
@akkie

akkie Aug 14, 2019

Author

Alright, done 👍

@akkie

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

I've updated the PR to run the agent as system user with gid 999 and uid 999

Dockerfile Outdated
COPY blackfire blackfire-agent /usr/local/bin/

# Don't run as root
RUN addgroup -S -g 999 blackfire && adduser -S -G blackfire -u 999 blackfire

This comment has been minimized.

Copy link
@lyrixx

lyrixx Aug 13, 2019

This does not work, the group 999 already exists on alpine

This comment has been minimized.

Copy link
@akkie

akkie Aug 13, 2019

Author

OK, I've removed it

Dockerfile Outdated
@@ -11,7 +11,7 @@ RUN apk add --no-cache curl
COPY blackfire blackfire-agent /usr/local/bin/

# Don't run as root
RUN addgroup -S -g 999 blackfire && adduser -S -G blackfire -u 999 blackfire
RUN addgroup -S blackfire && adduser -S -G blackfire -u 999 blackfire

This comment has been minimized.

Copy link
@lyrixx

lyrixx Aug 13, 2019

You forgot the -H option to not create the /home/blackfire dir

This comment has been minimized.

Copy link
@akkie

akkie Aug 13, 2019

Author

Sorry, fixed

@akkie

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

Is the PR now OK? Let me know if there's anything else to change!

@iamluc

iamluc approved these changes Aug 14, 2019

Copy link

left a comment

💎
Thanks a lot.

Before merging, could you squash the commits?

Run the agent process as non-root user
In kubernetes clusters is common to enable pod security policies, that disallow containers that run the main process as root, because hackers can gain root access to the Docker host by hacking the application running inside the container.

With this PR the blackfire agent runs as an unprivileged user.

@akkie akkie force-pushed the setusoft:master branch from f817c8a to 3b1d16f Aug 14, 2019

@akkie

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

Sure, done. You should consider to enable "Allow squash merging" and "Allow rebase merging" in the repo settings. So you could squash the commits directly in GitHub.

@iamluc

This comment has been minimized.

Copy link

commented Aug 14, 2019

@akkie in fact, I cannot merge this PR into this repository. Instead I must copy your changes in an internal script of Blackfire, so you will loose the ownership of the patch.

I hope this is OK for you?
Sorry about that.

@akkie

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

Sure, no problem 👍

@lyrixx

lyrixx approved these changes Aug 14, 2019

@iamluc

This comment has been minimized.

Copy link

commented Aug 14, 2019

The changes will be available with the next stable release.

Thanks @akkie (and @lyrixx for the review ;-) )

@iamluc iamluc closed this Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.