search

Versent / saml2aws

CLI tool which enables you to login and retrieve AWS temporary credentials using a SAML IDP
https://github.com/Versent/saml2aws
MIT License
2.09k stars 564 forks source link

feat: Add support for Windows WebAuthn 2FA #1221

Closed seppestas closed 9 months ago

seppestas commented 9 months ago

See #912. This uses a new library I created to use system level WebAuthn client libraries. Currently this only supports Windows Hello, I hope to one day also support MacOS Touch ID.

Note: I made some last minute changes before creating this PR that I was not able to fully verify due to me not being able to remove sessions / force 2FA.

codecov-commenter commented 9 months ago

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 41.79%. Comparing base (9a4c5b6) to head (c1df651). Report is 16 commits behind head on master.

Files Patch % Lines
pkg/provider/okta/okta_webauthn.go 0.00% 11 Missing :warning:
pkg/provider/okta/okta.go 0.00% 3 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1221 +/- ## ========================================== + Coverage 40.44% 41.79% +1.34% ========================================== Files 54 54 Lines 8276 6372 -1904 ========================================== - Hits 3347 2663 -684 + Misses 4491 3271 -1220 Partials 438 438 ``` | [Flag](https://app.codecov.io/gh/Versent/saml2aws/pull/1221/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/Versent/saml2aws/pull/1221/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent) | `41.79% <0.00%> (+1.34%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Versent#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

seppestas commented 9 months ago

@mapkon I had a look, but I think this might just be a regression in the test itself? Or is it just failing to build for ARM windows?

The error is "failed to build for windows_arm_6: exit status 1: go: downloading github.com/sirupsen/logrus v1.9.3".

This sounds to me like it might be failing to download a different package (logrus), or does goreleaser just print the last thing it did for some reason?

If there is a problem, I would expect it to be with building for ARM Windows, which is something I did not test TBH. Can I somehow get the logs of the go build?

seppestas commented 9 months ago

Turns out the problem is that CGO is not supported on Windows ARM, and somehow this just causes the whole webauthn_windows.go to be ignored iof. showing a useful error message. Even worse is that I can compile the package just fine, only when compiling the test / saml2aws it fails, because there is no explicit API :/.

Go-u2fhost, i.e bearsh/hid solves this by adding a !cgo build constraint, I'll do the same for now. This means using Windows Hello for 2FA will not be supported (nor is it when using raw USB authenticators).

In theory I can remove the dependency on cgo, I only use it as an easy way to create the structs I need from the C header files. In the long run, I would like to use something like winrt, but I am not sure if this is possible.

seppestas commented 9 months ago

@mapkon it should work with the latest changes.

mapkon commented 9 months ago

@mapkon it should work with the latest changes.

I will take a look

mapkon commented 9 months ago

@seppestas This looks fine. Any reason why you omitted tests?

seppestas commented 9 months ago

Eh, there's not all that much to test. I suppose error handling could be tested?

mapkon commented 9 months ago

Eh, there's not all that much to test. I suppose error handling could be tested?

Yeah - the codecov report above could help. Its not really mandatory, but I like to cover these bases since it makes future changes easier to merge, and regressions easier to spot.

seppestas commented 9 months ago

I had a look at adding tests. The most useful test would be to test func fidoWebAuthn, i.e. verify that if no NewFidoClient is found (or any other error occurs), it will try to use the system Webauthn library instead.

Please also help in thinking critically about this. This is a scenario that works for me, but it might not work for others. E.g people with a Yubikey, that want to use their Yubikey through Windows Hello, might not be happy with this solution. See my comment in #912. I think the "fall back to system Webauthn if no U2F USB device found" approach is good enough for most cases.

Sadly, no tests exists currently for the fidoWebAuthn function. The only tests that are there are for ChallengeU2F, and it only really tests the successful condition, arguably also quite useless, while there is plenty more interesting stuff like timeouts and error conditions.

I'm happy to write tests for the goal of improving quality, but not just to get a coverage number go up. To make the coverage number go up (or at least, to cover most of the lines I added), I could write a (arguably useless) test for ChallengeSystemWebAuthn that would do little more than verifying the contents of the SignedAssertion and checks if the error is propagated. This test would also be non-trivial, because I do not use any interfaces / objects, I just use a single function.

mapkon commented 9 months ago

I had a look at adding tests. The most useful test would be to test func fidoWebAuthn, i.e. verify that if no NewFidoClient is found (or any other error occurs), it will try to use the system Webauthn library instead.

Please also help in thinking critically about this. This is a scenario that works for me, but it might not work for others. E.g people with a Yubikey, that want to use their Yubikey through Windows Hello, might not be happy with this solution. See my comment in #912. I think the "fall back to system Webauthn if no U2F USB device found" approach is good enough for most cases.

Sadly, no tests exists currently for the fidoWebAuthn function. The only tests that are there are for ChallengeU2F, and it only really tests the successful condition, arguably also quite useless, while there is plenty more interesting stuff like timeouts and error conditions.

I'm happy to write tests for the goal of improving quality, but not just to get a coverage number go up. To make the coverage number go up (or at least, to cover most of the lines I added), I could write a (arguably useless) test for ChallengeSystemWebAuthn that would do little more than verifying the contents of the SignedAssertion and checks if the error is propagated. This test would also be non-trivial, because I do not use any interfaces / objects, I just use a single function.

Makes sense! You make a sound point so I am happy to merge either way. Do you want to write the test or should I just go ahead and merge?

seppestas commented 9 months ago

@mapkon thanks for merging this. If I find some spare time I might add some tests. I am hoping to add support for Linux and MacOS sometime in the future.

There's something to be said in at least making the Authn library more test friendly. I'll probably change the API in something that provides a list of authenticators, one of which could be e.g. Windows Hello. This is probably also a bit more elegant than returning an error on unsupported platforms, and it would allow platforms like Linux to have multiple options.

Any idea about how many users would use Windows on ARM? Would this be significant enough to try and get rid of CGO?

Either way, I have been using this feature daily since January, I hope other people find it useful as well.

mapkon commented 8 months ago

That is an excellent question (re: windows user base). However, I have no idea how many users consume the app on windows ARM. I know for sure that. @briantist is a windows advocate around these circles.

Re: CGO

From I think @wolfeidau has always wanted to get rid of CGO, but we never got around to it. There is a case to be made for simplifying the project by removing the CGO dependencies.

CC: @gliptak

seppestas commented 8 months ago

Re: CGO

I'll have a look. I did not really consider CGO would be this much of a pain. The worst part is that I started out creating my library without CGO, but only later added it to easily re-use the C structures in the official Windows WebAuthn API headers.

In an even more perfect world, I would be able to get it working Windows COM / WinRT, making it properly async. I looked into this, but I don't think it is supported, though in theory possible using lower level or maybe the Smartcard WebRT libraries.

mapkon commented 8 months ago

Most definitely (and thanks for looking into this). @wolfeidau do you have any input/preference on this?