In D52308#1205084, @emaste wrote:

Something like the patch below (applied to each virtual_oss subdir) works. It's gross, but we're already doing this in e.g. lib/googletest/gmock/Makefile

OBE

It does make me wonder about all of the freebsd11_* bits in the same file that we use for pre-ino64 stuff, but my main concern is just fixing initgroups(3) for stable/15... we can always haggle on any other details later, if we need to.

This fixes the AX210 in my frame.work that previously made it through association and DHCP, then stopped passing traffic. Thanks!

Yeah, I'm happy with that, thanks. I recognize this is maybe not ideal for more flexible system construction (piece together your own based on a set and removing some), but IMO we should err on the side of caution and consider whether we can do something to enable better behavior with the final form of pkg groups.

In D52562#1200178, @ivy wrote:

In D52562#1200173, @kevans wrote:

I think my complaint is that, iirc, all of the metapackage dependencies are marked as automatic. As soon as they break the set as you suggest, the remainder would be subject to autoremove

right, but if you run pkg remove FreeBSD-set-devel followed by pkg autoremove, don't you expect all of clang etc. to be removed?

if the user does this by accident then runs pkg autoremove, they will see it's trying to remove FreeBSD-clang or whatever and can abort and rectify the situation.

I think my complaint is that, iirc, all of the metapackage dependencies are marked as automatic. As soon as they break the set as you suggest, the remainder would be subject to autoremove

Yeah, this seems fine to me. I did wonder for a second if the /rescue variants of shells do or should assume setugid behavior and avoid loading dotfiles in case something in the profile or other bits finds a way to make the shell unusable, but I didn't really convince myself.

Account for origin not existing, use an empty string; add tests

In D52498#1199266, @tamelingdaniel_gmail.com wrote:

@kevans can you have a look at your patch in bug 266496? (link)
I think nvlist_lookup_string can also return non-zero if one uses "origin" as key and the boot environment doesn't have an origin data set.

Whoops, good shout.

Thanks!

Can we preemptively add /src.conf to .gitignore if we do this?

In D52402#1196665, @jrm wrote:

Oh, sorry. One can't add screen.font=32x64 to /boot/loader.conf to get the Spleen fonts? I really can't tell much of a difference between the fonts on my system.

I would, however, hassle you a little bit over a commit message nit. One might look and notice that fortune-mod-bible has a dance to try and be compatible with different locations of strfile, but a little bit of archaeology reveals that @cperciva killed off the need for that back in 2015 (rG11d9aa670723f508821f2bf6980a555360783a80), so there's no remaining version of FreeBSD that will not have strfile in a stock configuration (and we don't really account for special pkgbase configurations today in ports, and maybe we won't tomorrow, either). I would proactively note in the commit message that the strfile dance found in other fortune data ports isn't necessary today to preempt concerns there.

In D52350#1195799, @ziaee wrote:

Attempt to fix everything except constructing the plist in the Makefile.
The Porters Handbook does say it's better to have a pkg-plist so people
can use grep on it.

However, that's an excuse because I couldn't figure it out. I tried:
PLIST_FILES= $MODS:S,^,share/games/fortune/,}.dat (plus the same w/o
appending .dat) based off what I read in archivers/stuffit/Makefile,
but then pkg inf -l only shows the last .dat file?

Also: needs an entry in misc/Makefile to hook it up, please include here as well

In D52282#1195513, @olce wrote:

TL;DR: Please jump to "Finally going back to the dilemma" below.

In D52282#1195248, @kevans wrote:

This isn't really a fun position to be in.

Yes, that's exactly why I am hesitating, and I'll develop a bit more with some findings.

In D52282#1195248, @kevans wrote:

• On the other hand, we're conditioning sysadmins to as noted, which is a somewhat contradictory stance but probably also one that we've held for many years.

Yes, as detailed above: The text in setgroups(2), the fact that pw(8) and adduser(8) do not by default add the effective GID into the groups database, the fact that login(1) and su(1) do not either at runtime. All these point towards a practice where the effective GID has never been added into the supplementary groups automatically in FreeBSD.

Finally going back to the dilemma: Should initgroups(3) include the effective GID automatically in the supplementary groups?

• For
  • Full compatibility with the following open source systems: Linux+glibc, NetBSD, OpenBSD, illumos.
  • As the initial effective GID always stays in the supplementary groups, it can reliably be used as a tag to authorize/deny accesses regardless of effective GID change.
  • No need for the administrator to manually insert each user in its group in the groups database, risking discrepancies between users.
• Against
  • Breaks with FreeBSD's existing practice from the start.
  • Probably incompatible with Darwin/MacOS.
  • Less fine-grained control on the supplementary groups.
  • We could instead change pw(1) and adduser(1) to add by default some user in its group in the groups database, gaining some of the benefits of the other approach while retaining finer-grained control.
  • setgid executables, or privileged processes executing setgid()/setegid() only do not get rid of the initial user's group (it stays in the supplementary set). Thus, the resulting processes can still access files belonging to that user's group.

If it was not for compatibility, I would have chosen "Against" without hesitation as it leaves more leeway to administrators and we can alleviate inconsistencies issues between users by changing pw(1) and adduser(1). I'm personally not (yet?) convinced that full initgroups(3) compatibility is really compelling.

So, I'm at the moment more inclined to stay with "Against". In practice, I would modify initgroups() to just call setgroups() but omitting the first group returned by getgrouplist() (the one explicitly passed to initgroups()). A possible refinement would be to change pw(8) and adduser(8) to automatically include the user's group from the password database in the groups database, possibly based on some flag/config (command-line, pw.conf, adduser.conf).
Alternatives:

• Instead, just call setgroups(2) on the whole list returned by getgrouplist(3), that's the "For" alternative above.
• In addition, call setegid(2) to change the effective GID, this is equivalent to the current patch (which falls under "Against" as well for the dilemma above), basically restoring the previous behavior (before the setgroups(2)/getgroups(2) changes). However, other implementations never set the effective GID. "Portable" programs usually support our idiosyncrasy, but there are bugs sometimes (kevans@ uncovered one recently in OpenSSH).

What do people think?

My dilemma here is that, assuming the docs you recall for admins does exist (I don't recall either way), we have two competing notions of how this works:

In D52282#1195193, @olce wrote:

A priori, we have to decide between two alternatives for initgroups(3):

1. We restore the exact old behavior, which was modified by the setgroups(2)/getgroups(2) changes: The additional group passed to initgroups() is in fact meant to become the effective GID, and those retrieved from the groups database the supplementary groups.
2. Or we agree with the change of behavior, but then we have to add a new symbol version for initgroups(3) and provide the new implementation in this review as the backwards compatible implementation.

How about we cut a deal: if you slap a deprecation on comcontrol is 15.0, I'll look at adding drainwait configuration to stty(1) to replace it. We don't need to maintain a special command to "control a special tty device" for a configuration item that's implemented in the tty layer.

Note that all of these have .ORDER declarations a few lines up that effectively serializes all of the generated targets.

Revise based on feedback and further consideration

In D51837#1194076, @pfg wrote:

FWIW, just about the best test one can make for a significant patch change is to have an exp-run over the ports tree.

Move the out-node parameter to the second and update the spatch accordingly, and
set *opn up front.

This was an intentional decision, not an oversight. On every other platform it does exactly what we had previously documented, leaving the egid alone and ensuring the passed-in basegid is added to the supplementary group list.

Incorporate review feedback

Thanks!

Take a stab at converting pfs_uninit to last-unmount

In D52161#1191987, @rgrimes wrote:

In D52161#1191928, @kevans wrote:

but I don't think I've seen you comment on the reason it is like this: how do you feel about trying to set $HOME in init(8) when we invoke the single-user shell?

I think thats creep of shoving a problem in pkg inability to handle the situation by putting a set of bandaids (removal of /.profile being one, adding setting $HOME to init a second.)

In D52174#1191961, @imp wrote:

So this is an installer bug? We should fail here?

In D52174#1191958, @cracauer wrote:

I think the least bad thing as a first step is to allow boot with the raidz_expansion flag set if the zpool is not zraid in the first place. That covers many users.

In D52174#1191951, @cracauer wrote:

Another can of worms is what I often do, which is prepare a new FreeBSD installation by connecting a new disk as a second disk to an existing install and prepare a root filesystem on the commandline. It will be very hard to prevent me from footshooting in that situation.

In D52174#1191950, @cracauer wrote:

zfeature_common.c:759 says that this feature does not fall into the category of readonly-compatible features. That's the list that determines whether a lack of this feature in the running kernel would still allow a readonly mount.

We could also forbid actually expanding a raidz when it is a root filesystem

We chatted a bit and I was pointed to the theory statement in module/zfs/vdev_raidz.c (line ~148). My inclination is that we should actually disable this in the installer-created root pools and warn about it in documentation somewhere. Our read support would Just Work(TM) pre-expansion, but as @cracauer had already suspected there's a period of time in the middle of an expansion where you need to re-math for blocks that haven't been reflowed yet.

In D52174#1191937, @cracauer wrote:

In D52174#1191934, @imp wrote:

Linux has a policy of creating root partitions with a tiny subset of the capabilities in it. We should look into updating the installer to do that, but I'm pretty sure it wouldn't have big uptake.

But this change is needed, go ahead.

We remind ZFS users all the time to upgrade their pools (which I don't like). Users would re-introduce excluded features easily.

I am waiting for some ZFS specialist to voice an opinion on whether raidz_expansion is indeed read-neutral.

In D52161#1191908, @rgrimes wrote:

In D52161#1191907, @kevans wrote:

In D52161#1191888, @emaste wrote:

In D52161#1191886, @grahamperrin wrote:

In D52161#1191869, @rgrimes wrote:

… single user.

Is either .cshrc or .profile intended to be effective in this mode?

I don't see a reason it should not be.

I tried it on my system (the text block above) and confirmed that sh read /root/.profile when booted in single user mode.

I actually can't reproduce that result here. If I remove /.profile and add a SECRETVAR to /root/.profile, it's not populated (and I don't really see where we'd get a $HOME from in init or early /bin/sh)

First to Ed, did you rm /.profile? As if you read the contents you well see that it SETS /home.

To kevans, thanks that confirms that it was /.profile that set home.