PORTS_PRIVSEP=Yes and doas install

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

PORTS_PRIVSEP=Yes and doas install

Lucas-2
Hello ports@,

Today I spent a fair amount of time trying to understand why, after
having set relevant bits of configuration in /etc/doas.conf and
/etc/mk.conf, `make` was asking me for passwords.

Setup
=====

/etc/doas.conf:
        permit persist lucas as root
        permit nopass keepenv lucas as _pbuild
        permit nopass keepenv lucas as _pfetch
        permit nopass lucas cmd /usr/bin/touch
        permit nopass setenv { TRUSTED_PKG_PATH TERM } \
            lucas cmd /usr/sbin/pkg_add
        permit nopass setenv { TERM } lucas cmd /usr/sbin/pkg_delete

/etc/mk.conf:
        DISTDIR=/var/ports/distfiles
        FETCH_PACKAGES=
        PACKAGE_REPOSITORY=/var/ports/packages
        PORTSDIR=/home/cvs/ports
        PORTS_PRIVSEP=Yes
        SUDO=doas
        WRKOBJDIR=/var/ports/pobj

Ports tree is OPENBSD_6_6 and not -current, as I want a build machine
to backport some updates, to ease testing of ports updates (as in
packaging net/prosody 0.11.5 for testing it in my server which runs
-stable)

Note that I use some "unusual" paths. Reason for this is the machine
not being originally purposed to build packages, so I picked parts of
the filesystems that had spare space.

Debugging / solution proposals
==============================

After asking `ps auxww`, turns out the guilty is

        doas install -d -o _pfetch -g 56 \
            /var/ports/packages/amd64/cache/

which gets called at line 2084 of ports/infrastructure/mk/bsd.port.mk,
in some dependency chain (tbh, bsd.port.mk is a beast and I tried my
best to follow stuff).

So, first option would be adding another recommended line for
/etc/doas.conf in bsd.port.mk(5), in the same vein of the lines already
present in there.

Now, I don't feel to comfortable suggesting to add

        permit nopass solene cmd install

to it. Instead, I'd prefer using full path, but that would require
changing that precise invocation of install to full path, or creating
a INSTALL_PROG variable, much like PKG_ADD and friends, defaulting to
/usr/bin/install. This will require, for consistency, changing all the
calls to install among all the makefiles to ${INSTALL_PROG}, which
seems quite invasive.

If we take a look at why that install gets called with doas, it happens
because PORTS_PRIVSEP is set to Yes, and so, the .if in line 133 of
ports/infrastructure/mk/pkgpath.mk sets _INSTALL_CACHE_REPO to escalate
privileges, in line 144. This call creates a directory owned by
${FETCH_USER}:${FETCH_USER}. This *could* be replace with

        _INSTALL_CACHE_REPO = ${_PFETCH} install -d \
            ${PACKAGE_REPOSITORY_MODE}

which worked in my case, but I guess it's done with ${SUDO} because
${FETCH_USER} could not have write permissions in the directory where
the package cache gets created.

Which takes to the final alternative: ${_INSTALL_CACHE_REPO} is called
in only one place, line 2084 of bsd.port.mk. It's called with
${_CACHE_REPO}, assigned *unconditionally* at line 961 as

        _CACHE_REPO = ${PACKAGE_REPOSITORY}/${MACHINE_ARCH}/cache/

As such, I'd suggest having a new user variable in mk.conf(5),
CACHE_REPOSITORY, and set

        _CACHE_REPO = ${CACHE_REPOSITORY}/${MACHINE_ARCH}/cache/

This way, we can replace _INSTALL_CACHE_REPO as suggested above and
require CACHE_REPOSITORY to be owned by ${FETCH_USER} if PORTS_PRIVSEP
is Yes. We can also make fix-permissions handle the creation too.

Comments? Opinions? IMO introducing CACHE_REPOSITORY is quite clean and
the least invasive approach. I can prepare a patch for that.

-Lucas

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Kurt Mosiejczuk-9
On Thu, Mar 26, 2020 at 12:12:51AM +0000, Lucas wrote:

> Today I spent a fair amount of time trying to understand why, after
> having set relevant bits of configuration in /etc/doas.conf and
> /etc/mk.conf, `make` was asking me for passwords.

Did you just try typing "make install"?

If you do "doas make install" then all your doas.conf lines mean nothing,
because you are doing the "make install" as root. So when it comes to
using doas in bsd.port.mk, you are root, so it wants root's password,
because you haven't set up anything special for root.

--Kurt

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Lucas-2
Kurt Mosiejczuk <[hidden email]> wrote:
> Did you just try typing "make install"?

Yes, that was all I ran. If you read the whole email, you will see the
doas install in the subject refers to /usr/bin/install and not a "typo"
of sorts for doas make install, which is, I assume, what you think I
did.

-Lucas

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Lucas-2
In reply to this post by Lucas-2
Here is patch for the proposed approach. It's two attachments actually,
one for src and one for ports. I think I didn't miss any documentation
spot and did a test build of just one package, but I didn't do a bulk
nor any other stress test.

You can also find attached the script output of said test build. make
update-plist failure seems to be related to a missing bit of
configuration I haven't been able to debug yet (PORTSDIR gets picked
up as /usr/ports instead of /home/cvs/ports).

-Lucas


patch-CACHE_REPOSITORY-src (4K) Download Attachment
patch-CACHE_REPOSITORY-ports (2K) Download Attachment
test-build.log (111K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Kurt Mosiejczuk-9
In reply to this post by Lucas-2
On Thu, Mar 26, 2020 at 03:14:18AM +0000, Lucas wrote:
> Kurt Mosiejczuk <[hidden email]> wrote:
> > Did you just try typing "make install"?

> Yes, that was all I ran. If you read the whole email, you will see
> the doas install in the subject refers to /usr/bin/install and not a
> "typo" of sorts for doas make install, which is, I assume, what you
> think I did.

I did read the whole email. You never explicitly said what command you
were running. There was a whole bunch of rambling information that made
it hard to pin down what problem you were having. I have come across the
mistake of folks saying "doas make install" and not understanding why
their doas.conf didn't work. I figured I'd mention it and save you any
further headache if that was the mistake you'd made.

--Kurt

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Solene Rapenne
On Thu, Mar 26, 2020 at 02:56:25AM -0400, Kurt Mosiejczuk wrote:

> On Thu, Mar 26, 2020 at 03:14:18AM +0000, Lucas wrote:
> > Kurt Mosiejczuk <[hidden email]> wrote:
> > > Did you just try typing "make install"?
>
> > Yes, that was all I ran. If you read the whole email, you will see
> > the doas install in the subject refers to /usr/bin/install and not a
> > "typo" of sorts for doas make install, which is, I assume, what you
> > think I did.
>
> I did read the whole email. You never explicitly said what command you
> were running. There was a whole bunch of rambling information that made
> it hard to pin down what problem you were having. I have come across the
> mistake of folks saying "doas make install" and not understanding why
> their doas.conf didn't work. I figured I'd mention it and save you any
> further headache if that was the mistake you'd made.
>
> --Kurt
>

I can replicate the issue, it only happens with FETCH_PACKAGES= because
_pfetch must own a directory under the package directory owned by
_pbuild, so it must be run as root.

make debug output
-----------------
doas -u _pbuild install -d  /usr/ports/packages/amd64/all/ /usr/ports/packages/amd64/tmp/
doas install -d -o _pfetch -g $(id -g _pfetch) /usr/ports/packages/amd64/cache/
doas ([hidden email]) password:

Lucas diff propose to make a new folder ${PORTSDIR}/pcache to solve
diff.

I guess Adding a line to doas.conf to allow install(1) as root should
prevent the error too.

I have no opinion about this but I wanted to point there is a real issue
(or lack of documentation if the chosen solution is adding more lines to
doas.conf)

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Stuart Henderson
On 2020/03/26 12:48, Solene Rapenne wrote:
> I guess Adding a line to doas.conf to allow install(1) as root should
> prevent the error too.

Allowing install as root without password is an even easier privilege
escalation than allowing pkg_add without password.

doas install -m 4755 -o root /bin/sh /bin/rootsh

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Marc Espie-2
In reply to this post by Lucas-2
On Thu, Mar 26, 2020 at 03:21:40AM +0000, Lucas wrote:

> Here is patch for the proposed approach. It's two attachments actually,
> one for src and one for ports. I think I didn't miss any documentation
> spot and did a test build of just one package, but I didn't do a bulk
> nor any other stress test.
>
> You can also find attached the script output of said test build. make
> update-plist failure seems to be related to a missing bit of
> configuration I haven't been able to debug yet (PORTSDIR gets picked
> up as /usr/ports instead of /home/cvs/ports).
>
> -Lucas

No, it's just a question of giving cache/ to the right user, there's no
need for a complete new set of variables.

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Lucas-2
Marc Espie <[hidden email]> wrote:
> No, it's just a question of giving cache/ to the right user, there's no
> need for a complete new set of variables.

Then I can only think of moving _CACHE_REPO under DISTDIR. I can
prepare a patch for that too, if it's the direction to take.

-Lucas

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Marc Espie-2
On Thu, Mar 26, 2020 at 01:37:30PM +0000, Lucas wrote:
> Marc Espie <[hidden email]> wrote:
> > No, it's just a question of giving cache/ to the right user, there's no
> > need for a complete new set of variables.
>
> Then I can only think of moving _CACHE_REPO under DISTDIR. I can
> prepare a patch for that too, if it's the direction to take.
>
> -Lucas
>
Why ? there's no problem having separate ownership of stuff under
packages

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Lucas-2
Marc Espie <[hidden email]> wrote:

> On Thu, Mar 26, 2020 at 01:37:30PM +0000, Lucas wrote:
> > Marc Espie <[hidden email]> wrote:
> > > No, it's just a question of giving cache/ to the right user, there's no
> > > need for a complete new set of variables.
> >
> > Then I can only think of moving _CACHE_REPO under DISTDIR. I can
> > prepare a patch for that too, if it's the direction to take.
> >
> > -Lucas
> >
> Why ? there's no problem having separate ownership of stuff under
> packages

I consider being asked a password a problem (we can't make unattended
builds if FETCH_PACKAGES isn't No) and I don't feel that adding

        permit nopass lucas cmd install

to doas.conf is the correct solution.

At the very least, it should be noted somewhere that PORTS_PRIVSEP=Yes
and FETCH_PACKAGES != No don't play well together and changes to
doas.conf have to be made in order for not being asked frequently for
passwords.

From my ignorance, I think assigning _CACHE_REPO under DISTDIR is
non-invassive enough. I have yet to check what kind of files go under
DISTDIR and if it would conflict having ${MACHINE_ARCHITECTURE}/cache
in there.

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Marc Espie-2
On Thu, Mar 26, 2020 at 02:17:04PM +0000, Lucas wrote:

> Marc Espie <[hidden email]> wrote:
> > On Thu, Mar 26, 2020 at 01:37:30PM +0000, Lucas wrote:
> > > Marc Espie <[hidden email]> wrote:
> > > > No, it's just a question of giving cache/ to the right user, there's no
> > > > need for a complete new set of variables.
> > >
> > > Then I can only think of moving _CACHE_REPO under DISTDIR. I can
> > > prepare a patch for that too, if it's the direction to take.
> > >
> > > -Lucas
> > >
> > Why ? there's no problem having separate ownership of stuff under
> > packages
>
> I consider being asked a password a problem (we can't make unattended
> builds if FETCH_PACKAGES isn't No) and I don't feel that adding
>
> permit nopass lucas cmd install
>
> to doas.conf is the correct solution.
>
> At the very least, it should be noted somewhere that PORTS_PRIVSEP=Yes
> and FETCH_PACKAGES != No don't play well together and changes to
> doas.conf have to be made in order for not being asked frequently for
> passwords.
>
> >From my ignorance, I think assigning _CACHE_REPO under DISTDIR is
> non-invassive enough. I have yet to check what kind of files go under
> DISTDIR and if it would conflict having ${MACHINE_ARCHITECTURE}/cache
> in there.
>

Look I'll have to try and reproduce your issue, but basically we create
lots of dirs with different ownership in "fix-permissions"
_CACHE_REPO is missing in there, and it's very likely it's all that's
actually needed.

Index: bsd.port.mk
===================================================================
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1526
diff -u -p -r1.1526 bsd.port.mk
--- bsd.port.mk 24 Mar 2020 17:33:43 -0000 1.1526
+++ bsd.port.mk 26 Mar 2020 14:37:42 -0000
@@ -2056,7 +2056,9 @@ fix-permissions:
  else \
  install -o ${FETCH_USER} -g $$f -d ${DISTDIR}; \
  fi
-.  for d in ${LOCKDIR} ${PACKAGE_REPOSITORY} ${PLIST_REPOSITORY} ${WRKOBJDIR}
+.  for d in ${LOCKDIR} ${PACKAGE_REPOSITORY} \
+ ${PACKAGE_REPOSITORY}/${MACHINE_ARCH} \
+ ${PLIST_REPOSITORY} ${WRKOBJDIR}
  @b=`id -gn ${BUILD_USER}`; \
  echo "give $d to ${BUILD_USER}:$$b"; \
  if test -d $d; then \
@@ -2065,6 +2067,13 @@ fix-permissions:
  install -o ${BUILD_USER} -g $$b -d $d; \
  fi
 .  endfor
+ @f=`id -gn ${FETCH_USER}`; \
+ echo "give ${_CACHE_REPO} to ${FETCH_USER}:$$f"; \
+ if test -d ${_CACHE_REPO}; then \
+ cd ${_CACHE_REPO} && chown -R ${FETCH_USER}:$$f .; \
+ else \
+ install -o ${FETCH_USER} -g $$f -d $${_CACHE_REPO}; \
+ fi
 .endif
 
 .for _S in ${MULTI_PACKAGES}

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Stuart Henderson
In reply to this post by Lucas-2
On 2020/03/26 14:17, Lucas wrote:
> I consider being asked a password a problem (we can't make unattended
> builds if FETCH_PACKAGES isn't No) and I don't feel that adding

If unattended builds is your goal then really DPB is the thing to use. Start
it as root and it *drops* privs as required.

Any setup where you start the build as an unprivileged user and then escalate
privs for certain things, either requires attendance to type passwords, or
requires that you open a priv escalation hole.

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Lucas-2
In reply to this post by Marc Espie-2
Marc Espie <[hidden email]> wrote:
> Look I'll have to try and reproduce your issue, but basically we create
> lots of dirs with different ownership in "fix-permissions"
> _CACHE_REPO is missing in there, and it's very likely it's all that's
> actually needed.

I didn't think of it, it's even better! FWIW, looks fine to me.

pkgpath.mk still needs the modifications though, as it will keep
running `doas install` otherwise. If we don't care for
PACKAGE_REPOSITORY_MODE (tbh, I found out about the *_MODE vars after
having to take a look at this), _INSTALL_CACHE_REPO can be replaced
to just `:`.

Thanks for taking the time to look at this.

-Lucas


Index: infrastructure/mk/pkgpath.mk
===================================================================
RCS file: /home/cvs/ports/infrastructure/mk/pkgpath.mk,v
retrieving revision 1.83
diff -u -p -u -p -r1.83 pkgpath.mk
--- infrastructure/mk/pkgpath.mk 30 May 2019 17:23:46 -0000 1.83
+++ infrastructure/mk/pkgpath.mk 26 Mar 2020 00:15:11 -0000
@@ -141,7 +141,7 @@ _PREDIR = |${_PBUILD} tee >/dev/null
 _PSUDO = ${SUDO}
 _UPDATE_PLIST_SETUP=FAKE_TREE_OWNER=${BUILD_USER} \
  PORTS_TREE_OWNER=$$(id -un) ${SUDO}
-_INSTALL_CACHE_REPO = ${SUDO} install -d -o ${FETCH_USER} -g $$(id -g ${FETCH_USER}) ${PACKAGE_REPOSITORY_MODE}
+_INSTALL_CACHE_REPO = ${_PFETCH} install -d ${PACKAGE_REPOSITORY_MODE}
 .else
 _PFETCH =
 _PBUILD =

> Index: bsd.port.mk
> ===================================================================
> RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
> retrieving revision 1.1526
> diff -u -p -r1.1526 bsd.port.mk
> --- bsd.port.mk 24 Mar 2020 17:33:43 -0000 1.1526
> +++ bsd.port.mk 26 Mar 2020 14:37:42 -0000
> @@ -2056,7 +2056,9 @@ fix-permissions:
>   else \
>   install -o ${FETCH_USER} -g $$f -d ${DISTDIR}; \
>   fi
> -.  for d in ${LOCKDIR} ${PACKAGE_REPOSITORY} ${PLIST_REPOSITORY} ${WRKOBJDIR}
> +.  for d in ${LOCKDIR} ${PACKAGE_REPOSITORY} \
> + ${PACKAGE_REPOSITORY}/${MACHINE_ARCH} \
> + ${PLIST_REPOSITORY} ${WRKOBJDIR}
>   @b=`id -gn ${BUILD_USER}`; \
>   echo "give $d to ${BUILD_USER}:$$b"; \
>   if test -d $d; then \
> @@ -2065,6 +2067,13 @@ fix-permissions:
>   install -o ${BUILD_USER} -g $$b -d $d; \
>   fi
>  .  endfor
> + @f=`id -gn ${FETCH_USER}`; \
> + echo "give ${_CACHE_REPO} to ${FETCH_USER}:$$f"; \
> + if test -d ${_CACHE_REPO}; then \
> + cd ${_CACHE_REPO} && chown -R ${FETCH_USER}:$$f .; \
> + else \
> + install -o ${FETCH_USER} -g $$f -d $${_CACHE_REPO}; \
> + fi
>  .endif
>  
>  .for _S in ${MULTI_PACKAGES}

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Marc Espie-2
On Thu, Mar 26, 2020 at 02:53:17PM +0000, Lucas wrote:

> Index: infrastructure/mk/pkgpath.mk
> ===================================================================
> RCS file: /home/cvs/ports/infrastructure/mk/pkgpath.mk,v
> retrieving revision 1.83
> diff -u -p -u -p -r1.83 pkgpath.mk
> --- infrastructure/mk/pkgpath.mk 30 May 2019 17:23:46 -0000 1.83
> +++ infrastructure/mk/pkgpath.mk 26 Mar 2020 00:15:11 -0000
> @@ -141,7 +141,7 @@ _PREDIR = |${_PBUILD} tee >/dev/null
>  _PSUDO = ${SUDO}
>  _UPDATE_PLIST_SETUP=FAKE_TREE_OWNER=${BUILD_USER} \
>   PORTS_TREE_OWNER=$$(id -un) ${SUDO}
> -_INSTALL_CACHE_REPO = ${SUDO} install -d -o ${FETCH_USER} -g $$(id -g ${FETCH_USER}) ${PACKAGE_REPOSITORY_MODE}
> +_INSTALL_CACHE_REPO = ${_PFETCH} install -d ${PACKAGE_REPOSITORY_MODE}
>  .else
>  _PFETCH =
>  _PBUILD =
>

Nah, this is more complicated.

This part will actually not work, because we are trying to create a directory
belonging to pfetch inside a directory belonging to pbuild.

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Marc Espie-2
In reply to this post by Lucas-2
On Thu, Mar 26, 2020 at 02:53:17PM +0000, Lucas wrote:

> Marc Espie <[hidden email]> wrote:
> > Look I'll have to try and reproduce your issue, but basically we create
> > lots of dirs with different ownership in "fix-permissions"
> > _CACHE_REPO is missing in there, and it's very likely it's all that's
> > actually needed.
>
> I didn't think of it, it's even better! FWIW, looks fine to me.
>
> pkgpath.mk still needs the modifications though, as it will keep
> running `doas install` otherwise. If we don't care for
> PACKAGE_REPOSITORY_MODE (tbh, I found out about the *_MODE vars after
> having to take a look at this), _INSTALL_CACHE_REPO can be replaced
> to just `:`.
>
> Thanks for taking the time to look at this.
>
> -Lucas

I've committed a proper fix, acknowledging that INSTALL_CACHE_REPO
may not work without manual intervention.

Reply | Threaded
Open this post in threaded view
|

Re: PORTS_PRIVSEP=Yes and doas install

Lucas-2
Marc Espie <[hidden email]> wrote:
> I've committed a proper fix, acknowledging that INSTALL_CACHE_REPO
> may not work without manual intervention.

According to cvsweb.openbsd.org and obsdacvs.cs.toronto.edu, last
update to pkgpath.mk and bsd.port.mk are from about an hour ago, with
diffs similar to the ones we discussed. The log entry got screweed (it
reads "/tmp/cvseSzodp"), and you missed adding ${_CACHE_REPO} to
PORTS_PRIVSEP'd _INSTALL_CACHE_REPO.

Why running : if install is successful, and not just failing if it
can't create the directory?

Is -o and -g still needed, given it's running as FETCH_USER already?

-Lucas


Index: pkgpath.mk
===================================================================
RCS file: /home/cvs/ports/infrastructure/mk/pkgpath.mk,v
retrieving revision 1.84
diff -u -p -u -p -r1.84 pkgpath.mk
--- pkgpath.mk 29 Mar 2020 12:11:45 -0000 1.84
+++ pkgpath.mk 29 Mar 2020 13:16:50 -0000
@@ -142,9 +142,7 @@ _PSUDO = ${SUDO}
 _UPDATE_PLIST_SETUP=FAKE_TREE_OWNER=${BUILD_USER} \
  PORTS_TREE_OWNER=$$(id -un) ${SUDO}
 _INSTALL_CACHE_REPO = \
- if ${_PFETCH} install -d -o ${FETCH_USER} -g $$(id -g ${FETCH_USER}) ${PACKAGE_REPOSITORY_MODE}; then \
- :; \
- else \
+ if ! ${_PFETCH} install -d ${PACKAGE_REPOSITORY_MODE} ${_CACHE_REPO}; then \
  echo >&2 "Can't create ${_CACHE_REPO}; run 'make fix-permissions' as root"; \
  exit 1; \
  fi