[bug#77201] guix: substitute-key-authorization: Fix case when acl symlink is broken

One possible solution for an issue when /etc/guix/acl file exists, but points
to a non-existent location. This can for example happen if one is
reinitializing the system, and remove only /gnu/store and /var/guix, keep the
rest okay. This is a major advantage of guix as compared to other distros that
usually need you to reinitialize the whole root partition. But this will leave
the user with acl file pointing to non-existent location. The file-exists?
procedure will return #f for broken symbolic links.

I think that another reason one would get this issue is, if one was booted in
a live iso, chrooted, fixing their system. They would switch generations to
one with different acl file, delete other generations gc rooting the original
acl file and then gc. One could do this approach for example when recovering
from file corruptions in the store, to get rid of the unsubstitutable paths
that can't be repaired with guix gc --verify.

I don't know if there is a better way as I am not that proficient in guile,
but I definitely think this should be fixed and it should be checked if
anything exists in that place, not that the link points to a known location.
Does Guile have a procedure for that that I am missing? If not, shouldn't
we create one in Guix? I can imagine this being a common mistake, where we
want to check if something exists at place 'x', without caring if it's
actually an accessible file. I was looking online and someone made themselves
a function 'file-exists??' that checked basically this what I did here - that
it's either a valid file on the disk, or a broken symlink.

During debugging this issue I also noticed similar issue can occur in special
files and /run/current-system with the .new files that are created with
symlink procedure without checking for their existence. While it's not likely
(especially because the symlink is moved the second it's created)
the user would have /run/current-system.new nor /bin/sh.new etc., I still
think it would be worth fixing to make sure the system can boot even in cases
where something goes horribly wrong.

* gnu/services/base.scm (substitute-key-authorization): Check if acl file is a
(broken) symbolic link

Change-Id: I2f8170606b2f4afeea48f04acfd738b04cafc7cf
---
 gnu/services/base.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: fbfd2b93831978aadbb96f32cafdab997b04c6c6
prerequisite-patch-id: cf473eb15513404ca1d287f5b7eca109c848203c
prerequisite-patch-id: a46e75bdd193acb5e276e0aa31c77197a3254699
prerequisite-patch-id: a2b4aa0a33d89ee3f6c483aeb71a783cb0e63aa9
  

Hi Rutherther,

Rutherther <rutherther@ditigal.xyz> writes:

> One possible solution for an issue when /etc/guix/acl file 
> exists, but points
> to a non-existent location. This can for example happen if one 
> is
> reinitializing the system, and remove only /gnu/store and 
> /var/guix, keep the
> rest okay. This is a major advantage of guix as compared to 
> other distros that
> usually need you to reinitialize the whole root partition. But 
> this will leave
> the user with acl file pointing to non-existent location. The 
> file-exists?
> procedure will return #f for broken symbolic links.
>
> I think that another reason one would get this issue is, if one 
> was booted in
> a live iso, chrooted, fixing their system. They would switch 
> generations to
> one with different acl file, delete other generations gc rooting 
> the original
> acl file and then gc. One could do this approach for example 
> when recovering
> from file corruptions in the store, to get rid of the 
> unsubstitutable paths
> that can't be repaired with guix gc --verify.
>
> I don't know if there is a better way as I am not that 
> proficient in guile,
> but I definitely think this should be fixed and it should be 
> checked if
> anything exists in that place, not that the link points to a 
> known location.
> Does Guile have a procedure for that that I am missing? If not, 
> shouldn't
> we create one in Guix? I can imagine this being a common 
> mistake, where we
> want to check if something exists at place 'x', without caring 
> if it's
> actually an accessible file. I was looking online and someone 
> made themselves
> a function 'file-exists??' that checked basically this what I 
> did here - that
> it's either a valid file on the disk, or a broken symlink.
>
> During debugging this issue I also noticed similar issue can 
> occur in special
> files and /run/current-system with the .new files that are 
> created with
> symlink procedure without checking for their existence. While 
> it's not likely
> (especially because the symlink is moved the second it's 
> created)
> the user would have /run/current-system.new nor /bin/sh.new 
> etc., I still
> think it would be worth fixing to make sure the system can boot 
> even in cases
> where something goes horribly wrong.

Thanks for the explanation.

> * gnu/services/base.scm (substitute-key-authorization): Check if 
> acl file is a
> (broken) symbolic link
>
> Change-Id: I2f8170606b2f4afeea48f04acfd738b04cafc7cf
> ---
>  gnu/services/base.scm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gnu/services/base.scm b/gnu/services/base.scm
> index 0d2bb31190..e419d043ae 100644
> --- a/gnu/services/base.scm
> +++ b/gnu/services/base.scm
> @@ -1845,7 +1845,7 @@ (define (substitute-key-authorization keys 
> guix)
>          ;; If the ACL already exists, move it out of the way. 
>          Create a backup
>          ;; if it's a regular file: it's likely that the user 
>          manually updated
>          ;; it with 'guix archive --authorize'.
> -        (if (file-exists? acl-file)
> +        (if (or (file-exists? acl-file) (symbolic-link? 
> acl-file))

Guile semantics are unhelpful here: `file-exists?' returns #f for 
a broken symlink, but `symbolic-link?' raises an exception if 
given a nonexistent path.  The means that if /etc/guix/acl doesn’t 
exist, an exception will be raised.  There doesn’t appear to be a 
simple way to determine if a file exists which doesn’t resolve 
symlinks, which I think is a Guile bug.

Thinking through the possible situations here:

If /etc/guix/acl is a good symlink pointing into /gnu/store -> 
delete it.

If /etc/guix/acl is a broken symlink pointing anywhere -> delete 
it.

If /etc/guix/acl is a file -> rename it to ".bak"

else /etc/guix/acl must be missing -> mkdir-p /etc/acl.

...then populate /etc/guix/acl.

I think the right move here is to refactor the nested `if's into a 
cond to simplify the logic, and wrap `symbolic-link?' in 
`with-exception-handler' (possibly `let’-binding its result, since 
multiple things need it).

Thanks,

  -- Ian
  

Hello,

Ian Eure <ian@retrospec.tv> writes:

>> -        (if (file-exists? acl-file)
>> +        (if (or (file-exists? acl-file) (symbolic-link? acl-file))
>
> Guile semantics are unhelpful here: `file-exists?' returns #f for a
> broken symlink, but `symbolic-link?' raises an exception if given a
> nonexistent path.

I would go back to the fundamentals:

  (match (and=> (false-if-exception (lstat acl-file)) stat:type)
    (#f ;file does not exist
     …)
    ('symlink
     …)
    (_
     …))

HTH!

Ludo’.
  

Hi Ludo,

Ludovic Courtès <ludo@chbouib.org> writes:

> Hello,
>
> Ian Eure <ian@retrospec.tv> writes:
>
>>> -        (if (file-exists? acl-file)
>>> +        (if (or (file-exists? acl-file) (symbolic-link? acl-file))
>>
>> Guile semantics are unhelpful here: `file-exists?' returns #f for a
>> broken symlink, but `symbolic-link?' raises an exception if given a
>> nonexistent path.
>
> I would go back to the fundamentals:
>
>   (match (and=> (false-if-exception (lstat acl-file)) stat:type)
>     (#f ;file does not exist
>      …)
>     ('symlink
>      …)
>     (_
>      …))

This definitely helps, thanks, I am still not that skilled in Scheme, so
I wouldn't think about false-if-exception, and using match would also be
hard for me to figure out.

I have this now (untested for now)
```
(match (and=> (false-if-exception (lstat acl-file)) stat:type)
          (#f #f) ; File doesn't exist
          ('symlink ; Delete symlink pointing to store; backup otherwise.
           (if (or (store-file-name? (readlink acl-file)) ; Store symlink
                   (not (file-exists? acl-file))) ; Broken symlink
               (delete-file acl-file)
               (rename-file acl-file (string-append acl-file ".bak"))))
          (_ ; Backup
           (rename-file acl-file (string-append acl-file ".bak"))))
```
I will probably also make this into a reusable function in guix utils
build, but I have been thinking about a good name and couldn't come up with
one for a week! Programmers problems, I guess.

WDYT

Thanks,
Rutherther
  

Hi,

Rutherther <rutherther@ditigal.xyz> writes:

>>   (match (and=> (false-if-exception (lstat acl-file)) stat:type)
>>     (#f ;file does not exist
>>      …)
>>     ('symlink
>>      …)
>>     (_
>>      …))
>
> This definitely helps, thanks, I am still not that skilled in Scheme, so
> I wouldn't think about false-if-exception, and using match would also be
> hard for me to figure out.
>
> I have this now (untested for now)
> ```
> (match (and=> (false-if-exception (lstat acl-file)) stat:type)
>           (#f #f) ; File doesn't exist

Indentation is off (see above.)  Also (I’m nitpicking!), margin comment
should not be a full sentence, so rather:

  (#f #f)  ;file doesn't exist

>           ('symlink ; Delete symlink pointing to store; backup otherwise.
>            (if (or (store-file-name? (readlink acl-file)) ; Store symlink
>                    (not (file-exists? acl-file))) ; Broken symlink
>                (delete-file acl-file)
>                (rename-file acl-file (string-append acl-file ".bak"))))
>           (_ ; Backup
>            (rename-file acl-file (string-append acl-file ".bak"))))

Should be good!

> I will probably also make this into a reusable function in guix utils
> build,

Maybe not: (guix build utils) is rarely updated because it entails world
rebuilds, which also means that it’s interfaces must be very generic and
bullet-proof.  So I would keep it local for now.

Thanks,
Ludo’.
  

Apologies for the delay, I somehow didn't get to testing in a VM last
week, today I finally tested it, found a few issues, fixed them, and it
seems to be working fine for me. I tried to test all possible cases:

- No acl file
- Regular file
- Symlink to store
- Symlink out of store
- Broken symlink

- .bak (not) existing

I've kept it directly in this function upon Ludo's comment about
rebuilding the whole world if it was added to guix/build/utils, I can
create a new guix/build file if you think it makes sense to have it
reusable, or put it to another one(?).

Since this is activation logic, I think it's important to thoroughly
test it - should a new VM tests be added for this, or is it unnecesasry
and the current tests suffice? If something went wrong, the tests would
fail to boot (I haven't tried using system tests defined in guix yet, I
will try to run them).
Will QA run them for this commit or is it ran only for defined branches?

Submitted v2.

Regards,
Rutherther
  

I was skimming through the code and I found out install-channels-file
has the same issue as substitute-key-authorization has with this
symlinking. I suppose I will make this into a generic function after
all, so that it can be used in both of those functions.

Just wondering if I should make it a gexp or put it to some of the
guix/build/ files (or a new one?)

"Rutherther" <rutherther@ditigal.xyz> writes:

> Apologies for the delay, I somehow didn't get to testing in a VM last
> week, today I finally tested it, found a few issues, fixed them, and it
> seems to be working fine for me. I tried to test all possible cases:
>
> - No acl file
> - Regular file
> - Symlink to store
> - Symlink out of store
> - Broken symlink
>
> - .bak (not) existing
>
> I've kept it directly in this function upon Ludo's comment about
> rebuilding the whole world if it was added to guix/build/utils, I can
> create a new guix/build file if you think it makes sense to have it
> reusable, or put it to another one(?).
>
> Since this is activation logic, I think it's important to thoroughly
> test it - should a new VM tests be added for this, or is it unnecesasry
> and the current tests suffice? If something went wrong, the tests would
> fail to boot (I haven't tried using system tests defined in guix yet, I
> will try to run them).
> Will QA run them for this commit or is it ran only for defined branches?
>
> Submitted v2.
>
> Regards,
> Rutherther