Skip to content

openssh 8.2p1 (with FIDO support) #50311

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

Closed
wants to merge 1 commit into from

Conversation

xanderlent
Copy link
Contributor

@xanderlent xanderlent commented Feb 16, 2020

This uses libfido2 to add FIDO support to OpenSSH.
This is arguably the most prominent feature in the 8.2 release.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This is a draft because I'm still working on the formulae for the dependencies, which are not yet upstream. See #50305 for libcbor and #50326 for libfido2. Deps are upstream. 🎉

Would the maintainers also prefer if we devised some sort of test for the FIDO/U2F functionality? It's difficult to test w/o a physical security key.

@xanderlent xanderlent mentioned this pull request Feb 17, 2020
@gzm55
Copy link

gzm55 commented Feb 20, 2020

can we build libcbor and libfido2 on the fly while building the openssh?

@SMillerDev
Copy link
Member

You could but that doesn't have much benefit afaik

@gzm55
Copy link

gzm55 commented Feb 20, 2020

You could but that doesn't have much benefit afaik

that can avoid depending on libfido2 and libcbor formula before they are accepted.

@SMillerDev
Copy link
Member

If they can be separate formula, building inline isn't going to be accepted either.

This uses libfido2 to add FIDO support to OpenSSH.
This is arguably the most prominent feature in the 8.2 release.
@xanderlent xanderlent marked this pull request as ready for review February 22, 2020 02:21
@xanderlent
Copy link
Contributor Author

Maintainers: Landing this patch before libfido2 is patched on Linux will also break this formula on Linux. I'll patch libfido2 once it lands in linuxbrew-core regardless. If you decide to wait, I'll also report back here when patching is done.

@zbeekman
Copy link
Contributor

@xanderlent This looks ready to ship now that libfido2 was merged. Can we 🚢 this?

Also, are there instructions anywhere for using a FIDO key with openssh? I guess, presumably the sshd server is setup to support some sort of PAM module or challenge response?

@xanderlent
Copy link
Contributor Author

xanderlent commented Feb 22, 2020

To 🚢 or not to 🚢, that is the question:

@zbeekman On macOS, it's 100% ready to ship!

On Linux, merging this will break OpenSSH until Homebrew/linuxbrew-core#19684 is resolved, because libfido2 uses libudev on Linux (instead of IOKit on macOS) which is not something expressed here in upstream.

I can't say if "breaks Linux" is a blocker for homebrew-core, since that's an executive decision.

Using SSH with FIDO tokens

The short answer is that you'll need to make & distribute new security-key-backed SSH keys. I think you may also need an SSH server with support for these.

For full instructions, see the OpenSSH Release Notes for 8.2/8.2p1:
https://www.openssh.com/txt/release-8.2

RSA key users should make sure that they're using SHA-2 signatures. (My previous statement here was incorrect.)

@zbeekman zbeekman added the ready to merge PR can be merged once CI is green label Feb 22, 2020
@zbeekman
Copy link
Contributor

@xanderlent thanks for the info!

Linuxbrew tests and merges on their own. So long as the test catches the problem there won’t be an issue.

As far as RSA goes I use ed25519 keys whenever possible.

Copy link
Contributor

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

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

LGTM

I think we can 🚢 this unless the test won’t catch the problems on Linux.

@dawidd6 dawidd6 closed this in e19d50d Feb 22, 2020
@lou-k
Copy link

lou-k commented Feb 24, 2020

Thanks for this, @xanderlent ! Very much appreciated.

FWIW, it looks like only ecdsa-sk are supported with the default build, ed25519-sk yields a Key enrollment failed: requested feature not supported error.

@gzm55
Copy link

gzm55 commented Feb 24, 2020

if using yubikey, ed25519-sk requires new firmware 5.2.3

@xanderlent
Copy link
Contributor Author

@lou-k Glad I could help. As mentioned in the upstream release notes, ecdsa is the only mandatory algorithm in the FIDO standards. If you want support for ed25519, you'll need to get a token that supports it.

@lock lock bot added the outdated PR was locked due to age label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants