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

fix(protoc): Run protoc container as invoking user #115

Merged
merged 5 commits into from
Jul 9, 2020
Merged

fix(protoc): Run protoc container as invoking user #115

merged 5 commits into from
Jul 9, 2020

Conversation

ezimanyi
Copy link
Contributor

@ezimanyi ezimanyi commented Jul 8, 2020

  • style(proto): Add newlines in docker command

    This will make it easier to see what changed when adding/removing arguments to the command.

  • refactor(proto): Use mount instead of volume command

    The mount command is newer and recommended by Docker. It also allows us to mount a volume readonly, which we want to do for the proto directory (which is input only).

  • refactor(protoc): Use better directories in container

    It's customary for mounted filesystems to be in /mnt; let's mount our files there instead of directly in the root of the filesystem.

    Rather than have a top-level staging directory, make this directory in /tmp. Also create it as part of the genproto script so it's automatically owned by whichever user is running the script.

  • fix(protoc): Fix file permissions in kleat-protobuilder

    This commit updates the intermediate containers used to fetch protoc and its dependencies to no longer run as root.

    The first change is to update the 'curl' container to run as a non-root user 'curl' to set the work directory to this user's home directory.

    After downloading and unzipping the desired files, we run chmod to set the desired permissions, which in many cases are not set to what we want in the tar/zip archive. We want 755 for directories and executable files and 644 for non-executable files, so that only the owner can modify but anyone can use the files when running protoc.

    When copying the files to the kleat-protobuilder container, explicitly have them owned by root (leaving the permissions as what we set them to after downloading). This matches the way things are normally installed on linux, where root owns the files and is the only user with permission to write, but any user can read/execute the files.

    In the case of genproto, we'll explicitly follow up with a chmod to ensure it has mode 755 as we can't necessarily control what the user has on their system.

    As of this commit, we've fixed the permissions so that it is now possible to run the kleat-protobuilder as a non-root user by passing the -u flag to docker, but we're not actually doing that yet, which will follow in an upcoming commit.

  • fix(protoc): Run protoc container as invoking user

    This commit adds the -u flag to the docker command running the protocol buffer compilation, so that the container runs with the same uid/gid as the invoking user. This will cause any output files to have this uid/gid (instead of being owned by root as before).

    I've also created a user 'protoc' in the container and set the user to that user. This is really just a fallback so we're falling back to a non-root user; this will always be overridden by the --user flag that we've added to the docker run command.

ezimanyi added 5 commits July 8, 2020 21:54
This will make it easier to see what changed when adding/removing
arguments to the command.
The mount command is newer and recommended by Docker. It also
allows us to mount a volume readonly, which we want to do for the
proto directory (which is input only).
It's customary for mounted filesystems to be in /mnt; let's mount
our files there instead of directly in the root of the filesystem.

Rather than have a top-level staging directory, make this directory
in /tmp. Also create it as part of the genproto script so it's
automatically owned by whichever user is running the script.
This commit updates the intermediate containers used to fetch protoc
and its dependencies to no longer run as root.

The first change is to update the 'curl' container to run as a
non-root user 'curl' to set the work directory to this user's
home directory.

After downloading and unzipping the desired files, we run chmod
to set the desired permissions, which in many cases are not set
to what we want in the tar/zip archive. We want 755 for directories
and executable files and 644 for non-executable files, so that only
the owner can modify but anyone can use the files when running
protoc.

When copying the files to the kleat-protobuilder container,
explicitly have them owned by root (leaving the permissions as what
we set them to after downloading). This matches the way things are
normally installed on linux, where root owns the files and is the
only user with permission to write, but any user can read/execute
the files.

In the case of genproto, we'll explicitly follow up with a chmod
to ensure it has mode 755 as we can't necessarily control what
the user has on their system.

As of this commit, we've fixed the permissions so that it is now
possible to run the kleat-protobuilder as a non-root user by
passing the -u flag to docker, but we're not actually doing that
yet, which will follow in an upcoming commit.
This commit adds the -u flag to the docker command running the
protocol buffer compilation, so that the container runs with the
same uid/gid as the invoking user. This will cause any output files
to have this uid/gid (instead of being owned by root as before).

I've also created a user 'protoc' in the container and set the
user to that user. This is really just a fallback so we're falling
back to a non-root user; this will always be overridden by the
--user flag that we've added to the docker run command.
@ezimanyi ezimanyi requested a review from maggieneterval July 8, 2020 22:00
@ezimanyi
Copy link
Contributor Author

ezimanyi commented Jul 8, 2020

It looks like MacOs does some translation, so that even when the container is running as root, the files it writes to mounted volumes are still owned by the user who invoked docker, which is why I didn't notice this before. I ran make proto from my Linux machine recently and noticed that all the files were owned by root which was annoying as then I couldn't overwrite them without sudo.

Copy link
Contributor

@maggieneterval maggieneterval left a comment

Choose a reason for hiding this comment

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

👨‍💻 ⚒️ 💻 🔐 🌳 ⬇️ 🚫 ✅ 💯 🤯

@maggieneterval maggieneterval added the ready to merge Reviewed and ready for merge label Jul 9, 2020
@mergify mergify bot merged commit c94067a into spinnaker:master Jul 9, 2020
@mergify mergify bot added the auto merged label Jul 9, 2020
@ezimanyi ezimanyi deleted the fix-protoc-permissions branch July 9, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged ready to merge Reviewed and ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants