Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Makefile target for sk-libfido2.so #524

Closed
wants to merge 1 commit into from

Conversation

stoggi
Copy link
Contributor

@stoggi stoggi commented Oct 19, 2024

Compiles the standalone FIDO2 security key shared library, suitable for use with the SecurityKeyProvider option. misc.h is required even when SK_STANDALONE is defined, because of the use of monotime_tv in sk_select_by_touch.

The distributed OpenSSH client in macOS is compiled with security key support, but without linking against libfido2. As a result, on the latest versions of macOS (15.0.1), *-sk key types do not work out of the box without an external SecurityKeyProvider library (Yubico/libfido2#464). This library used to be provided by libfido2, but was removed a number of years ago because the code was included upstream in OpenSSH (https://github.com/Yubico/libfido2/blob/e1c761a9a03ffe95fbb11367252a0d091ea6a1d5/NEWS#L127).

OpenSSH has all of the code needed to produce the shared library to support usb FIDO2 devices via the SecurityKeyProvider option that is normally builtin. It is just missing the Makefile target to compile it. Ultimately I would like to add a official homebrew recipe for sk-libfido2, that only depends on the OpenSSH source code.

Thanks to @scottgeary for help with compiling and testing.

@stoggi
Copy link
Contributor Author

stoggi commented Oct 25, 2024

Having thought about this a little more I have two followup ideas:

1. Cross platform shared library extension

It would be nice if the sk-libfido2.so target had a cross platform extension (.so, .dylib, .dll). From what I can see there isn't a SHLIBEXT feature of autoconf similar to EXEEXT, so we could define SHLIBEXT in configure.ac in each of the platform case statements.

For example:

*-*-darwin*)
    SHLIBEXT=".dylib"
    [...]
    ;;
*-*-linux*)
    SHLIBEXT=".so"
    [...]
    ;;

And then in the Makefile.in, we could use this defined variable in the target:

sk-libfido2$(SHLIBEXT): sk-usbhid.c $(LIBCOMPAT) libssh.a
	$(CC) -o $@ -shared $(CFLAGS_NOPIE) $(CPPFLAGS) [...]

Note

An alternative simple approach would be to just use a wildcard as the extension in the target sk-libfido.% and let the user define their extension when invoking make with make sk-libfido2.dylib or as part of a configure option below. Wildcards are not used for other targets, and a more explicit approach above seems more appropriate.

2. Include target in test builds and add configure option

Even though many users will not need the standalone SK library, it would still be great if it compiled as part of the test builds, and potentially could be included as a target if configured with a new option like:

./configure --with-security-key-standalone

This new option would then include the correct platform specific library when running make. This option would not need to be mutually exclusive with --with-security-key-builtin, and would not define -DSK_STANDALONE for all other targets.

To implement this, a target like sk-standalone could be added to $(TARGETS) which depending on the configure option includes the sk-libfido2.$(SHLIBEXT) target as a depenency.

@stoggi
Copy link
Contributor Author

stoggi commented Oct 27, 2024

Hmm, seems to be failing on x86_64 when linking against libssh.a, as it wasn't compiled with position independent code flag. I was compiling and testing on arm64 which must have a different position independent code implementation.

/usr/bin/ld: ./libssh.a(misc.o): relocation R_X86_64_PC32 against symbol `stderr@@GLIBC_2.2.5' can not be used when making a shared object; recompile with -fPIC

https://github.com/openssh/openssh-portable/actions/runs/11529618399/job/32098242170?pr=524#step:10:762

But if all linked libraries need to also be compiled with -fPIC, this makes the change not such a simple change to the Makefile. I'll have to rethink the approach here. Any suggestions?

@djmdjm
Copy link
Contributor

djmdjm commented Nov 7, 2024

We already have some hacky makefile rules to build sk-dummy.so with -fPIC, if you can make a version of
LIBSSH_OBJS with s/.o/.lo/ and build a libssh.la from those then that might work...

@djmdjm
Copy link
Contributor

djmdjm commented Nov 7, 2024

When I say "make a version of LIBSSH_OBJS" ideally this would be derived from the existing LIBSSH_OBJS rather than a separate thing that needs to be maintained in parallel. idk how to do that in portable makefile off the top of my head unfortunately.

@djmdjm
Copy link
Contributor

djmdjm commented Nov 7, 2024

actually, something like this syntax might do the trick:

LIBSSH_OBJS=foo.o
LIBSSH_PIC_OBJS=$(LIBSSH_OBJS:.o=.lo)

all:
        echo $(LIBSSH_PIC_OBJS)

It's portable between GNU and BSD make at least, though it might be problematic on other make implementations.

@stoggi
Copy link
Contributor Author

stoggi commented Nov 12, 2024

Thank you for the suggestions @djmdjm! I attempted to use the LIBSSH_PIC_OBJS method you suggested above. Note I still needed to add a static library libssh-pic.a instead of adding all the objects directly to the sk-libfido2 compilation step because defining -DSK_STANDALONE removed a symbol needed by sshkey.lo.

Seems to compile OK now: https://github.com/openssh/openssh-portable/actions/runs/11794137749/job/32851155604?pr=524#step:10:1012

It feels a bit messy, what do you reckon?

@djmdjm
Copy link
Contributor

djmdjm commented Nov 14, 2024

This looks pretty reasonable. Could you squash the commits into a single commit? (Otherwise I can)

@djmdjm
Copy link
Contributor

djmdjm commented Nov 14, 2024

@daztucker FYI

…key shared library, suitable for use with the SecurityKeyProvider option.

Add a new configure option `--with-security-key-standalone` that optionally sets the shared library target sk-libfido2$(SHLIBEXT), and adds it to $(TARGETS).
misc.h is required when SK_STANDALONE is defined, because of the use of `monotime_tv` in `sk_select_by_touch`.
Sets the shared library extension for sk-libfido2 is by setting `SHLIBEXT` depending on the platform in configure.ac.
Add the shared library to the CI builds in the `sk` target config to make sure it can compile under the same conditions as `--with-security-key-builtin`.
Add a libssh-pic.a static library that compiles with `-fPIC` reusing .c.lo method in sk-dummy.so for use in the shared library sk-libfido2.
Note, a separate static library libssh-pic.a is needed, since defining -DSK_STANDALONE excludes some symbols needed in sshkey.lo.
@djmdjm
Copy link
Contributor

djmdjm commented Nov 28, 2024

Merged as ca0697a - thanks!

@djmdjm djmdjm closed this Nov 28, 2024
@stoggi
Copy link
Contributor Author

stoggi commented Nov 28, 2024

Great! Thank you for your help @djmdjm 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants