Skip to content

Support U2F/WebAuthn keys USB detection in Linux#592

Closed
ocraviotto wants to merge 3 commits intoVersent:masterfrom
ocraviotto:fido-linux-support
Closed

Support U2F/WebAuthn keys USB detection in Linux#592
ocraviotto wants to merge 3 commits intoVersent:masterfrom
ocraviotto:fido-linux-support

Conversation

@ocraviotto
Copy link
Copy Markdown

This PR updates marshallbrekka/go-u2fhost, imported by the Okta and Google providers to support U2F/WebAuthn USB keys as 2FA devices.

The update should help fix the problems reported in #419 as they had their origin in marshallbrekka/go-u2fhost not able to properly detect these devices in Linux. That was fixed upstream with marshallbrekka/go-u2fhost#10 which also explains what other changes were done in the hid libraries used by go-u2fhost to make the detection work in Linux too.

Along the library change and other mod updates, this commit includes a change to the makefile to support the required tag to use
hidraw instead of libusb when compiling linux (and so really apply the fix when compiling) as well as related changes to make it easier to build/install saml2aws in different platforms, including OS detection to prevent macOS builds from building in non-Darwin environments, which should tackle #56.

Finally, I've also included some updates in the README to clarify the changes in the build process and the additional per OS targets.

@wolfeidau
Copy link
Copy Markdown
Contributor

This is a fantastic idea but i am really not keen on adding any CGO dependent stuff as it is already killing me.

I would love to figure out a way to put this in a plugin or hook which people can install separately.

@fieldju
Copy link
Copy Markdown

fieldju commented Mar 16, 2021

Would love to get this merged, had to fork the fork, clone, and compile it myself 🤷‍♂️.

@ocraviotto
Copy link
Copy Markdown
Author

ocraviotto commented Mar 19, 2021

This is a fantastic idea but i am really not keen on adding any CGO dependent stuff as it is already killing me.

@wolfeidau Your project is already using it. I just updated the go-u2fhost ref so as to pick up the change in the underlying library used by it, so a dependency of your dependency.

Under the hood, go-u2fhost was using github.com/karalabe/hid, I just made a contribution (referenced above) to change that to github.com/bearsh/hid, which is a fork that updated karalabe's libusb and hidapi code base with upstream libusb/libusb and
libusb/hidapi, which among other things incorporates the changes in libusb/hidapi to support Usage Page and Usage on Linux with hidraw (see https://github.com/libusb/hidapi/pull/139/files) when packages are built with "-tags=hidraw", while maintaining the old behaviour when built without it. I also contributed with a fix to the fork.

In brief, this PR only adds the "-tags=hidraw" tag on Linux builds to support Usage Page and Usage on Linux with hidraw.
The rest of the changes should address other building issues but do not include additional CGO dependencies that were not already there in previous versions.

If you want me to update my branch with master to reconsider, I'll be happy to. Let me know.

@smlx
Copy link
Copy Markdown
Contributor

smlx commented Mar 24, 2021

Hi, thanks for working on this! I found this PR and rebased it onto master to get multiple yubikey support in Okta. The nice thing is that master now has the updated go-u2fhost module version so this PR can be trimmed down to only README and Makefile changes 🎉

In any case, I pushed a branch with the rebase over here: https://github.com/smlx/saml2aws/tree/fido-linux-support

cc @ocraviotto and @wolfeidau

edit: forgot to mention that this PR works beautifully 👍

@ocraviotto ocraviotto force-pushed the fido-linux-support branch 4 times, most recently from d63a624 to 718009d Compare April 8, 2021 10:28
@ocraviotto
Copy link
Copy Markdown
Author

ocraviotto commented Apr 8, 2021

@wolfeidau I've gone ahead and refactored my initial PR to reflect all of your changes to the release flow with goreleaser. Had a couple of initial failures but finally managed to make it work. I've basically split the build/release process between linux and non-linux in Ubuntu and MacOs contexts respectively. This should maintain the original context for non-linux and build and so support u2f keys in linux by building on Ubuntu. I've also changed the Makefile to allow building with goreleaser locally, except that it won't build linux if in non-linux.

I'm not sure whether MacOs binaries would work for all cases if built on Ubuntu (asked a colleague to test and the limited cases he tried did work, but can't be sure without further testing), in which case it would be possible to move the build/release fully to ubuntu-latest (not sure why you had builds on ubuntu and Releases done with macos?).

Finally, I've changed the Build step from macOS-latest to macos-latest (aligned case to the docs specs for OS types supported and their YAML label), so I suspect that's why the PR check is missing: https://github.com/Versent/saml2aws/pull/592/checks?check_run_id=2295884756.

Do let me know if this now looks more in line with what you wanted and of course I'd be happy to read your feedback.

PS: I've also fixed a bug I found with the latest changes to the okta provider: See my commit message and the line changed for more details.

@ocraviotto
Copy link
Copy Markdown
Author

@smlx Tanks for trying it out. Please note I've rebased and updated with changes that should allow you to build with U2F keys support from linux. I've created a test release in my fork with almost the same code base I used to update this PR. Release builds here: https://github.com/ocraviotto/saml2aws/releases/tag/v2.28.5-testrel1

