FTR: https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslcarevocationcheck with chain, leaf, none.

In D47433#1082495, @franco_opnsense.org wrote:

In D47433#1082491, @michaelo wrote:

I don't disagree, but introducing multiple vars for the same config isn't better either in my opinion. Consider you want to expose that to the CLI for fetch(1), do you want to introduce multiple switches?

For historic context: right now handling of X509_V_ERR_UNABLE_TO_GET_CRL / D47449 is unconditional for OPNsense due to lack of the scope of this patch here. For FreeBSD inclusion I pondered the side effect of introducing this breaking standard verification behaviour of SSL_CRL_FILE and there it would also be beneficial.

In D47433#1082489, @franco_opnsense.org wrote:

In D47433#1082488, @michaelo wrote:

Well, then maybe SSL_VERIFY_CRL should not be boolean, but rather an enum? E.g, optional, yes, much like https://httpd.apache.org/docs/current/mod/mod_ssl.html#sslverifyclient because it the end it will require more and more flags. Default value would be none/NULL.

Also doable, but personally I dislike the fuzzy matching on the value to act according to user (case sensitivity and ambiguity of yes and no etc and garbage input). The vars in libfetch are set and forget, if referencing a file or dir letting other parts deal with the complexity of the validation too.

In D47433#1082487, @franco_opnsense.org wrote:

In D47433#1082486, @michaelo wrote:

Like fine, but then CR, not CRL because we don't verify the list, do we? :-D Since it is a *verbose* flag I don't mind being verbose literally.

Technically the list's signature and expiry is verified as well but we could also call it a "check" but then the env var should be renamed for clarity as well? Already expected the naming aspect of it to be difficult but I agree that it should be as good as it can be since it will likely stay that way.

In D47433#1082485, @franco_opnsense.org wrote:

Oh about SSL_VERIFY or SSL_CRL I'm not sure. Keeping it closer to SSL_CRL_FILE may be more beneficial also with SSL_CRL_OPTIONAL in mind later. Don't want these vars too long if it can be avoided and cluster all CRL into SSL_CRL prefix?

In D47433#1082484, @franco_opnsense.org wrote:

In D47433#1082483, @michaelo wrote:

WDYT?

I'm ok with that, maybe with brevity in mind just this:

CRL verification enabled

But I don't mind either way.

I see inconsistency in env vars and in output:

I have now played around with the patch and one of our intermediate CAs:

While testing this, do you intend to add a flag to fetch(1) as well? E.g., --crl-verify?

I think I have found it, the documentation isn't really good in this case for both SSL_CTX_load_verify_locations() and SSL_CTX_set_default_verify_paths(). If a hashed dir is passed it boils down to https://github.com/openssl/openssl/blob/ccaa754b5f66cc50d8ecbac48b38268e2acd715e/crypto/x509/x509_d2.c#L73-L76 where the manpage says:

X509_LOOKUP_add_dir() passes a directory specification from which certificates and CRLs are loaded on demand into the associated X509_STORE. type indicates what type of object is expected. This can only be used with a lookup using the implementation X509_LOOKUP_hash_dir(3).

Will happily look at this, already have a few questions. Will chime in later this week.

Change merged.

Didn't notice that the devel port is maintained by @driesm ...

No objections received, maintainer timeout. Will go ahead and merge it.

In D47164#1075300, @jrm wrote:

Assuming @kevans or @des don't chime in to say something is missing in the base certificate store, lgtm.

This looks like a stupid typo, no?

In D46807#1067249, @jrm wrote:

FYI, when I was testing, I set WITH_SUBVERSION_VER to lts instead of LTS and it wasn't handled as would be expected after a look in devel/subversion/Makefile.addons.

jrm@ser /usr/local/etc/poudriere.d % s poudriere testport -i -j 15amd64 devel/py-subversion
[00:00:00] Creating the reference jail... done
[00:00:00] Mounting system devices for 15amd64-default
[00:00:00] Stashing existing package repository
[00:00:00] Mounting ccache from: /usr/local/poudriere/ccache/15amd64
[00:00:00] Mounting ports from: /usr/ports
[00:00:00] Mounting packages from: /usr/local/poudriere/data/packages/15amd64-default
[00:00:00] Mounting distfiles from: /usr/ports/distfiles
[00:00:00] Copying /var/db/ports from: /usr/local/etc/poudriere.d/15amd64-options
[00:00:00] Appending to make.conf: /usr/local/etc/poudriere.d/15amd64-make.conf
/etc/resolv.conf -> /usr/local/poudriere/data/.m/15amd64-default/ref/etc/resolv.conf
[00:00:00] Starting jail 15amd64-default
Updating /var/run/os-release done.
[00:00:00] Will build as nobody:nobody (65534:65534)
[00:00:01] Ports supports: FLAVORS SUBPACKAGES SELECTED_OPTIONS
[00:00:01] Acquiring build logs lock for 15amd64-default... done
[00:00:01] Logs: /usr/local/poudriere/data/logs/bulk/15amd64-default/2024-09-27_10h24m46s
[00:00:01] WWW: http://pkg.ftfl.ca/build.html?mastername=15amd64-default&build=2024-09-27_10h24m46s
[00:00:01] Loading MOVED for /usr/local/poudriere/data/.m/15amd64-default/ref/usr/ports
[00:00:01] Gathering ports metadata
[00:00:01] Warning: (devel/py-subversion): make: "/usr/ports/Mk/bsd.port.mk" line 1859: Malformed conditional (${WITH_PKG} == devel)
[00:00:01] Warning: (devel/py-subversion): in /usr/share/mk/bsd.port.mk:27
[00:00:01] Warning: (devel/py-subversion): in /usr/share/mk/bsd.port.post.mk:4
[00:00:01] Warning: (devel/py-subversion): in /usr/ports/devel/py-subversion/Makefile:34
[00:00:01] Warning: (devel/py-subversion): make: "/usr/ports/Mk/bsd.ccache.mk" line 78: Malformed conditional (!defined(NO_CCACHE_DEPEND) &&  ${PKGORIGIN} != ${PKG_ORIGIN})
[00:00:01] Warning: (devel/py-subversion): in /usr/ports/Mk/bsd.port.mk:2082
[00:00:01] Warning: (devel/py-subversion): in /usr/share/mk/bsd.port.mk:27
[00:00:01] Warning: (devel/py-subversion): in /usr/share/mk/bsd.port.post.mk:4
[00:00:01] Warning: (devel/py-subversion): in /usr/ports/devel/py-subversion/Makefile:34
[00:00:01] Warning: (devel/py-subversion): make: "/usr/ports/Mk/bsd.port.mk" line 3445: Malformed conditional (${PKGORIGIN} == "ports-mgmt/pkg" || ${PKGORIGIN} == "ports-mgmt/pkg-devel")
[00:00:01] Warning: (devel/py-subversion): in /usr/share/mk/bsd.port.mk:27
[00:00:01] Warning: (devel/py-subversion): in /usr/share/mk/bsd.port.post.mk:4
[00:00:01] Warning: (devel/py-subversion): in /usr/ports/devel/py-subversion/Makefile:34
[00:00:01] Warning: (devel/py-subversion): make: Fatal errors encountered -- cannot continue
[00:00:01] Warning: (devel/py-subversion): Error: Error looking up dependencies for devel/py-subversion
[00:00:01] Error: /usr/local/share/poudriere/testport.sh:gather_port_vars:183:Fatal errors encountered gathering ports metadata
[15amd64-default] [2024-09-27_10h24m46s] [crashed] Time: 00:00:00
[00:00:01] Logs: /usr/local/poudriere/data/logs/bulk/15amd64-default/2024-09-27_10h24m46s
[00:00:01] WWW: http://pkg.ftfl.ca/build.html?mastername=15amd64-default&build=2024-09-27_10h24m46s
[00:00:01] Cleaning up
15amd64-default: removed
15amd64-default-n: removed
[00:00:01] Unmounting file systems

Process comments

Weird, the maintainer picked up PR, but totally ignored this review....closing.

In D46643#1064388, @diizzy wrote:

Friendly reminder that you need a PR in bugzilla for maintainer timeout to apply.

Guys, can we focus on the actual change. I agree with @jrm, that is makes little sense to waste cycles for ./configure. I will provide this change with a separate PR.
There is only one way to make sure that an artifact ist authentic either X.509 or GPG.

Incorporate @diizzy's comments

In D46643#1063864, @diizzy wrote:

Any reason as to why we're not using upstream release archive?
https://github.com/msktutil/msktutil/releases/download/1.2.2/msktutil-1.2.2.tar.bz2
That will also likely get rid of USES= autoreconf

Other notes:
Remove GNU_CONFIGURE_MANPREFIX

Will wait for the maintainer timeout.

In D46643#1063059, @jrm wrote:

lgtm assuming all the necessary poudriere runs pass and @pi has no objections.

Issues addressed.

Add mat's comments

In D46096#1053797, @diizzy wrote:

Passes Poudriere and lgtm,

While out of scope for the PR perhaps it's time to retire the -lts port given upstream development?

In D46059#1053623, @mat wrote:

Something like this

Anyone else, any objections?

@diizzy , can you please have another sharp look at this?

@jrm, this happens when pkgconf is not present:

In D46059#1052323, @jrm wrote:

Should add portmgr as a reviewer? There may be a server-side git hook to ensure commits under Mk/ are approved by them.

Address diizzy's comment

Remove port option STATIC

Formatting

In D45873#1045890, @jrm wrote:

RFC 3361 declares 120 to be the code for the option, and this is doing the same thing as what's done for other unimplemented options, so this seems fine to me too. I'll add @brooks, a src committer who just touched dhclient, to be sure we're not missing something. Please make sure you use git commit --author to attribute Yuichiro NAITO in the commit logs.

Please also note the discussion in the PR: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=280119

Formatting again

Fix formatting

I'll give @ale the two-week timeout for maintainers. We still have time to next quarterly branch.

@mandree The text has been already reduced. Would you like to have it removed completely?

Incorporate Baptiste's proposal

In D45073#1028240, @jilles wrote:

The immediately preceding sentence about shell functions is an example of unexpected behaviour of set -e. The part "all commands of the function are considered to be tested as well" may result in code unexpectedly executing in the function and the function returning a different return status.

Many more examples can be found at https://mywiki.wooledge.org/BashFAQ/105 . Bash does not work exactly the same as our sh, but most of the problems are the same.

Bash's behaviour related to set -e has changed over the versions, but it has not succeeded in fixing the confusing parts. I don't believe it's possible to fix, and it is best to leave it unchanged so it is at least consistent.

set -e works reasonably well for scripts that are plain lists of commands with possible case, for, if, while and until, and only use && and || in conditions, which probably explains its use in make. However, even then, command substitution might lead to confusing results, for example because expr returns status 1 if the result is zero or the empty string, and the exit status from the inner command is passed through only in particular cases (for the last command substitution if there is no utility name).

A linting tool will probably be more effective (I've heard of ShellCheck, but I don't know whether it can catch missing error checks). With a linting tool, it matters less how beautiful the code looks but whether the tool can analyze it. Not enabling set -e makes the code easier to analyze, for example because functions don't behave differently depending on whether their return status is tested.

Another option is to use a better programming language. For a long time, there did not use to be one in the base system, but perhaps Lua is now a suitable option.