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

installer: Don't require userns. Accept kernel 3.10. #2656

Merged
merged 4 commits into from
Oct 27, 2016

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Oct 14, 2016

Depends on #2644.

We've decided to delay releasing the installer changes until we've been able to test the privileged (non-userns) sandbox mode more thoroughly, because once we update the installer, users of Debian and Ubuntu will probably start using the privileged sandbox by default.

@kentonv
Copy link
Member Author

kentonv commented Oct 14, 2016

Looks like the mail test is currently failing due to the Roundcube bug where it wants the UID to be 1000.

@paulproteus
Copy link
Collaborator

So far, my feedback is:

Per our discussion today, I believe this needs to try to use the userns sandbox if:

  • You are installing in -d mode ("defaults" for "development")
  • You have /proc/sys mounted read-write
  • The system (via looking in /proc/sys) shows that there exists a sysctl for enabling userns

The one thing it should do, in that case, is to create the sysctl.d file (...or modify sysctl.conf if you don't have a sysctl.d? but you will have one in the one case that we care about, which is vagrant-spk using this, so we could just go for the cleaner sysctl.d approach).

@kentonv is that something you'll do as a patch to this, or is that my job? :)

It does mean we still can remove the x86-64 binary in install.sh. :)

Also I should presumably add an installer-test for this case, where -d prefers the userns sandbox.

This is intended specifically to enable user namespaces in vagrant-spk, which we want because user namespaces let us randomize UIDs in the development environment which is important for detecting bugs in the app around hardcoded UID assumptions.
@kentonv
Copy link
Member Author

kentonv commented Oct 18, 2016

@paulproteus OK, I added code to enable the userns sysctl specifically when -d is used.

fi
# Also make sure it is re-enabled on boot. If sysctl.d exists, we drop our own config in there.
# Otherwise we edit sysctl.conf, but that's less polite.
local SYSCTL_FILENAME="/etc/sysctl.conf"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Arguably feel free to bail on enabling the sysctl if /etc/sysctl.d doesn't exist. Not a show-stopper, just an opportunity for slightly fewer lines of code. vagrant-spk will have /etc/sysctl.d.

Copy link
Collaborator

@paulproteus paulproteus left a comment

Choose a reason for hiding this comment

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

I would like to see install.sh -d succeed in a Docker environment, rather than result in install.sh aborting. Let me know what you think.

return
fi

# Enable it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If /proc/sys is read-only, we should probably avoid setting the flag. It's read-only within Docker.

We have the following code currently within install.sh:

  # The /proc/sys issue applies to Docker containers and probably other sorts of containers, too.
  if egrep -q '/proc/sys\s+proc\s+ro' /proc/mounts || [ ! -e /etc/sysctl.conf ] ; then

Presumably we should use the same egrep test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would test -w work instead of grepping mounts?


# Enable it.
echo "NOTE: Enabling unprivileged user namespaces because you passed -d."
sysctl -wq kernel.unprivileged_userns_clone=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm concerned that if this fails, we have no code to handle the failure. The current install.sh does things this way:

    sysctl -w kernel.unprivileged_userns_clone=1 >/dev/null \
      || fail "E_SYSCTL_WRITING_FAILED" "'sysctl -w" \
              "kernel.unprivileged_userns_clone=1' failed. If you are inside docker, please run the" \
              "command manually inside your host and update /etc/sysctl.conf."

If the sysctl command fails, arguably we want to abort this function and continue with the install.

Same with the writing to $SYSCTL_FILENAME.

Curious if you agree.

@kentonv
Copy link
Member Author

kentonv commented Oct 20, 2016

I've added a commit to handle when sysctl -w fails, and also when writing to sysctl.d fails.

# Already enabled.
return
fi

# Enable it.
echo "NOTE: Enabling unprivileged user namespaces because you passed -d."
sysctl -wq kernel.unprivileged_userns_clone=1
if ! sysctl -wq kernel.unprivileged_userns_clone=1; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the user gain any benefit from this message? My hunch is no. It seems OK to allow the install to proceed without any warnings.

Doubly so since https://docs.sandstorm.io/en/latest/install/#option-6-using-sandstorm-within-docker uses install.sh -d

Perhaps the Docker instructions should contain something else, so that those people don't get a warning that doesn't really affect them? But for now I think the warning isn't adding value if you're running Sandstorm as root anyway. Curious what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason I assumed Docker installs required userns, but in retrospect that doesn't make sense. OK, removed the warning.

@paulproteus
Copy link
Collaborator

Status update: Looks good to me; In 24 hours, I'll merge then ask for a install.sh release.

Let's wait the 24 hours so the new Roundcube update can flow to people's servers via auto-updates, then release this at/around 5 PM Pacific tomorrow.

I've tested everything I want to test by then and updated the docs with a PR #2694 and updated the website with a PR sandstorm-io/sandstorm-website#253, which I can merge+deploy after we do the install.sh release.

"support for unprivileged user namespaces (CONFIG_USER_NS=y), or something else is" \
"preventing creation of user namespaces. This feature is critical for sandboxing." \
"Read more at: " \
"https://docs.sandstorm.io/en/latest/administering/faq/#enabling-user-namespaces"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Notably: In #2694 I plan to remove this section from the docs. Happily, install.sh is also about to stop referring to it.

This does mean that old copies of install.sh will link here. I keep wondering if we should avoid breaking that link. But I guess people will be OK.

@paulproteus
Copy link
Collaborator

This change does mean that there is now no way to do a headless install on a Debian system that doesn't enable this sysctl; furthermore, we now don't ask if that's OK up-front. In https://docs.sandstorm.io/en/latest/administering/install-script/ you'll see that using -d is part of our suggestions for headless installs.

@kentonv if that's OK with you, it's OK with me. It's a little suboptimal but there's so complexity in the requirements that I can live with it.

If it's not OK with you, one thing we could do is add a OVERRIDE_TRY_TO_ENABLE_USERNS=no environment variable which would become one of the documented overrides at https://docs.sandstorm.io/en/latest/administering/install-script/ .

A different option is we could "just" warn people as part of the "Are you sure?" printout, like we used to.

Now it's my turn to be apologetic about not catching this in earlier review!

I'll adjust docs to accommodate the current behavior, and you can see what I mean.

@kentonv
Copy link
Member Author

kentonv commented Oct 25, 2016

@paulproteus I'm not too worried about that, but I think the solution if we feel it's needed is to introduce a new flag for unattended non-dev installs. I don't feel the need to do it in this PR, in any case.

@paulproteus
Copy link
Collaborator

Works for me for now.

@paulproteus paulproteus merged commit 9f3b6ca into master Oct 27, 2016
@paulproteus
Copy link
Collaborator

@kentonv once this is live, I'll click merge on:

@paulproteus paulproteus deleted the no-userns-installer branch October 27, 2016 22:48
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