Done in devel.
Can I integrate this to test and stable-rc ?

You don't need to set the integrate to stable flag for an integrate to 
stable-rc.

If it fixes a bug in stable-rc it can go there as long as it's not a new 
feature -- is this a new feature?  Is it reliable?  How much cooking in 
devel/test has it seen?

Seth

Looking at the ISO bugs and what it runs, we could probably put it in 
stable-rc. after you've had three confirmed reports of success in 
devel/test.

We should probably get hopping on integrating this


-- 

Wait with the hopping.
We have received reports on IRC about grpconv eating memory and then getting
OOM'ed. This was caused in at least one case by a corrupt /etc/group file
(duplicate group name).
The solution is to run grpck first, to fix the group file, or report problems
that the user should fix (grpck -r is read-only testing, grpck by itself is
interactive)

This is in stable now, so can we declare this FIXED? I haven't heard of any
cases where it failed.

This bug now exists in stable-rc, version 0.5

beep?

Thomas.Houssin@sourcemage.org is no longer a valid e-mail address and Thomas 
himself will not be replying, whomever wants to make this happen should do so.

wrong circle

Oh... I remember grpconv and/or pwconv screwing...
They may screw on _valid_ files as I remember.
Different names with same id is a setup that some people actually enjoy for 
many years now.
It has uses.

At least this conversion should always be optional... with defaulting to "n" 
even.
And... heck, without castfs, that will always modify /etc/passwd 
and /etc/grpconv (I presume). That is not good!
Anyone objecting me adding such a query with a safe default of n?

I suggest first a grpck -r is ran, so the user has more information on what 
would happen.

And no, no objections from me, go ahead. :)

(I meant /etc/passwd  and /etc/group being modifed when no castfs is there, of 
course)

1. So we silently ignore the issue that the host system's passwd database is 
modified instead of $INSTALL_ROOT/etc/passwd ... until we got castfs settled 
in. Well, I guess we have to, since every user stuff we do currently happens 
on hosts database... so we break nothing that wasn't broken in advance.

2. grpck -r ... to inform the user; so you mean with a possibility to 
interact?
When should this happen? In configure stage it is strictly speaking too early 
because other spells in the cast can modify accounts in between, in FINAL, it 
is too late since we don't want questions _after_ configure phase.
I don't see that fitting. Also, I want at least those basic spells to cast 
with at least the theoretic possibility of cross-compiling.
I want cross-compiling to happen some time and just executing the installed 
binaries assumes that the arch that compiled them can execute them (which is 
generally not true in cross compiling).
So, I'd go for a query in CONFIGURE "attempt to convert/fix accounts?" and 
perhaps a grpck happening before the others... hm, only converting when the 
check shows no errors... if it does show errors, just print em.
Really fixing up the files is a process that we should not attempt to do 
non-interactively - hence, not in a spell's FINAL.

I think it is safe to assume other spells will do the right thing.

I'd add to CONFIGURE something like this:
{ grpck -r ||
if query "Attempt to convert/fix accounts?" n; then
  grpck
fi
}

grpck is interactive, but that would be ok in CONFIGURE. grpck -r isn't.

I don't like the not-asked calling of grpck. Even if it is in basesystem, 
expecially _because_ it messes with host system currently, I would not cast it 
into a chroot but only after entering it.
After entering chroot, there is no grpck to start with.
We have to wait after shadow is installed first time.
Of course we can argue about shadow having to be installed on host system and 
whatever, but I don't think that it is any good to run grpck unasked at all.
Perhaps we can change the default for this question to "y", but I don't feel 
good with this as I once got bitten by grpconv bitching about a group file it 
didn't like.

Hm, one can think of a box without shadow. Not just think. IMHO I spent days 
in an office with boxen that didn't have/use shadow in favor of PAM/LDAP, not 
totally sure, though, for the root account was local. But you can imagine...
heck, when all normal users come via LDAP, you can make /etc/passwd readable 
only for root - what to use shadow for, then?

Anyway, what about

if query "Attempt to convert/fix accounts (with installed shadow utils)?" n; 
then
  grpck -r "$INSTALL_ROOT/etc/group" "INSTALL_ROOT/etc/gshadow" ||
  {
    message "there are problems in your group file... runinng grpck 
interactively" &&
    grpck "$INSTALL_ROOT/etc/group" "INSTALL_ROOT/etc/gshadow"
  }
