Skip to content

Add another alternative. #57

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

Closed
wants to merge 1 commit into from
Closed

Add another alternative. #57

wants to merge 1 commit into from

Conversation

songdongsheng
Copy link

No description provided.

Copy link
Owner

@tianon tianon left a comment

Choose a reason for hiding this comment

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

To say I'm confused here would be putting it pretty mildly. 😕


### `songdongsheng/su-exec`

[`songdongsheng/su-exec`](https://github.com/songdongsheng/su-exec) is another very minimal re-write of `gosu` in C, making for a much smaller binary, and safer than the [`ncopa/su-exec`](https://github.com/ncopa/su-exec).
Copy link
Owner

Choose a reason for hiding this comment

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

You're definitely going to have to qualify this a bit; what do you mean by "safer"?

Have you run our existing test suite against it to verify compatibility?

Did you perhaps consider instead collaborating with ncopa on making his implementation better before creating your own in the same language with the same apparent set of goals? (He's a nice guy, and I'm sure would be open to improvement if there's something concrete.)

As mentioned in `INSTALL.md`, [`su-exec`](https://github.com/ncopa/su-exec) is a very minimal re-write of `gosu` in C, making for a much smaller binary, and is available in the `main` Alpine package repository.
As mentioned in `INSTALL.md`, [`ncopa/su-exec`](https://github.com/ncopa/su-exec) is a very minimal re-write of `gosu` in C, making for a much smaller binary, and is available in the `main` Alpine package repository.

The ncopa's su-exec have critical bugs (or features), it will use root permission silently if invalid user-spec given !
Copy link
Owner

Choose a reason for hiding this comment

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

This line leads me to believe there's been a grave misunderstanding of the intended purpose / use case for gosu and su-exec.

The entire point of this project is for a Docker image to be able to start as root, and then use gosu to become non-root (to "step down", as it were). The way it does this is by using Docker's own user:group parsing code for Docker's --user flag (which comes from runc these days), and this is where the "empty string is root" edge case comes from.

Copy link
Author

Choose a reason for hiding this comment

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

I'm agree "empty string is root", but "invalid string is root" is not acceptable. It will swallow typo or error configuration.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, great idea; let's add some false negatives to our test suite. FWIW, gosu has sane behavior here:

$ ./gosu-amd64 0day true
error: failed switching to "0day": unable to find user 0day: no matching entries in passwd file
$ echo $?
1

Also, this is probably a simple fix to su-exec too (although it has been a bit since I looked at the code over there).

Copy link
Owner

Choose a reason for hiding this comment

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

#60 👍

Copy link
Owner

Choose a reason for hiding this comment

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

@tianon
Copy link
Owner

tianon commented Aug 21, 2019

Closing in favor of ncopa/su-exec#26.

@tianon tianon closed this Aug 21, 2019
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