@wolfeidau
Copy link
Copy Markdown
Contributor

@ocraviotto yeah I have dug into this and agree it is 100% already there and actively used.

I need to pull apart your PR a bit as a lot of things are going in there.

ocraviotto added 2 commits May 4, 2021 19:21
This commits updates marshallbrekka/go-u2fhost, imported by
the Okta and Google providers to support U2F/WebAuthn USB keys
as 2FA devices.

The update should help fix the problems reported in
Versent#419 as they had their
origin in marshallbrekka/go-u2fhost not able to properly detect
these devices in Linux. That was fixed upstream with
marshallbrekka/go-u2fhost#10 which also
explains what other changes were done in the hid libraries
used by go-u2fhost to make the detection work in Linux too.

Along the library change and other mod updates, this commit includes
a change to the makefile to support the required tag to use
hidraw instead of libusb when compiling linux (and so really apply
the fix when compiling) as well as related changes to make it easier
to build/install saml2aws in different platforms, including OS
detection to prevent macOS builds from building in non-Darwin
environments, which should tackle
Versent#56.

Finally, I've also included some updates in the README
to clarify the changes in the build process and the additional
per OS targets.
The build change allows linux builds to support u2f keys
by splitting release builds between Ubuntu and MacOs types
so maintaining the current behavior for non-linux builds.

This change has been extended to the make file, that will build all
targets if in Linux else all minus linux. Linux installs also support
u2f keys detection.
@ocraviotto ocraviotto force-pushed the fido-linux-support branch from 718009d to 55637a0 Compare May 4, 2021 16:52
@ocraviotto
Copy link
Copy Markdown
Author

@wolfeidau Rebased once more. I see actions need a maintainer's approval to run, might be a good additional check. I have fixed a minor conflict but have not noticed anything else worth checking in more detail, though did run tests and builds locally.
As earlier, if there is anything I can do to help, happy to.

runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would it work to run only on ubuntu-latest and have a single .goreleaser.yml that has 2 builds, something like:

builds:
- id: saml2aws-linux
  main: ./cmd/saml2aws/main.go
  binary: saml2aws
  flags:
    - -tags=hidraw
    - -trimpath
    - -v
  ldflags:
    - -s -w -X main.Version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.Date}}
  goos:
    - linux
  goarch:
    - amd64
    - arm64
    - arm
- id: saml2aws-non-linux
  main: ./cmd/saml2aws/main.go
  binary: saml2aws
  flags:
    - -trimpath
    - -v
  ldflags:
    - -s -w -X main.Version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.Date}}
  goos:
    - windows
    - darwin
  goarch:
    - amd64
    - arm64
    - arm

You would also avoid having to conditionally install libudev-dev and also avoid having to use a matrix.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

hmmmm scratch that, you can't easily cross-compile with CGO enabled 😞

Comment thread .github/workflows/go.yml
Comment on lines +63 to +65
strategy:
matrix:
os: [ubuntu-latest, macos-latest]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you could do something like

strategy:
  matrix:
    include:
    - os: macos-latest
      build-type: non-linux
    - os: ubuntu-latest
      build-type: linux

then you can use it like:

    - name: GoReleaser
      uses: goreleaser/goreleaser-action@v2
      with:
        version: latest
        args: build --snapshot --rm-dist --config .goreleaser-${{ matrix.build-type }}.yml

Comment thread .github/workflows/go.yml
Comment on lines +76 to +78
- name: Install libudev if in Linux
run: sudo apt-get install libudev-dev
if: ${{ startsWith(matrix.os, 'ubuntu') }}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can move this into .goreleaser-linux.yml:

before:
  hooks:
    - sudo apt-get install libudev-dev

@jplanckeel
Copy link
Copy Markdown

Hello, we have test fork release for FIDO U2F on linux Fedora, it works perfectly.

When you can integrated this in release ?

@abualy
Copy link
Copy Markdown

abualy commented Nov 22, 2021

worked perfectly for fedora :D
libudev-dev quivalent in fedora is ... systemd-devel

dnf install systemd-devel

@abualy
Copy link
Copy Markdown

abualy commented Feb 10, 2022

any news on this one ?

@rarruda
Copy link
Copy Markdown

rarruda commented Jun 13, 2022

@duboisf : Anything I can do to help to get this merged?

I would love to be able to use it in linux (and i could use a custom build myself, as hack), but I don't want to recommend it to others in my team unless it merged upstream.

@vascorsd
Copy link
Copy Markdown

Hello, as an interested party, any reason why this is still pending and support for webauthn not merged yet? From the comments seems the PR works. What are the pending concerns?

@gregflynn
Copy link
Copy Markdown

@wolfeidau Any recent insights on this? I'm similarly facing the merge-or-fork dilemma others had faced in this thread prior as my organization works to limit second factors to more secure forms. If there are outstanding items you feel should be addressed then there can be resources attributed to them, but currently this pull request lacks feedback to @ocraviotto or anyone else on how to proceed.

@smlx smlx mentioned this pull request Feb 9, 2023
@mapkon mapkon closed this in #951 Feb 28, 2023
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.

10 participants