[bug#61744] services: base: Deprecate 'pam-limits-service' procedure.

* doc/guix.texi (Base Services): Replace pam-limits-service with pam-limits-service-type.
* gnu/packages/benchmark.scm (python-locust)[description]: Update index anchor to manual.
* gnu/services/base.scm (pam-limits-service-type): Accept both lists and
file-like objects for compatibility.
(pam-limits-service): Deprecate procedure.
---

Sending this one for review now since this service is a bit unusual compared to the other ones.

 doc/guix.texi              | 18 ++++++++---------
 gnu/packages/benchmark.scm |  2 +-
 gnu/services/base.scm      | 41 +++++++++++++++++++++++++++-----------
 3 files changed, 39 insertions(+), 22 deletions(-)


base-commit: 5d10644371abd54d0edcd638691113f0a92de743

Hi,

thank you for the patches!

The effective change looks fine to me, but I’m confused about why these
are two patches.  The first one introduces this as an example in the
docs:

+(service
+  pam-limits-service-type
+  (plain-file
+    "limits.conf"
+    (string-join
+      (map pam-limits-entry->string
+        (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
+              (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
+      "\n")))

But the second removes this again in favour of this prettier form:

+(service pam-limits-service-type
+         (list
+          (pam-limits-entry "@@realtime" 'both 'rtprio 99)
+          (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))

Which is really close to the original form:

-(pam-limits-service
- (list
-  (pam-limits-entry "@@realtime" 'both 'rtprio 99)
-  (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))

Could you merge these two patches to reduce the number of unnecessary
changes?  I don’t think we should change to file-likes as the argument
value for the pam-limits-service-type.

Another thing that confused me:

+            (test-equal "/etc/security/limits.conf content matches"
+              #$(string-join (map pam-limits-entry->string pam-limit-entries)
+                             "\n" 'suffix)
+              (marionette-eval
+               '(call-with-input-file "/etc/security/limits.conf"
+                  get-string-all)
+               marionette))

Why use the gexp with a computed value here instead of using just the
plain text of the expected contents of that file?  Computing
the expected value in a test where the compared value is computed in the
same way feels like begging the question.

Or perhaps I’m misunderstanding something here?

Hi Ricardo,

On 2023-03-10 17:52, Ricardo Wurmus wrote:
> Hi,
> 
> thank you for the patches!
> 
> The effective change looks fine to me, but I’m confused about why these
> are two patches.  The first one introduces this as an example in the
> docs:

[...]

> 
> +(service
> +  pam-limits-service-type
> +  (plain-file
> +    "limits.conf"
> +    (string-join
> +      (map pam-limits-entry->string
> +        (list (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> +              (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> +      "\n")))
> 
> But the second removes this again in favour of this prettier form:

This was to ensure that each commit is "atomic".

> 
> +(service pam-limits-service-type
> +         (list
> +          (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> +          (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> 
> Which is really close to the original form:
> 
> -(pam-limits-service
> - (list
> -  (pam-limits-entry "@@realtime" 'both 'rtprio 99)
> -  (pam-limits-entry "@@realtime" 'both 'memlock 'unlimited)))
> 
> Could you merge these two patches to reduce the number of unnecessary
> changes?  I don’t think we should change to file-likes as the argument
> value for the pam-limits-service-type.
The v2 patch-series are a dis-aggregation of the v1 series (save for a bug fix
in the match clauses, test suite and using raise instead of report-error)
as indicated in the 10/27 patch-series review from #61789.

> 
> Another thing that confused me:
> 
> +            (test-equal "/etc/security/limits.conf content matches"
> +              #$(string-join (map pam-limits-entry->string pam-limit-entries)
> +                             "\n" 'suffix)
> +              (marionette-eval
> +               '(call-with-input-file "/etc/security/limits.conf"
> +                  get-string-all)
> +               marionette))
> 
> Why use the gexp with a computed value here instead of using just the
> plain text of the expected contents of that file?  Computing
> the expected value in a test where the compared value is computed in the
> same way feels like begging the question.
> 
> Or perhaps I’m misunderstanding something here?
> 

I wrote this test suite to simply check that both deprecated and "new" service-type forms
work correctly, i.e. the files are present in their locations.
(this actually caught a bug within the match clauses in the v1 patch)


Cheers,
Bruno

Hi Bruno,

Thanks for this great and important work!

Can we refer to the limits.conf file in the store, please? I do not
believe we need a copy in /etc/security, and should not keep one
there.

The "conf=" argument to pam_limits(8) accepts an absolute path. [1] We
use that mechanism already (for the default path). Thanks!

Kind regards,
Felix Lechner

[1] https://linux.die.net/man/8/pam_limits

Hi Felix,

Felix Lechner <felix.lechner@lease-up.com> skribis:

> Can we refer to the limits.conf file in the store, please? I do not
> believe we need a copy in /etc/security, and should not keep one
> there.

I’m generally in favor of not populating /etc and instead referring to
store file names.

In some cases (maybe this one), this can be a problem though, in
particular for upgrades (the module keeps referring to the old config
file in the store).  So I don’t know, but this needs to be taken into
account.

Ludo’.

Hi Bruno,

Thanks for explaining.  It seems to me that none of the issues raised is
a blocker, so I went ahead and applied these two patches.

Thank you, and apologies for the delay!

Ludo’.

Hi Ludovic,

On Thu, Mar 30, 2023 at 1:54 PM Ludovic Courtès <ludo@gnu.org> wrote:
>
> In some cases (maybe this one), this can be a problem

Thanks for pointing that out! I would like to learn more about that.

My next suggestion would have been to refer to the core PAM modules,
which ship with Linux-PAM, by absolute paths as well.

You can see the current inconsistencies in my PAM 'login' service,
which I included below. Which breakage do you expect?

On a side note, I am also working with the pam_mount maintainer on a
store path for /etc/security/pam_mount_conf.xml. [1] (Jan previously
accepted another suggestion of mine, and it became popular with
users.) Then we can drop the definition of 'greet-pam-mount' [2] which
is very nearly a duplicate of the regular 'pam-mount'. [3]

Kind regards
Felix

[1] https://codeberg.org/jengelh/pam_mount/issues/1
[2] https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/admin.scm#n5314
[3] https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/admin.scm#n4709

* * *

account required pam_unix.so
auth required pam_unix.so nullok
auth optional /gnu/store/zb9ns323p7yv8m1m155yfgrxlxaadx3d-greetd-pam-mount-2.18/lib/security/pam_mount.so
disable_interactive
password required pam_unix.so sha512 shadow
session required
/gnu/store/7sq4qp09fl1pn72jw828ndm13nbpknhv-elogind-246.10/lib/security/pam_elogind.so
session required pam_limits.so conf=/etc/security/limits.conf
session optional pam_motd.so
motd=/gnu/store/mrk0km6gqw4zn20az2bqidvajps7yy93-motd
session required pam_loginuid.so
session required pam_env.so
session required pam_unix.so
session optional
/gnu/store/zb9ns323p7yv8m1m155yfgrxlxaadx3d-greetd-pam-mount-2.18/lib/security/pam_mount.so
disable_interactive