fi

Oh, and of course I'd do the same with pwck.

Looks good to me. For pwck I'd also add -q, so it doesn't needlessly spam (even 
fails otherwise). I got some 10 lines of missing $HOME warnings ...

       -q Report errors only. The warnings which do not require any action from 
the user won't be displayed.

I prepped the spell now with this in CONFIGURE:

config_query SHADOW_CONV "Attempt to convert/fix accounts (with installed 
shadow utils)" y &&
if [[ "$SHADOW_CONV" == y ]]; then
  message "OK, checking your user and group accounts" &&
  grpck -r "$INSTALL_ROOT/etc/group" "$INSTALL_ROOT/etc/gshadow" ||
  {
    message "there are problems with group... runinng grpck 
interactively" &&
    grpck "$INSTALL_ROOT/etc/group" "$INSTALL_ROOT/etc/gshadow"
  } &&
  pwck -q -r "$INSTALL_ROOT/etc/passwd" "$INSTALL_ROOT/etc/shadow" ||
  {
    message "there are problems with passwd... runinng grpck 
interactively" &&
    grpck "$INSTALL_ROOT/etc/passwd" "$INSTALL_ROOT/etc/shadow"
  }
fi

and this in FINAL:

if [ "$SHADOW_CONV" == y ]; then
  #Create or update necessary files for shadow
  message "running pwconv and grpconv" &&
  ${INSTALL_ROOT}/usr/sbin/pwconv &&
  ${INSTALL_ROOT}/usr/sbin/grpconv
fi


Is that OK with everyone? I am not totally sure if we want to modify the 
system in CONFIGURE (policy?), but we for sure don't want questions on 
DETAILS, so _if_ grp/pwck is run, it has to happen in CONFIGURE...
When I don't hear a scream in a day I guess I'll commit it to test.

I would change
  } &&
to
  };

so if the user for some reason doesn't want to fix the file, the pwck won't be 
skipped.

# grpck /etc/group; echo $?
duplicate group entry
delete line 'bin:x:1:'? n
duplicate group entry
delete line 'bin:x:1:'? n
grpck: no changes
2

Even if she fixes it, the status is not 0:
# grpck /etc/group; echo $?
duplicate group entry
delete line 'bin:x:1:'? y
grpck: the files have been updated
2

So I'd also add a "true" at the end of CONFIGURE.

What about

c0695802d6900775e09054cef09412bdd1fd8647

?
Can we close this?

Approved.

(In reply to comment #11)
> At least this conversion should always be optional... with defaulting to "n" 
> even.
> And... heck, without castfs, that will always modify /etc/passwd 
> and /etc/grpconv (I presume). That is not good!
> Anyone objecting me adding such a query with a safe default of n?

I agree with the above, but why did you actually commit it with:

 config_query SHADOW_CONV "Attempt to convert/fix accounts (with installed 
shadow utils)" y &&

That should certainly default to n.

And grpconv updates the backups, so the conversion should be pretty safe. The 
same holds true for pwconv.

From the passwd case it seems it does the right thing and doesn't overwrite the 
backup if the original is not ok.

lynxlynx navaden # ls -lh /etc/group*
-r--r--r-- 1 root root 477 2007-01-22 20:47 /etc/group
-rw------- 1 root root 486 2007-01-22 12:10 /etc/group-
lynxlynx navaden # grpconv
lynxlynx navaden # echo $?
0
lynxlynx navaden # ls -lh /etc/group*
-r--r--r-- 1 root root 477 2007-01-22 21:59 /etc/group
-rw------- 1 root root 477 2007-01-22 20:47 /etc/group-


