ssh: use standalone config file for security key support

[emaste]

Summary

An upcoming OpenSSH update has multiple config.h settings that change when builtin security key support is enabled or disabled. Prepare for this by setting ENABLE_SK_INTERNAL to a new sk_config.h header (as is done with krb5 support) and including that instead of defining the macro directly from CFLAGS.

[emaste]

For ssh 8.9p1 sk_config.h looks like:

#define ENABLE_SK_INTERNAL /**/
#define HAVE_FIDO_ASSERT_SET_CLIENTDATA 1
#define HAVE_FIDO_CRED_PROT 1
#define HAVE_FIDO_CRED_SET_CLIENTDATA 1
#define HAVE_FIDO_CRED_SET_PROT 1
#define HAVE_FIDO_DEV_GET_TOUCH_BEGIN 1
#define HAVE_FIDO_DEV_GET_TOUCH_STATUS 1
#define HAVE_FIDO_DEV_SUPPORTS_CRED_PROT 1

Now, we could in fact leave the HAVE_* macros defined always and continue to toggle only ENABLE_SK_INTERNAL, but (a) I think that would be more awkward to generate from freebsd-configure.sh and (b) it's not future proof against other sk-related changes.

Note that 8.9p1 will still need some work on freebsd-configure.sh because configure needs libcbor and libfido2 headers and libraries available. I generated the content above by adding CFLAGS=-I/usr/local/include and LDFLAGS=-I/usr/local/lib and installing matching libcbor and libfido2 versions from ports, but this isn't the right way to do it. This is made a bit more complicated by the fact that libfido2 and libcbor are built as private libs (e.g., /usr/lib/libprivatecbor.so) and the headers are not installed.

[kevans]

In D34407#779442, @emaste wrote:

For ssh 8.9p1 sk_config.h looks like:

#define ENABLE_SK_INTERNAL /**/
#define HAVE_FIDO_ASSERT_SET_CLIENTDATA 1
#define HAVE_FIDO_CRED_PROT 1
#define HAVE_FIDO_CRED_SET_CLIENTDATA 1
#define HAVE_FIDO_CRED_SET_PROT 1
#define HAVE_FIDO_DEV_GET_TOUCH_BEGIN 1
#define HAVE_FIDO_DEV_GET_TOUCH_STATUS 1
#define HAVE_FIDO_DEV_SUPPORTS_CRED_PROT 1

Now, we could in fact leave the HAVE_* macros defined always and continue to toggle only ENABLE_SK_INTERNAL, but (a) I think that would be more awkward to generate from freebsd-configure.sh and (b) it's not future proof against other sk-related changes.

Note that 8.9p1 will still need some work on freebsd-configure.sh because configure needs libcbor and libfido2 headers and libraries available. I generated the content above by adding CFLAGS=-I/usr/local/include and LDFLAGS=-I/usr/local/lib and installing matching libcbor and libfido2 versions from ports, but this isn't the right way to do it. This is made a bit more complicated by the fact that libfido2 and libcbor are built as private libs (e.g., /usr/lib/libprivatecbor.so) and the headers are not installed.

It sounds like we should go ahead and install the headers (INCSDIR will default to /usr/include/private) and add a configure target... somewhere... to abstract away these details.

[emaste]

add a configure target... somewhere...

We have crypto/openssh/freebsd-configure.sh and could add -I entries for /usr/include/private/cbor and /usr/include/private/libfido2 easily, but will still need to convince configure to add -lprivatecbor and -lprivatefido2.

configure does:

if test "x$use_pkgconfig_for_libfido2" = "xyes"; then
        LIBFIDO2=`$PKGCONFIG --libs libfido2`
        CPPFLAGS="$CPPFLAGS `$PKGCONFIG --cflags libfido2`"
else
        LIBFIDO2="-lfido2 -lcbor"
fi

The easiest path for 8.9p1 may just be patching configure.ac to specify -lprivatefido2; eventually I'd like to have pkgconf support for all of the base system and some sort of special mechanism to add private libs to the mix... but that can come later on. Anyhow short or longer term changes will come as part of the 8.9p1 update; I believe the first step (in this review) is appropriate regardless of what we end up doing.

[kevans]

In D34407#779445, @emaste wrote:

add a configure target... somewhere...

We have crypto/openssh/freebsd-configure.sh and could add -I entries for /usr/include/private/cbor and /usr/include/private/libfido2 easily, but will still need to convince configure to add -lprivatecbor and -lprivatefido2.

configure does:

if test "x$use_pkgconfig_for_libfido2" = "xyes"; then
        LIBFIDO2=`$PKGCONFIG --libs libfido2`
        CPPFLAGS="$CPPFLAGS `$PKGCONFIG --cflags libfido2`"
else
        LIBFIDO2="-lfido2 -lcbor"
fi

Right, so my silly idea here was that a configure target could, for instance, toss a couple linker scripts in .OBJDIR and call them libfido2 and libcbor as a naive shim, but

The easiest path for 8.9p1 may just be patching configure.ac to specify -lprivatefido2; eventually I'd like to have pkgconf support for all of the base system and some sort of special mechanism to add private libs to the mix... but that can come later on. Anyhow short or longer term changes will come as part of the 8.9p1 update; I believe the first step (in this review) is appropriate regardless of what we end up doing.

+1 pkgconf. Nevertheless, yeah, this looks to be the optimal approach for now.

[emaste]

Add sk_config.h to SRCS for the same reason as in e42070a701fc8:

bsd.lib.mk and bsd.prog.mk already depend all objs on headers in SRCS if
there is not yet a depend file.  The headers in SRCS are never built or
installed.  After 'make depend' the header was already added as a proper
dependency on the objects where needed.

[emaste]

The explicit dependency was added in R10:9fd9594daf9457ae7b72e53860d2534f4207a23d although it doesn't say why; I am not sure it's needed. Maybe it was necessary for the now-removed make depend only?

[imp]

secure/ssh.mk

8

You don't need this. We don't regenerate ssh_namespace.h each build, even though we have a script to do it.

13

This isn't needed either since this file is hand-curated.

[emaste]

Discussed with @imp on IRC, it appears SRCS+= sk_config.h is unnecessary. I will commit the earlier version.