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

Allow building sail to run PHP as root #677

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

vmsh0
Copy link
Contributor

@vmsh0 vmsh0 commented Mar 6, 2024

Hi,

Today, I tried running Sail in a rootless Podman container, and the experience was less-than-ideal. You might better recognize the class of issues as being similar to Docker Desktop - this would be precisely right: it's the same all over again.

The reason boils down to the follow: the image, as it is laid out currently, has its entrypoint running as root, and its main functionality (i.e. php) running as the sail user. This means that root inside the container is the host user I'm using to run the container, and sail is some random UID.

Some relevant supporting documentation and references to previous issues:

The proposed proof-of-concept patch simply allows setting the $SUPERVISOR_PHP_USER env variable to run PHP in the container as root. Users should be instructed about this by the "getting started" documentation, to avoid spending an evening figuring it out (or not) like I just did.

In my opinion this is the only valid long-term solution for rootless containers, as:

  • There is a lot of voodoo around the web and even in some of the issues linked above. The bottom line is that rootless unprivileged users (like sail in the sail container) have an effective ("host") UID that will not in any case be able to access /var/www/html (i.e. the user's project files), regardless of the value of --userns, if the files themselves are not chowned to this unprivileged by either Podman (U flag for bind mounts) or something running on the host. This is because root in the container is mapped to the user launching the container, and all other uids in the container are mapped to random junk host uids (usually >100000). It's simple to test this: open a root shell in a Sail container, and run touch /var/www/html/example && chown sail:sail /var/www/html/example; go on that directory outside the container, the UID will be some junk number. You can also run cat /proc/self/uid_map inside the container to see the mapping
  • For that exact reason, bind mounts are only accessible by root inside the containers (unless the files in the bind mounts have world r/w permissions of course)
  • It follows from this that PHP in Sail rootless containers should run as root

This will effectively make PHP have the permissions and capabilities of the launching user, which imho is the standard expectation for a dev environment. Furthermore, in a world where people don't just go around launching containers as root, this would also be a sensible default setting, but since the real world is the insurmountable single point of truth I won't advocate for that.

So, to recap:

  • By default, rootless containers map root in the container to the user running the container (meets expectations: Laravel runs as the user who launches the command)
  • Root inside a rootless container is NOT root on the host, even if it escapes the container (secure)
  • This patch adds support to build the Sail container to run rootless
  • If the idea accepted, the patch should, at a minimum, be should be supplemented by adequate documentation
  • If you want to pursue this but you feel like the patch is not adequate, please feel free to request any changes that would make the patch more adherent to your coding customs; I will be glad to oblige

This commit is a proof-of-concept to kickstart the PR and should not be
merged as-is.
@taylorotwell taylorotwell merged commit cd032b0 into laravel:1.x Mar 7, 2024
5 checks passed
@muriloloffi
Copy link

Well done and thank you @vmsh0. Your explanation and PoC were helpful to me.

tisnamuliarta referenced this pull request in tisnamuliarta/laravel-shadcn Mar 14, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [laravel/sail](https://togithub.com/laravel/sail) | `1.28.2` ->
`1.29.0` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/laravel%2fsail/1.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/laravel%2fsail/1.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/laravel%2fsail/1.28.2/1.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/laravel%2fsail/1.28.2/1.29.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>laravel/sail (laravel/sail)</summary>

###
[`v1.29.0`](https://togithub.com/laravel/sail/blob/HEAD/CHANGELOG.md#v1290---2024-03-08)

[Compare
Source](https://togithub.com/laravel/sail/compare/v1.28.2...v1.29.0)

- Allow building sail to run PHP as root by
[@&#8203;vmsh0](https://togithub.com/vmsh0) in
[https://github.com/laravel/sail/pull/677](https://togithub.com/laravel/sail/pull/677)
- Update MAILER config to use mailpit on L11 by
[@&#8203;SamuelMwangiW](https://togithub.com/SamuelMwangiW) in
[https://github.com/laravel/sail/pull/678](https://togithub.com/laravel/sail/pull/678)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/tisnamuliarta/laravel-shadcn).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMzguMSIsInVwZGF0ZWRJblZlciI6IjM3LjIzOC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
@vmsh0
Copy link
Contributor Author

vmsh0 commented Mar 14, 2024

Thank you for merging this, I'm glad to see it's useful to people.

Please, help me with the next steps to improve this:

  • Right now this functionality is pretty hidden. I would say that a reference should be added to the various "getting started" paths of Laravel suggesting the use of Laravel Sail. Do you agree?
  • Should this be added to any other documentation pages?
  • Is there anything else that should be built upon this PR to ensure the best possible user experience and maintainability?

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.

3 participants