[bug#44700] services: setuid: More configurable setuid support.

[Christopher Lemmer Webber]

Ludovic Courtès writes:

> Hello!
>
> Christopher Lemmer Webber <cwebber@dustycloud.org> skribis:
>
>>>From eadac673fb22132c555a4e1cee57a6308ecfdad4 Mon Sep 17 00:00:00 2001
>> From: Christopher Lemmer Webber <cwebber@dustycloud.org>
>> Date: Sun, 15 Nov 2020 16:58:52 -0500
>> Subject: [PATCH] services: setuid: More configurable setuid support.
>>
>> New record <setuid-program> with fields for setting the specific user and
>> group, as well as specifically selecting the setuid and setgid bits, for a
>> program within the setuid-program-service.
>>
>> * gnu/services.scm (<setuid-program>): New record type.
>>   (setuid-program, make-setuid-program, setuid-program?)
>>   (setuid-program-program, stuid-program-setuid?, setuid-program-setgid?)
>>   (setuid-program-user, setuid-program-group): New variables, export them.
>>   (setuid-program-entry): New variable, a procedure used for the
>>   service-extension of activation-service-type as set up by
>>   setuid-program-service-type.  Unpacks the <setuid-program> record,
>>   handing off within the gexp to activate-setuid-programs.
>>   (setuid-program-service-type): Make use of setuid-program-entry.
>> * gnu/build/activation.scm (activate-setuid-programs): Update to expect a
>>   ftagged list for each program entry, pre-unpacked from the <setuid-program>
>>   record before being handed to this procedure.
>
> This looks like the right approach to me!
>
>> +  (for-each (match-lambda
>> +              [('setuid-program src-path setuid? setgid? user group)
>> +               (let ((uid (match user
>> +                            [(? string?) (passwd:uid (getpwnam user))]
>> +                            [(? integer?) user]))
>> +                     (gid (match group
>> +                            [(? string?) (group:gid (getgrnam user))]
>> +                            [(? integer?) group])))
>> +                 (catch 'system-error
>> +                   (lambda ()
>> +                     (let ((target (string-append %setuid-directory
>> +                                                  "/" (basename src-path)))
>> +                           (mode (+ #o0555                   ; base 
>> permissions
>> +                                    (if setuid? #o4000 0)    ; setuid bit
>> +                                    (if setgid? #o2000 0)))) ; setgid bit
>> +                       (copy-file src-path target)
>> +                       (chown target uid gid)
>> +                       (chmod target mode)))
>
> Nitpick: I’d write “program” or “source” instead of “src-path” and avoid
> square brackets for consistency with the rest of the code base (you
> spent time in Racket-land, didn’t you? ;-)).

Sounds good.  And yes, Racket influence is shining through, oops!

>> +(define (setuid-program-entry programs)
>> +  #~(activate-setuid-programs
>> +     ;; convert into a tagged list structure as expected by
>> +     ;; activate-setuid-programs
>> +     (list #$@(map (match-lambda
>> +                     [(? setuid-program? sp)
>> +                      #~(list 'setuid-program
>> +                              #$(setuid-program-program sp)
>> +                              #$(setuid-program-setuid? sp)
>> +                              #$(setuid-program-setgid? sp)
>> +                              #$(setuid-program-user sp)
>> +                              #$(setuid-program-group sp))]
>> +                     ;; legacy, non-<setuid-program> structure
>> +                     [program
>> +                      ;; TODO: Spit out a warning here?
>> +                      #~(list 'setuid-program
>> +                              #$program
>> +                              #t #t 0 0)])
>> +                   programs))))
>
> Maybe what we could do is rename ‘operating-system-setuid-programs’ to
> ’%operating-system-setuid-programs’, keep that internal, and add a new
> ‘operating-system-setuid-programs’ that calls the other one and
> “canonicalizes” list entries so that they’re all <setuid-program>
> records.

"rename"?  There is no operating-system-setuid-programs so I'm not sure
what you mean to rename from... setuid-program-entry, or presumably
activate-setuid-programs...?

> It would call:
>
>   (warning log (G_ "representing setuid programs with strings is \
> deprecated; use 'setuid-program' instead~%"))

Aha, I wasn't sure what to use for deprecation warnings actually, so
this is helpful, thanks!

> WDYT?
>
> Could you also update the “Setuid Programs” section of the manual?

Happy to do it.

> In a subsequent commit, we need to adjust all the services that extend
> ‘setuid-program-service-type’ so they pass a <setuid-program> and not a
> string.

Yes... let's worry about that once this interface is hammered out. :)

Glad it seems like the general approach was right though!

> Thanks!
>
> Ludo’.