Hi Marc,
I have a working implementation of the counter, with tests, and running it
raised four questions I would rather settle with you than guess.
1. A single counter does not survive sub-ranges
The naive reading of "store the last uid and increment from there" breaks when
a range is requested explicitly:
# REUSE_IDS=no
adduser --firstuid 5000 --lastuid 5010 rhigh # gets 5000, counter=5000
addgroup --firstgid 2090 --lastgid 2093 rlow
warn: No GID is available in the range 5001-2093 (FIRST_GID - LAST_GID)
fatal: The group `rlow' was not created.
The 2090-2093 range is completely empty, but the counter of the default range
pushed the floor above its ceiling.
My fix is to make the range part of the state file key, so each range carries
its own counter:
@global:last_id_1000_59999=1006:last_sys_id_100_999=104
The guarantee becomes "an id is not handed out twice within the range it came
from". Does that match what you had in mind, or would you rather have a single
counter with a different rule for sub-ranges?
2. Should an explicit range turn off the automatism?
You wrote that for --uid "the wish of the local admin wins and turns off the
automatism". Asking for --firstuid/--lastuid looks like the same kind of
explicit wish to me, and treating it that way would also leave
firstlastuidgid.t untouched. But it does mean that an admin who always carves
ranges never gets the guarantee. Your call.
3. The default is more expensive than it looks
With the counter always on, 122 assertions fail in firstlastuidgid.t,
firstlastuidgid_orig.t, uidgidpool.t and suidsgidpool.t. They are not stale:
they encode the current contract, that adduser picks the first free id of the
range. After any deletion that stops being true.
So I currently default REUSE_IDS to yes, which leaves every existing
installation and the whole test suite untouched, and lets an admin opt in.
Flipping it to no is a one-word change plus a NEWS.Debian entry, but it needs
those tests adapted. Which do you prefer?
I did not want to rewrite assertions in firstlastuidgid*.t anyway, since you
are reworking that file in wip/new-firstlastuidgid.
4. Where should the counter live inside the state file?
The store is keyed by user name and a counter belongs to no user. Rather than
storing it under a fake user name, I taught _read_state() and _write_state()
about a reserved '@global' record, so delete_state_user() can not take the
counters with it. That touches the format of your module, so tell me if you
prefer something else.
5. Is a second knob wanted at all?
You never mentioned configuration, so this may be over-engineering on my side:
I added ID_COUNTER_INIT=first_free|highest, because starting at the first free
id (as you suggested) cannot cover accounts that were deleted before the state
file existed, which was Aaron Hall's unanswered question from 2006. If you
would rather not have that option, I will drop it.
Unrelated to this bug, I also came across a couple of small things while
reading the code (a duplicated exit code value that contradicts the manpage,
among others). I will file those separately rather than mixing them in here.
--
[o]---[o]---[o]
| \ | / | Juan Muñoz <[email protected]>
[o]---[*]---[o] Building things the universe didn't ask for.
| / | \ | Tor relay operator · RIPE Atlas probe
[o]---[o]---[o] OONI Probe · salsa.debian.org/juaesm
