GNU bug report logs - #72337
Add /etc/subuid and /etc/subgid support

Previous Next

Full log

Message #62 received at 72337 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Giacomo Leidi <goodoldpaul <at> autistici.org>
Cc: 72337 <at> debbugs.gnu.org
Subject: Re: bug#72337: Add /etc/subuid and /etc/subgid support
Date: Thu, 19 Sep 2024 13:14:57 +0200

Giacomo Leidi <goodoldpaul <at> autistici.org> skribis:

> This commit adds allocation logic for subid ranges. Subid ranges are
> ranges of contiguous subids that are mapped to a user in the host
> system. This patch implements a flexible allocation algorithm allowing
> users that do not want (or need) to specify details of the subid ranges
> that they are requesting to avoid doing so, while upholding requests of
> users that need to have specific ranges.
>
> * gnu/build/accounts.scm (list-set): New variable;
> (%subordinate-id-min): new variable;
> (%subordinate-id-max): new variable;
> (%subordinate-id-count): new variable;
> (subordinate-id?): new variable;
> (within-interval?): new variable;
> (insert-subid-range): new variable;
> (reserve-subids): new variable;
> (range->entry): new variable;
> (entry->range): new variable;
> (allocate-subids): new variable;
> (subuid+subgid-databases): new variable.
>
> * gnu/system/accounts.scm (subid-range-end): New variable;
> (subid-range-has-start?): new variable;
> (subid-range-less): new variable.
>
> * test/accounts.scm: Test them.
>
> Change-Id: I8de1fd7cfe508b9c76408064d6f498471da0752d
> Signed-off-by: Giacomo Leidi <goodoldpaul <at> autistici.org>

[...]

> +(define (vlist-set vlst el k)
> +  (if (>= k (vlist-length vlst))
> +      (vlist-append vlst (vlist-cons el vlist-null))
> +      (vlist-append
> +       (vlist-take vlst k)
> +       (vlist-cons el (vlist-drop vlst k)))))

So hmm, this is not great either because the ‘else’ branch has linear
complexity.

I don’t think there’s a good persistent data structure for this in Guile
unfortunately.  Again maybe plain lists or vlists are okay *if* we know
the lists are going to be small, but there needs to be a comment stating
it.

> +(define-condition-type &subordinate-id-range-error &subordinate-id-error
> +  subordinate-id-range-error?
> +  (message subordinate-id-range-error-message)
> +  (ranges subordinate-id-range-error-ranges))

Remove ‘message’ from here.  If we want a human-readable message, we can
always raise a “compound error condition” that combines
‘&subordinate-id-range-error’ and ‘&message’.

But I’m not sure we want messages anyway; I think we should focus on
ensuring ‘&subordinate-id-range-error’ has all the info.

> +(define (insert-subid-range range vlst)
> +  "Allocates a range of subids in VLST, based on RANGE.  Ranges
> +that do not explicitly specify a start subid are fitted based on
> +their size.  This procedure assumes VLIST is sorted by SUBID-RANGE-LESS and
> +that all VLST members have a start."

I’m not convinced by the use of (v)lists and the lack of abstraction
here.

How about having a tree along these lines:

  (define-record-type <unused-subuid-range>
    (unused-subuid-range left min max right)
    unused-subuid-range?
    (left    unused-subuid-range-left) ;previous unused subuid range or #f
    (min     unused-subuid-range-min)  ;lower bound of this unused subuid range
    (max     unused-subuid-range-max)  ;upper bound
    (right   unused-subuid-range-right)) ;next unused subuid range or #f

We’d start with:

  (unused-subuid-range #f %subordinate-id-min %subordinate-id-max #f)

Then, when consuming “to the left”, we’d add a child there, and so on.

Searching for an available range would be logarithmic.

Does that make sense?

(I’m really thinking out loud, this probably needs more thought.)

> +(let ((inputs+currents
> +       (list
> +        (list
> +         "ranges must have start"
> +         (list (subid-range (name "m")))
> +         (list (subid-range (name "x")))
> +         "Loaded ranges are supposed to have a start, but at least one does not.")
> +        (list
> +         "ranges must fall within allowed max min subids"
> +         (list (subid-range (name "m")
> +                            (start (- %subordinate-id-min 1))
> +                            (count
> +                             (+ %subordinate-id-max %subordinate-id-min))))
> +         (list
> +          (subid-range (name "root") (start %subordinate-id-min)))
> +         "Subid range of m from 99999 to 600299998 spans over illegal subids.  Max allowed is 600100000, min is 100000."))))
> +
> +  ;; Make sure it's impossible to explicitly request impossible allocations
> +  (for-each
> +   (match-lambda
> +     ((test-name ranges current-ranges message)
> +      (test-assert (string-append "allocate-subids, impossible allocations - "
> +                                  test-name)
> +        (guard (c ((and (subordinate-id-range-error? c)
> +                        (string=? message (subordinate-id-range-error-message c)))
> +                   #t))
> +          (allocate-subids ranges current-ranges)
> +          #f))))
> +   inputs+currents))

This is hard to read.  It might be best to unroll the loop?

Also, I would check for ‘&subordinate-id-range-error’ details than for
messages: messages are for human beings, not for automated tests.

Thoughts?

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.