lynxlynx navaden # ls -lh /etc/pas*
-r--r--r-- 1 root root 626 2007-01-18 10:52 /etc/passwd
-rw------- 1 root root 588 2006-11-25 13:27 /etc/passwd-
lynxlynx navaden # pwck -q
lynxlynx navaden # echo $?
0
lynxlynx navaden # pwck -r
user navaden: no group 100
user fcron: directory /var/run/fcron does not exist
user sshd: directory /var/run/sshd does not exist
user messagebus: directory /var/run/messagebus does not exist
user haldaemon: directory /var/run/haldaemon does not exist
user boinc: directory /var/lib/boinc does not exist
user wnn: directory /home/wnn does not exist
user iplog: directory /var/run/iplog does not exist
user smmsp: directory /var/run/smmsp does not exist
user distcc: directory /var/run/distcc does not exist
user pascal: directory /var/run/pascal does not exist
user mail: directory /var/run/mail does not exist
pwck: no changes
lynxlynx navaden # echo $?
2
lynxlynx navaden # pwconv
lynxlynx navaden # ls -lh /etc/pas*
-r--r--r-- 1 root root 626 2007-01-18 10:52 /etc/passwd
-rw------- 1 root root 588 2006-11-25 13:27 /etc/passwd-

I let the default "y" stay because I didn't feel like arguing and ... hm, it 
was the default behaviour before my change.
I still would prefer it the other way in fact.

When we agree on that, I'll change this to "n". Will we need to re-clear the 
integrate flag?

Defaulting to n for the files modification:

2e6b8321d6dfd901a7db1979ca65cde4e19b8109

Jaka I don't see a reason you shouldn't integrate this once your INSTALL/TRACK
ROOT question is answered.

ok, that was nicely clarified and the fixes integrated.

Do we need to query/do this on every cast, or only if /etc/shadow or
/etc/gshadow don't exist?

it's a config_query, so you won't be prompted each time. But all the info in 
this bug suggests it is needed every time for
Instead of doing `touch       ${INSTALL_ROOT}/etc/shadow` in FINAL, the spell
could run `pwconv` and `grpconv`.

it is more a matter of the files being bad than missing.

Just now I was shocked that I saw passwords in /etc/passwd on my box.
They always were at /etc/shadow on my box, of course I have shadow installed.
Seems last changes to shadow (do not know which one and when, I periodically run
"sorcery system-update" and don't check individual spells, only if there are
crucial problems).

After "cast -r -c shadow" and answer y to "Attempt to convert/fix accounts (with
installed shadow utils)" passwords are back in /etc/shadow.

IMHO not only my box was "broken". What about our users? We should probably warn
them to check their /etc/passwd.

We should create some source-mage-check-security script which could warn users
about such as problems.

Holy Maccaroni! You're right.
We had all this thinking about pwconv and grpconv but managed to forget the 
unconditional pwunconv and grpunconv in PRE_REMOVE!
That's bad. Actually, it's bad that the unconv performed at all - formerly, an 
upgrade of shadow included one unconv followed immediately by a conv to shadow 
files.
So the bright side of the untolerable security issue about having the hashes 
back in passwd is that we finally managed to stop that PRE_REMOVE behaviour.
Theoretically, there was a time span on every upgrade (re)cast of shadow where 
the password hashes are readable in passwd/group instead of the shadow files 
that only root can see...

Long speech, undefined meaning: Here is the commit to test that hopefully 
fixes that

fe545c4b24abdeb7177efbab4a650469f4d6a284

I request integration of that into stable and stable-rc asap as both are 
affected and this issue is serious. I tested the spell on two boxen, so it 
_should_ be fine now.

It is not all-nice polish because now after having the case of 
deleted /etc/shadow from pwunconv I see where the pwck in CONFIGURE fails when 
there is no $INSTALL_ROOT/etc/shadow .
Solution would be to not provide the files in $INSTALL_ROOT to pwck and let it 
just work on /etc/passwd and /etc/shadow if it exists or to touch 
$INSTALL_ROOT/etc/shadow beforehand.
The thing with $INSTALL_ROOT is, that pwck supports working on files other 
than /etc/passwd and /etc/shadow but pwconv doesn't so that the use of 
INSTALL_ROOT is not totally honest.

Anyhow, that failure of pwck (and of course grpck, too) is not fatal and 
pwconv is still run in FINAL ... but I guess it should be fixed.
Shall I

1. just call pwck / grpck without specifying files in $INSTALL_ROOT
2. touch $INSTALL_ROOT/etc/[g]shadow
3. skip the check if no shadow there (not so good, one wants to check if 
passwd / group are ok since the conv tools can go berserk on double entries)
4. something different

?

Modified the fix with the add-on commit

37a38d2e3b991e47fc2b4cf2e4836dacea0c2ab9

