Re: [Buildroot] [PATCH 1/1] package/certmonger: add certmonger package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Ayrton,

Thanks for your patch! Here as well, I have a number of comments.

First, same as the other patch, the commit title should be:

	package/certmonger: new package

On Mon, 21 Oct 2024 11:56:54 +0000
Ayrton Leyssens <aleyssens@idtech.be> wrote:

> Added support for the certmonger package (version 0.79.20).
> Certmonger is used to monitor certificates and can refresh the certificate with the help of a CA.
> In today's age, cybersecurity is a critical need for network connected devices.
> Certmonger can help maintain all the certificate stuff so the user itself has less to worry about.

Please wrap the commit log to ~80 columns.

I think the sentence "In today's age, cybersecurity is a critical need
for network connected devices." is quite useless in such a commit log.

> Signed-off-by: Ayrton aleyssens@idtech.be<mailto:aleyssens@idtech.be>

Please fix your Signed-off-by line, like explained on the libevent
patch.

> ---
> package/Config.in                  |  1 +
> package/certmonger/Config.in       | 19 +++++++++++++++++++
> package/certmonger/certmonger.hash |  2 ++
> package/certmonger/certmonger.mk   | 25 +++++++++++++++++++++++++
> 4 files changed, 47 insertions(+)

Please add an entry in the DEVELOPERS file.

> create mode 100644 package/certmonger/Config.in
> create mode 100644 package/certmonger/certmonger.hash
> create mode 100644 package/certmonger/certmonger.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index d4cc03dcdb..73efaae622 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -2688,6 +2688,7 @@ endif
>                source "package/zabbix/Config.in"
>                source "package/zeek/Config.in"
>                source "package/znc/Config.in"
> +             source "package/certmonger/Config.in"

Indentation and alphabetic ordering should be fixed here.

>  endmenu
> diff --git a/package/certmonger/Config.in b/package/certmonger/Config.in
> new file mode 100644
> index 0000000000..9d0e112b5a
> --- /dev/null
> +++ b/package/certmonger/Config.in
> @@ -0,0 +1,19 @@
> +config BR2_PACKAGE_CERTMONGER
> +             bool "Certmonger"
> +             select BR2_PACKAGE_LIBTALLOC
> +             select BR2_PACKAGE_LIBTEVENT
> +             select BR2_PACKAGE_DBUS
> +             select BR2_PACKAGE_LIBNSS
> +             select BR2_PACKAGE_LIBXML2
> +             select BR2_PACKAGE_LIBCURL
> +             select BR2_PACKAGE_JANSSON
> +             select BR2_PACKAGE_LIBKRB5
> +             select BR2_PACKAGE_POPT
> +             select BR2_PACKAGE_LIBOPENSSL
> +             select BR2_PACKAGE_POPT
> +             select BR2_PACKAGE_OPENLDAP
> +             help

Indentation of bool, select and help should be one tab.

The select entries should be alphabetically sorted.

Also, are you sure all those dependencies are mandatory dependencies,
i.e certmonger cannot be built without them?

Finally, many of those options that you select have "depends on". You
need to replicate them here. For example, BR2_PACKAGE_OPENLDAP has:

        depends on BR2_USE_WCHAR
        depends on BR2_USE_MMU # needs fork()

so you need to add:

        depends on BR2_USE_WCHAR # openldap
        depends on BR2_USE_MMU # openldap

to this BR2_PACKAGE_CERTMONGER option. And do that for all other
options you are select-ing.


> +               Certmonger is a service which attempts to simplify
> +               interaction with certifying authorities (CAs) on
> +               networks which use public-key infrastructure (PKI).

Indentation of help text should be one tab + two spaces.

(Make sure to run "make check-package" to find all those small coding
style details).

The help text should also be followed by the upstream URL of the
project, see all other packages.

> +
> diff --git a/package/certmonger/certmonger.hash b/package/certmonger/certmonger.hash
> new file mode 100644
> index 0000000000..653ba1dcfc
> --- /dev/null
> +++ b/package/certmonger/certmonger.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256  1e66094ef4130fc15c8acf5abfca097c2f8f40f57f699f80bb14e9941260d473  certmonger-0.79.20-git4.tar.gz

Please add the hash of the license file.

> diff --git a/package/certmonger/certmonger.mk b/package/certmonger/certmonger.mk
> new file mode 100644
> index 0000000000..c6a9c179e4
> --- /dev/null
> +++ b/package/certmonger/certmonger.mk
> @@ -0,0 +1,25 @@
> +################################################################################
> +#
> +# CERTMONGER

lower case.

> +#
> +################################################################################
> +
> +CERTMONGER_VERSION = 0.79.20
> +CERTMONGER_SITE = https://pagure.io/certmonger.git
> +CERTMONGER_LICENSE_FILES = LICENSE

Please add CERTMONGER_LICENSE, which it seems is "GPL-3.0+ with OpenSSL
exception".

> +CERTMONGER_NAME = certmonger

Not useful.

> +CERTMONGER_SITE_METHOD = git
> +CERTMONGER_DEPENDENCIES = openssl dbus libtalloc libtevent libnss libcurl jansson libkrb5 openldap popt libxml2

Please sort alphabetically.

> +
> +MY_CFLAGS = $(TARGET_CFLAGS) -lssl -ldbus-1 -ltalloc -ltevent -lnss -lcurl -ljansson -lkrb5 -lldap -lpopt -lxml2 -lcom_err

This variable shouldn't be named MY_CFLAGS: all variables in a package
should start with the package name, i.e here CERTMONGER_<something>.
However, why do you need this special CFLAGS variable? It only contains
libraries that should be detected by the configure script of
certmonger, so this variable really shouldn't be needed.

> +define CERTMONGER_RUN_AUTOGEN
> +             cd $(@D) && PATH=$(BR_PATH) ./autogen.sh
> +endef
> +CERTMONGER_PRE_CONFIGURE_HOOKS += CERTMONGER_RUN_AUTOGEN

Please use instead CERTMONGER_AUTORECONF = YES instead. Indeed what you
did doesn't work, because it doesn't add host-autoconf, host-automake
and host-libtool to certmonger's dependencies.

> +define CERTMONGER_BUILD_CMDS
> +             $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) CXX="$(TARGET_CXX)" CFLAGS="$(MY_CFLAGS)"

Why is this needed?

certmonger seems like a quite complicated package. Is there a way to
easily test it? If so, it would be good to add a runtime test in
support/testing/.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot