Skip to content

openssh: Add FIDO2 hardware token support#14415

Merged
neheb merged 3 commits intoopenwrt:masterfrom
linosgian:openssh_add_fido2_support
Jan 8, 2021
Merged

openssh: Add FIDO2 hardware token support#14415
neheb merged 3 commits intoopenwrt:masterfrom
linosgian:openssh_add_fido2_support

Conversation

@linosgian
Copy link
Contributor

@linosgian linosgian commented Jan 3, 2021 •
edited
Loading

Maintainer: me
Compile tested: x86-64/generic/snapshot(c5d033a)
Run tested: x86-64/generic/snapshot(c5d033a) - Tested this end-to-end with a hardware token for the ecdsa-sk type

Description:

Version 8.2 added support for two new key types: "ecdsa-sk" and
"ed25519-sk". These two types enable the usage of hardware tokens that
implement the FIDO (or FIDO2) standard, as an authentication method for
SSH.

Since we're already on version 8.4 all we need to do is to explicitly enable
the support for hardware keys when compiling OpenSSH and add all the
missing dependencies OpenSSH requires.

OpenSSH depends on libfido2, to communicate with the FIDO devices
over USB. In turn, libfido2 depends on libcbor, a C implementation of
the CBOR protocol and OpenSSL.

I've also compiled openssh-server for 19.07.04 (ramips/mt7621) and I've been using the new ecdsa-sk type for a while now.

@linosgian
Copy link
Contributor Author

linosgian commented Jan 3, 2021

cc'ing @tripolar as he's the maintainer of the openssh package

@linosgian
Copy link
Contributor Author

linosgian commented Jan 3, 2021 •
edited
Loading

It seems like the failing builds are due to musl being used instead of glibc.

libfido2 includes #include <linux/hidraw.h> which defines the constants that are passed to ioctl as unsigned ints.
Glibc defines ioctl as ioctl (int fd, unsigned long int request, ...) whereas musl defines it as: ioctl(int fd, int request), hence the implicit conversion.

I am not sure what would be the "correct" solution to this issue.. Apart from -Wno-error=-sign-conversion

@linosgian linosgian force-pushed the openssh_add_fido2_support branch from a9028b8 to da21b71 Compare January 4, 2021 18:33
@neheb
Copy link
Contributor

neheb commented Jan 6, 2021

Commit subject line MUST start with ': ' (fixup! openssh: Add FIDO2 hardware token support)

Remove fix up!

@linosgian linosgian force-pushed the openssh_add_fido2_support branch from da21b71 to e91cc9c Compare January 6, 2021 01:26
@neheb
Copy link
Contributor

neheb commented Jan 6, 2021

Alright...

Please split up the commits for each package added/edited.

The OpenSSH change requires a version bump of PKG_RELEASE.

neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
libs/libfido2/Makefile

PKG_FORTIFY_SOURCE:=0

TARGET_CFLAGS += -Wno-error=overflow -Wno-error=sign-conversion
Copy link
Contributor

@neheb neheb Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pass -Wno-error

Copy link
Contributor Author

@linosgian linosgian Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to get overridden by explicit -Werror=foo flags (e.g. -Werror=overflow).
It currently breaks all the PR checks.

An excerpt from a failing build:

cd /home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0/src && /home/build/openwrt/staging_dir/toolchain-aarch64_cortex-a53_gcc-8.4.0_musl/bin/aarch64-openwrt-linux-musl-gcc -DHAVE_CLOCK_GETTIME -DHAVE_DEV_URANDOM -DHAVE_ENDIAN_H -DHAVE_ERR_H -DHAVE_EXPLICIT_BZERO -DHAVE_GETLINE -DHAVE_GETOPT -DHAVE_GETPAGESIZE -DHAVE_GETRANDOM -DHAVE_SIGNAL_H -DHAVE_STRLCAT -DHAVE_STRLCPY -DHAVE_SYSCONF -DHAVE_SYS_RANDOM_H -DHAVE_UNISTD_H -DSIGNAL_EXAMPLE -DTLS=__thread -D_DEFAULT_SOURCE -D_FIDO_INTERNAL -D_FIDO_MAJOR=1 -D_FIDO_MINOR=6 -D_FIDO_PATCH=0 -D_GNU_SOURCE -I/home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0/src -Os -pipe -mcpu=cortex-a53 -fno-caller-saves -fno-plt -fhonour-copts -Wno-error=unused-but-set-variable -Wno-error=unused-result -Wno-error -ffile-prefix-map=/home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0=libfido2-1.6.0 -Wformat -Werror=format-security -fstack-protector -Wl,-z,now -Wl,-z,relro -Wall -Wextra -Werror -Wshadow -Wwrite-strings -Wmissing-prototypes -Wbad-function-cast -pedantic -pedantic-errors -fstack-protector-all -std=c99 -Wno-unused-result -Wcast-qual -DNDEBUG -D_FORTIFY_SOURCE=2 -fPIC -Wconversion -Wsign-conversion -o CMakeFiles/fido2.dir/hid_linux.c.o -c /home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0/src/hid_linux.c
/home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0/src/hid_linux.c: In function 'get_report_descriptor':
/home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0/src/hid_linux.c:30:16: error: overflow in conversion from 'long unsigned int' to 'int' changes value from '2147764225' to '-2147203071' [-Werror=overflow]
  if (ioctl(fd, HIDIOCGRDESCSIZE, &s) < 0 || s < 0 ||
                ^~~~~~~~~~~~~~~~
/home/build/openwrt/build_dir/target-aarch64_cortex-a53_musl/libfido2-1.6.0/src/hid_linux.c:38:16: error: overflow in conversion from 'long unsigned int' to 'int' changes value from '2416199682' to '-1878767614' [-Werror=overflow]
  if (ioctl(fd, HIDIOCGRDESC, hrd) < 0) {
                ^~~~~~~~~~~~
cc1: all warnings being treated as errors

Copy link
Contributor

@neheb neheb Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this briefly. This is a bug and should be fixed by not including sys/ioctl.h

Copy link
Contributor

@neheb neheb Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reported upstream Yubico/libfido2#259

Patch is included if you want to port it here.

Copy link
Contributor Author

@linosgian linosgian Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we address this in another PR, once it's merged upstream?

neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
@linosgian linosgian force-pushed the openssh_add_fido2_support branch from e91cc9c to f5d2053 Compare January 6, 2021 21:58
neheb
neheb reviewed Jan 6, 2021
View reviewed changes
@linosgian linosgian force-pushed the openssh_add_fido2_support branch 2 times, most recently from 54aa819 to 8314eb3 Compare January 6, 2021 22:12
@linosgian
libcbor: Add new package
42e2f42 
Libcbor is a C library for parsing and generating CBOR[0],
the general-purpose schema-less binary data format.

[0]: https://tools.ietf.org/html/rfc7049

Signed-off-by: Linos Giannopoulos <linosgian00@gmail.com>
@linosgian
libfido2: Add new package
1ce5b10 
libfido2 provides library functionality and command-line
tools to communicate with a FIDO device over USB, and to
verify attestation and assertion signatures.

libfido2 supports the FIDO U2F (CTAP 1) and FIDO 2.0 (CTAP 2) protocols.

Signed-off-by: Linos Giannopoulos <linosgian00@gmail.com>
@linosgian
openssh: Add FIDO2 hardware token support
855db86 
Version 8.2[0] added support for two new key types: "ecdsa-sk" and
"ed25519-sk". These two type enable the usage of hardware tokens that
implement the FIDO (or FIDO2) standard, as an authentication method for
SSH.

Since we're already on version 8.4 all we need to do is to explicitly enable
the support for hardware keys when compiling OpenSSH and add all the
missing dependencies OpenSSH requires.

OpenSSH depends on libfido2[1], to communicate with the FIDO devices
over USB. In turn, libfido2 depends on libcbor, a C implementation of
the CBOR protocol[2] and OpenSSL.

[0]: https://lwn.net/Articles/812537/
[1]: https://github.com/Yubico/libfido2
[2]: tools.ietf.org/html/rfc7049

Signed-off-by: Linos Giannopoulos <linosgian00@gmail.com>
@linosgian linosgian force-pushed the openssh_add_fido2_support branch from 8314eb3 to 855db86 Compare January 6, 2021 22:53
neheb
neheb reviewed Jan 7, 2021
View reviewed changes
net/openssh/Makefile
--with-stackprotect \
--with$(if $(CONFIG_OPENSSL_ENGINE),,out)-ssl-engine

--with$(if $(CONFIG_OPENSSL_ENGINE),,out)-ssl-engine \
Copy link
Contributor

@neheb neheb Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it interesting that CONFIG_OPENSSL_ENGINE is not in PKG_CONFIG_DEPENDS. But that's beside the point.

Copy link
Contributor Author

@linosgian linosgian Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like an omission indeed, but I think it escapes the scope of this PR

@linosgian
Copy link
Contributor Author

linosgian commented Jan 7, 2021

@neheb This should be okay. Anything else that needs to be addressed before merging?

@neheb neheb merged commit 8ede716 into openwrt:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@neheb neheb neheb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@linosgian @neheb