In D50020#1141667, @jhb wrote:

This isn't about namespacing though. Even without the namespacing you'd just have undefined symbols for the symbol without the prefix. Doesn't this just revert your earlier change though, and shouldn't we now drop all of the SKSRCS in all of the other Makefiles (not just the ones I added) since they are now redundant?

Minor typos.

This should fix sh freebsd-namespace.sh and technically, we can (should?) now revert a098111a28ed59e1ab1101ad09913f0235ebd28f.
In my opinion this is the least obstructive way forward.

Pedantically, to be on the same page in sshd_config(5):

1. The first configured value is the one that counts.
2. Some OSs (macOS)/Linux distros (Debian-based) just add Include /etc/ssh/sshd_config.d/* at the very top of the configuration file, so cloudinit should just create a new file under this directory with the desired settings, and call it a day. I only mention this option, as it is becoming popular (in case anyone has friends in openssh-portable ;-).
3. I would have rather/also liked to see something like:

# ssh_pwauth: true
printf "#PasswordAuthentication no # Should be ignored\n" > etc/ssh/sshd_config
printf "PasswordAuthentication no # Should change\n" >> etc/ssh/sshd_config
atf_check -o empty -e empty /usr/libexec/nuageinit "${PWD}"/media/nuageinit nocloud
atf_check -o match:"^#PasswordAuthentication no # Should be ignored$" \
    -o match:"^PasswordAuthentication yes$" cat etc/ssh/sshd_config

Regardless, the implementation seems correct.

In D49492#1139179, @bnovkov wrote:

In D49492#1131965, @jlduran wrote:

These are all good (at catching the current failures).
My only concern however, is that we should also check that makefs takes into consideration the timestamp in the mtree file, so the priorities (at this point of the revision stack) are:

1. -T flag .
2. Time in mtree file.

This guarantees (or not) documentation on the behavior regarding the priorities when the SOURCE_DATE_EPOCH environment variable is introduced later on (D49602).

Thank you for pointing this out and for sketching out the testcase!
After a brief discussion in D49531, the consensus was that the timestamp sources should be prioritized as follows:

1. -F mtree specfile time
2. -T
3. Timestamp in input mtree file

I've added testcases that test the interactions between -F and -T.

Additionally, if you really wan to keep the NANO_MEDIASIZE unchanged, you can also add WITHOUT_TOOLCHAIN=true.
For self reference, EC2 images are built with:

• WITHOUT_DEBUG_FILES=YES
• WITHOUT_KERNEL_SYMBOLS=YES
• WITHOUT_LIB32=YES
• WITHOUT_TESTS=YES
• WITHOUT_LLDB=YES

But these options would only fit with 4000000, changing the last option, WITHOUT_LLDB, to the entire WITHOUT_TOOLCHAIN, would make it still fit 2000000. But I don't know where to draw the line when removing things by default for NanoBSD.

If you have the cycles, I would prefer that the tests without tcptump are committed first (you can add me as Reviewed-by). And then submit the tcpdump part for review.
Note that I am not opposing to adding tcpdump to the tests. It is a great idea. It is just I'm not sure if the extra time it adds to each test is justified. Originally, the idea of the tests was to provide some level of confidence when unifying traceroute/traceroute6, and I believe the basic form (without tcpdump) achieves this.
Another option I see is that in the Reviewed-by trailer, you can add me with "(without tcpdump)".
I trust your judgement fully, so whatever option you choose to land this review will be fine by me. In the very rare event something wrong happens, we can always fix it post-commit.
Thank you!

In D49794#1135230, @emaste wrote:

Re progressmeter.c, OpenBSD includes it only in usr.bin/ssh/scp/Makefile and usr.bin/ssh/sftp/Makefile. I think it may be a bug in the portable-ization (from d194048f573136f1c2c34d25f951b78f4ffc7d77) that we can report to openssh-portable.

What about also adding:

WITHOUT_LIB32=true
WITHOUT_KERNEL_SYMBOLS=true

It should fit with:

NANO_MEDIASIZE=4000000

Reference: https://github.com/freebsd/freebsd-src/pull/964

I'll wait a few days for this one, upstream is already taking a look at the problem report.

• Remove gssapi_krb5 from LIBADD/_DP_ssh (before sorting out Mkaefile.inc1/tools/make_libdeps.sh).

I was not able to reproduce the failure. But it is safe to always have it, just like rm -f, when the user has -i aliased to rm and ln (e.g., `alias rm='rm -i' uncommented in the "be paranoid" section).
This change should not get reverted with D48782, as it does essentially the same thing.
Thank you!

PR number should be 284417.

Note to self: Update after D49873 (if accepted).

In D49794#1135292, @jlduran wrote:

• Remove experimental XMSS_SRCS (these should be committed separately, and guarded by WITH_XMSS)
• Remove progressmeter.c from libssh sources, it is not needed and has been reported upstream

Add missing libraries to libssh, otherwise (scp/sftp):

$ scp file.a user@server
ld-elf.so.1: /usr/lib/libprivatessh.so.5: Undefined symbol "GSS_C_NT_HOSTBASED_SERVICE"

Moved from https://github.com/freebsd/freebsd-src/pull/1652 per my request, as there is greater visibility here.

In D49832#1136272, @jlduran wrote:

If the prompt buffer overflows, fail authentication without prompting the user.

$ USER=$(printf "%266s" | tr " " a) telnet localhost
Trying ::1...
Connected to localhost.
Escape character is '^]'.
Trying SRA secure login:

If the prompt buffer overflows, fail authentication without prompting the user.

The GitHub issue should be:
https://github.com/containers/podman/issues/25270

Remove progressmeter.c from libssh sources.

• Remove experimental XMSS_SRCS (these should be committed separately, and guarded by WITH_XMSS)
• Remove progressmeter.c from libssh sources, it is not needed and has been reported upsttream

Hmm... OpenBSD uses "4555".
Abandoning this revision.

In D49794#1135230, @emaste wrote:

Re progressmeter.c, OpenBSD includes it only in usr.bin/ssh/scp/Makefile and usr.bin/ssh/sftp/Makefile. I think it may be a bug in the portable-ization (from d194048f573136f1c2c34d25f951b78f4ffc7d77) that we can report to openssh-portable.

In D49794#1135230, @emaste wrote:

Re progressmeter.c, OpenBSD includes it only in usr.bin/ssh/scp/Makefile and usr.bin/ssh/sftp/Makefile. I think it may be a bug in the portable-ization (from d194048f573136f1c2c34d25f951b78f4ffc7d77) that we can report to openssh-portable.

Address suggestions:

• Move $(SKSRCS) to our central location (secure/ssh.mk), matching upstream Makefile.in as close as possible

Match upstream Makefile.in as close as possible.

Match upstream Makefile.in as close as possible.

Hopefully this revision stack helps facilitate adapting OpenSSH 10.0p2 changes, and the new sshd-auth binary.

In D49742#1134602, @emaste wrote:

Upstream meaning OpenSSH or Clang?

I meant OpenSSH - i.e. is the problem in our headers or implementation, or is OpenSSH doing something strange? On one hand it does actually work with const so it seems to be the test is at fault.

In D49742#1134186, @emaste wrote:

I think this is fine, but we should have a comment explaining that this is working around a bug* and does not in fact disable hardening options in the FreeBSD build.

• we should have a FreeBSD bug report or upstream report for this issue and ideally reference it in the comment

Regarding const detection, this is one way out:
D49742
At the moment it only seems to affect SNPRINTF_CONST (in a positive way).

In D49739#1134004, @emaste wrote:

Thanks, updated and will update in a moment. There are two nits to sort out still in config.h generation:

--- a/crypto/openssh/config.h
+++ b/crypto/openssh/config.h
@@ -1863,14 +1863,10 @@
 #define SIZEOF_SHORT_INT 2
 
 /* The size of 'time_t', as computed by sizeof. */