Now the extra emergency query is a one-timer and still defaults to n since we 
consider our users to be aware of things and upgrade stuff like shadow with 
open eyes (or at least check the log for the messages).

Jeremy: I didn't bother with git-revert the old commit and then re-applying 
stuff from it together with your new suggestion because that woudld have 
complicated things for me which is not good in a time like this.

So, the RFI is about 

fe545c4b24abdeb7177efbab4a650469f4d6a284
and
37a38d2e3b991e47fc2b4cf2e4836dacea0c2ab9

Not that I have lots of authority, but I think this is good.

I don't like this dialogue when we've just told them it's broken and we want to
fix it.  It looks like it's failing more, even though the FINAL is going to take
care of it.

I'm going to add some -f checks before it runs the chk commands.  We can remove
those later when we figure out what we really want.

# cast -c shadow
Computing previously installed dependencies...
shadow preparing environment...
shadow running configuration...
Checking passwd for shadowness (there could have been an unwanted run of
pwunconv/grpunconv, see bug #8834).
Your passwd file contains password hashes, resuggesting conversion to shadow.
You can still say n to the upcoming query but make sure to run pwconv / grpconv
yourself if you want existing passwords shadowed.
One-time query: convert/fix accounts (with installed shadow utils) this time [n] y
OK, checking your user and group accounts
grpck: cannot open file /etc/gshadow
there are problems with group... runinng grpck 
interactively
grpck: cannot open file /etc/gshadow
pwck: cannot open file /etc/shadow
there are problems with passwd... runinng grpck 
interactively
grpck: cannot open file /etc/shadow

By the way is that second "running grpck interactively" for the _passwd_ file
correct??

these 2 + ac992cbabf4dbf0cb6eccb5392401f4b37fd2a84 are integrated, rolling
tarballs now

there is more to be done though so leaving this open

so, add commit
983d10e36a60751d5eb3e96fe8e37879dfcd8ab6

That actually runs pwck where it should.
Focus, people that now was a typical copy and paste mistake that just happens 
and that has a guarantee to slip through the author's fingers.
Didn't it really loook strange that after pwck -q -r we call grpck to fix 
stuff?
Obeying Murphy's law, Jeremy has seen that after rolling out the tarballs...

Anyway. We should really kill this beast off, and hence I suggest the patch 
I'll attach.
Just drop the $INSTALL_ROOT/etc/ files specification to the *ck calls. It was 
well meant by me but it really helps nothing when the *conv programs 
unconditionally work on /etc.
When we don't specify the $INSTALL_ROOT files, the *ck silently deal with 
non-existent shadow files. Did I say that these shadow utils could use quite 
some spit and polish? I should have.
Well, there's a newer shadow release out there, dunno how far it goes into the 
right direction.

So Jeremy, are we re-opening the integration fun?

Created attachment 6627 [details]
patch to shadow for removing the INSTALL_ROOT nonsense

Do we really think this needs integrating?  What is the result of running the
wrong check command?

Right now I am trying to figure out why vlock doesn't find my user anymore after
casting this latest version.  :-|

We could revert all the conversion changes until Sorcery supports castfs, and
then these checks/conversions won't be done against the live system. Would that
be the 'better' approach?

The proposed patch looks fine, since it's pointless to check a specific file
when the actual conversion occurs to a hardcoded file.

We can't just revert it all because we broke people's /etc/shadow and have to at
least fix that.

I think we've done that so far so I would prefer to leave this alone in stable
for now unless this other thing is also major breakage.  If it just causes a
no-op that doesn't do damage then I don't think it needs to go forward for now.

I applied that INSTALL_ROOT patch in test. 
Hm... castfs istn't yet active during CONFIGURE, is it?
We'd need that for really supporting INSTALL_ROOT there...

We've discussed on IRC that the wrong run of grpck on passwd leads to

"delete user 0?" y/n

...

Please integrate these two to stable-rc. We stopped changing it in stable just 
again, but we should not ship the clearly known broken spell we have in 
stable-rc now.
 9e41ddb1b6b997a33656708754ce46f6aa1e7eca
 983d10e36a60751d5eb3e96fe8e37879dfcd8ab6 

Shouldn't we check if $INSTALL_ROOT is set and bail out if it is, or something?
 I don't like just blithely ignoring $INSTALL_ROOT.

Hm, so prevent shadow from casting into a chroot?
It's an idea, but then we should add a profile spell like my private 
baresystem that depends on the minimal stuff you'll need for a chroot, casting 
rest of basesystem when being chrooted.
But then, on could use INSTALL_ROOT for other wishes than chrooting.
Someone wanting to have a /sec-tools prefix on the shadow utils or whatever...
Or did you mean with "bail out" that we just should prevent any chkpwd/pwconv 
action from happening?

(In reply to comment #44)
> Hm, so prevent shadow from casting into a chroot?

You're telling me it won't work with an $INSTALL_ROOT set, so yes, we should
prevent it from casting if we can't honor the $INSTALL_ROOT and would instead
touch the live system.

> It's an idea, but then we should add a profile spell like my private 
> baresystem that depends on the minimal stuff you'll need for a chroot, casting 
> rest of basesystem when being chrooted.

As a feature request maybe but for now we just need to not try to cast something
we know isn't going to honor an $INSTALL_ROOT.

> But then, on could use INSTALL_ROOT for other wishes than chrooting.
> Someone wanting to have a /sec-tools prefix on the shadow utils or whatever...
> Or did you mean with "bail out" that we just should prevent any chkpwd/pwconv 
> action from happening?

It either needs to refuse to go if they ask for the pwconvs and there's an
$INSTALL_ROOT (since it cannot do what they are asking), or it needs to prompt
them that it can't comply and let them choose what to do.

Can we try
chroot $INSTALL_ROOT /sbin/grpconv
? It won't work on a half-finished chroot of course, but it could be a start

Hm, on that alley, I'd prefer it to 
a) warn about pwconv/ck not working with $INSTALL_ROOT
b) always do the query and store the answer
c) in case of $INSTALL_ROOT, just skip all the action and tell the user about 
that
d) but still cast the plain shadow programs

The $INSTALL_ROOT case was actually what triggered my interest in making these 
conversions optional. I wanted to cast shadow into the chroot, but wanted it 
not to mess with passwd database.

I'd like any spell to be able to install without messing around (ideally, 
without having to run any of the newly installed progs so that crosscompiling 
may work in future).

Please get something together that solves the current major problems at least in
a workaround fashion so we don't hold up the release.  Bugs that existed prior
to stbale 0.6 don't have to get fixed, but anything introduced in 0.6 needs to
get fixed in 0.7.

We don't have to have final solution yet, we do need to have something.  I'd
prefer to see a minimal fix now and future work go on in devel or test til it's
done and ready to move forward.  stable-rc isn't the place to test things out.

So if the INSTALL_ROOT problems existed in 0.4 I guess we can get by with just
fixing that grpchk typo and call that good for now.  If those are new we need to
deal with them in a minimal fashion.

(In reply to comment #48)
> So if the INSTALL_ROOT problems existed in 0.4 I guess we can get by with just
> fixing that grpchk typo and call that good for now.  If those are new we need to
> deal with them in a minimal fashion.

Clarifying that, if the INSTALL_ROOT problem existed in 0.4 then the requested
integration is probably fine.  Otherwise we need something else.

The INSTALL_ROOT problem has been there since the addition of pwconv/grpconv, 
so yes, it was in stable-0.4 - following Arwed's statement in comment #6 .
When you look at my first coments in this discussion, you'll see that the 
INSTALL_ROOT ignorance was the grief I had to begin with - I just wanted a 
possibility to skip the *convs
... and this caused the *cks to be thrown in
... me seeing that _those_ could indeed work with INSTALL_ROOT
... adding fake INSTALL_ROOT support to these extra activities of shadow
... which it never really had.

So the current state in test is a clear improvement over stable-0.4  .
For now, it will mess with /etc in case of INSTALL_ROOT if ppl say yes to the 
query to mod passwd, but it did that before, too -- only without asking first.

(In reply to comment #42)
> Please integrate these two to stable-rc. We stopped changing it in stable just 
> again, but we should not ship the clearly known broken spell we have in 
> stable-rc now.
>  9e41ddb1b6b997a33656708754ce46f6aa1e7eca
>  983d10e36a60751d5eb3e96fe8e37879dfcd8ab6 
> 

approved

and integrated.

closing fixed bugs

reassign to sm-grimoire-bugs