GNU bug report logs - #67555
[PATCH 0/2] Add Heimdal Kerberos system services.

Previous Next

Full log

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

From: Bruno Victal <mirai <at> makinata.eu>
To: Felix Lechner <felix.lechner <at> lease-up.com>
Cc: 67555 <at> debbugs.gnu.org
Subject: Re: [bug#67555] [PATCH 2/2] services: kerberos/heimdal.scm: New file, 
 add Heimdal Kerberos services.
Date: Sat, 16 Dec 2023 21:35:16 +0000

[Message part 1 (text/plain, inline)]

Hi Felix,

On 2023-12-01 00:45, Felix Lechner wrote:
> +  (ports
> +   (list-of-strings '())
> +   "Ports to listen on.")

I'd prefer to use a list of exact-integers. (*)
Hint: you can use the procedures in (gnu services configuration)
to define this predicate with (list-of exact-integer?).

> +  (disable-des?
> +   (boolean #f)
> +   "Disable all DES encryption types."))

I'd avoid the double negative here, i.e. by naming this enable-des?.
Another note, how about defaulting to disabled DES support
to discourage its use?

> +     (start #~(make-forkexec-constructor
> +               (list #$(file-append heimdal "/libexec/kdc")
> +                     #$@(if (maybe-value-set? config-file)
> +                            `(,(string-append "--config-file=" (maybe-value config-file)))
> +                            '())

Simply do:
`(,(string-append "--config-file=" config-file))

You don't need to use 'maybe-value' to extract the value if
you've already tested it with 'maybe-value-set?'.
> +               #:log-file "/var/log/kdc-shepherd"))

I'd make this configurable in <heimdal-kdc-configuration>.

> +  (ports
> +   (list-of-strings '())
> +   "Ports to listen on."))

See (*).

> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2017 Peter Mikkelsen <petermikkelsen10 <at> gmail.com>
> +;;; Copyright © 2022 Bruno Victal <mirai <at> makinata.eu>

Copy-paste leftovers perhaps? 😅

> new file mode 100644
> index 0000000000..b6424ace9e
> --- /dev/null
> +++ b/gnu/tests/heimdal-kdc.scm

How about merging these tests under a single gnu/tests/krb-heimdal.scm
instead of splitting them as gnu/tests/heimdal-kadmind.scm and
gnu/tests/heimdal-kadmind.scm?

If you're up for it I'd love to see one more test (might
involve multiple VMs) that actually tests the kerberos integration.
(i.e. performs an actual kerberos test)
That way we could be at least sure that there's a working kerberos
setup that we can use as a reference point for documentation/cookbooks.

My 2¢!

-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

[OpenPGP_signature.asc (application/pgp-signature, attachment)]

Previous Next

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