-#ifdef __i386__
-#define SIZEOF_TIME_T 4
-#else
 #define SIZEOF_TIME_T 8
-#endif

When regenerating config.h (second commit), the HAVE_DECL_OPENSSL_IS_BORINGSSL and the HAVE_DECL_OPENSSL_NO_DSA stanzas should be removed as well.

This would break the test case mentioned in D49492, where makefs ignores the time in the mtree file.
If this is the desired behavior, the test should be updated accordingly.

These are all good (at catching the current failures).
My only concern however, is that we should also check that makefs takes into consideration the timestamp in the mtree file, so the priorities (at this point of the revision stack) are:

1. -T flag .
2. Time in mtree file.

This guarantees (or not) documentation on the behavior regarding the priorities when the SOURCE_DATE_EPOCH environment variable is introduced later on (D49602).
For example (very crude) just for FFS, but should be the similar for all cases:

# This helper function can live in makefs_tests_common.sh
change_mtree_timestamp()
{
	filename="$1"
	timestamp="$2"

Shouldn't src.conf.5 (make makeman) be regenerated after this change?

• Fix the procedure to use a commit id from zoulasc/blocklist as the version
• Fix cross-references in bin/blocklistd.8

Only for statistical reference:
https://ci.freebsd.org/job/FreeBSD-main-amd64-build/31456/warnings10Result/fixed/

In D49527#1129188, @imp wrote:

As for dates, you could use the raw date (%s) and add 1 day or 30 days to that.

In D49527#1129185, @jrtc27 wrote:

In D49527#1129162, @jlduran wrote:

In D49527#1129159, @jlduran wrote:

Would replacing:

START_DATE!=  $(printf "%s-%s-%0.2d\n" "$(date -u +%Y)" "$(date -u +%m)" $(( $(date -u +%d) - 1 )))
EXPIRY_DATE!= $(printf "%s-%0.2d-%s\n" "$(date -u +%Y)" $(( $(date -u +%m) + 1 )) "$(date -u +%d)")

in Makefile.azure avoid this? It is horrible, but portable.

Never mind, this would fail on certain dates.

Yeah and whilst date -d 'today + 1 month' is supported by GNU date, it's slightly different when dealing with invalid dates, it'll add on the number of days in the current month rather than clamping to the end of the next month (e.g. -v+1m on 31st January is 28th February, but -d today + 1 month is 3rd March). Though perhaps +1m is a bit odd to be using here, maybe we do in fact want, say, +28d or +30d, as having VM images built in February expire sooner than those built in March could be surprising and confusing.

In D49527#1129159, @jlduran wrote:

Would replacing:

START_DATE!=  $(printf "%s-%s-%0.2d\n" "$(date -u +%Y)" "$(date -u +%m)" $(( $(date -u +%d) - 1 )))
EXPIRY_DATE!= $(printf "%s-%0.2d-%s\n" "$(date -u +%Y)" $(( $(date -u +%m) + 1 )) "$(date -u +%d)")

in Makefile.azure avoid this? It is horrible, but portable.

Would replacing:

START_DATE!=  $(printf "%s-%s-%0.2d\n" "$(date -u +%Y)" "$(date -u +%m)" $(( $(date -u +%d) - 1 )))
EXPIRY_DATE!= $(printf "%s-%0.2d-%s\n" "$(date -u +%Y)" $(( $(date -u +%m) + 1 )) "$(date -u +%d)")

in Makefile.azure avoid this? It is horrible, but portable.

No markdown file.

Request the API globally.

In D49517#1129003, @emaste wrote:

What do you think of just adding it in secure/ssh.mk?

This particular probe is not working.
Sorry for the noise.

I forget that @christos_netbsd.org can be conveniently tagged here:

Update missing patches to be upstreamed.

Update patch, upstream has accepted our fixes.

This patch has been committed upstream, and will land with the normal vendor import procedure.

I also don't know if there is a trailer for that, but I think it is appropriate to write a few words thanking cperciva for the donation of cloud resources. That allowed us to test and discover a few missing probes, by exposing these ports to the public and examining the logs.

A fix has already been committed upstream.

In D49398#1126583, @emaste wrote:

I think this is fine. backticks and checkyesno moving are minor improvements (arguably not bugs).

I think the problem could also be fixed with [ "$(echo ${MODULE_DIR}/*.ko)" != "${MODULE_DIR}/*.ko" ] if we want to continue avoiding the extra process invocation. Is there a PR for it?

Note to self: isdigit(3) also needs a little fix.

Address suggestions: