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

FIDO support for openssh #34

Merged
merged 3 commits into from
Mar 2, 2020
Merged

FIDO support for openssh #34

merged 3 commits into from
Mar 2, 2020

Conversation

tavrez
Copy link

@tavrez tavrez commented Feb 27, 2020

if you are on Windows 10 version 1903 or higher you need to run git-bash as administrator

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

This is an incredible achievement, in such a short amount of time.

It inspired me to try to assist more thoroughly than I normally would, e.g. by figuring out why the Release mode would not build, or why C99 was a problem.

I pointed out the fixes I made locally, and suggested a couple more changes.

Additionally, I would think that this commit would like to be split into three steps: adding libcbor and libfido2 separately, then using the latter in OpenSSH.

@tavrez could you fetch the openssh-with-security-tokens branch from https://github.com/dscho/MSYS2-packages and use git log and git diff to compare my suggested edits with your patch, and if you like all of that, force-push and we're good to merge?

libfido2/PKGBUILD Outdated Show resolved Hide resolved
libcbor/PKGBUILD Outdated Show resolved Hide resolved
libcbor/PKGBUILD Outdated Show resolved Hide resolved
pkgname=("${pkgbase}" "${pkgbase}-devel")
pkgver=1.3.1
pkgrel=1
pkgdesc="libfido2 provides library functionality and command-line tools to communicate with a FIDO device over USB, and to verify attestation and assertion signatures."
Copy link
Member

Choose a reason for hiding this comment

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

To anyone unfamiliar with FIDO or U2F, this description leaves things a bit unclear. I know it was lifted from the README of https://github.com/Yubico/libfido2, and they're probably too deep down in the rabbit hole to come up with a better description.

I think something like this might be better:

A library to support easy, strong authentication (e.g. via USB security tokens or smartcards)

Or something like that.

libfido2/0001-cmake-msys-fixes.patch Outdated Show resolved Hide resolved
@rimrul
Copy link
Member

rimrul commented Feb 28, 2020

openssh/PKGBUILD probably also needs it's pkgrel incremented.

@cw789
Copy link

cw789 commented Feb 28, 2020

Wow! Thanks a lot Reza.

@dscho
Copy link
Member

dscho commented Feb 28, 2020

openssh/PKGBUILD probably also needs it's pkgrel incremented.

@rimrul that is done automatically in the release management: https://github.com/git-for-windows/build-extra/blob/48bcaa455490c664fd9e8c003fdd02134a430869/please.sh#L2749-L2755

I think I forgot to update the checksums, though... My bad.

@dscho
Copy link
Member

dscho commented Feb 28, 2020

I think I forgot to update the checksums, though... My bad.

Yep. Fixed. (Force-pushed openssh-with-security-tokens to https://github.com/dscho/MSYS2-packages.)

@rimrul
Copy link
Member

rimrul commented Feb 28, 2020

that is done automatically in the release management

Right. I tend to forgett about that part of please.sh

@dscho
Copy link
Member

dscho commented Feb 28, 2020

Oh, that part is new... I worked pretty hard on parallelizing the Azure Pipeline I use to build and publish Pacman packages. I also moved it from a private org with a private agent pool to the public org over at https://dev.azure.com/git-for-windows/git/_build

In the process, I had to reimplement small parts of the upgrade subcommand in please.sh, to allow for finer-grained steps in the Pipeline. I then introduced the --no-build (or was it --skip-build?) option to that subcommand so that I could run it in the first job (whose only purpose is now to figure out whether a new source version is available for the given component, and create the necessary change for the corresponding PKGBUILD as well as for the ReleaseNotes.md).

So yeah, lots has changed, lots of code should probably be removed, too.

@tavrez
Copy link
Author

tavrez commented Feb 28, 2020

Sorry I'm little busy these days and can only work at nights. I liked your suggestions and will add some question and notes to them right now.

We are about to build libfido2 so that we can rebuild OpenSSH with
support for USB security tokens, smartcards, etc.

To that end, we add libcbor (a library to read/write CBOR, a
general-purpose schema-less binary data format) as it is a dependency of
libfido2.

Signed-off-by: Reza Tavakoli <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
This library will be useful in the next step when we rebuild OpenSSH to
allow using security tokens.

Signed-off-by: Reza Tavakoli <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
With this patch, our copy of OpenSSH can make use of the FIDO support
library to access USB security tokens, smartcards, etc for easy and
strong authentication.

Signed-off-by: Reza Tavakoli <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho merged commit 09dc470 into git-for-windows:master Mar 2, 2020
@dscho
Copy link
Member

dscho commented Mar 3, 2020

For lurkers who wonder why this is not deployed to Git for Windows' Pacman repositories yet: I have a reliable hang in the CMake process in 32-bit, just after detecting the C++ compiler: https://dev.azure.com/git-for-windows/git/_build/results?buildId=51479&view=logs&j=cfa20e98-6997-523c-4233-f0a7302c929f&t=4fefe862-c3fb-598f-9db1-d9857aa98bc4&l=377

@dscho
Copy link
Member

dscho commented Mar 4, 2020

Okay, I finally got it all built: https://dev.azure.com/git-for-windows/git/_build/results?buildId=51609&view=logs&j=0b109914-fd3b-548c-e58c-f9d47cb04f5f&t=0b109914-fd3b-548c-e58c-f9d47cb04f5f.

@tavrez
Copy link
Author

tavrez commented Mar 4, 2020

Thanks for all the efforts, I've tracked everything you did through pipelines and pushes on git repo, I had a mistake on not adding libcbor-devel as dependency package on OpenSSH, hope I do things better in my next contributions :)

@dscho
Copy link
Member

dscho commented Mar 4, 2020

@tavrez oh, don't worry. The biggest issue here was not caused by you, it was caused by me not realizing that librhash insists on having OpenSSL v1.0.x as a runtime dependency, and not releasing it properly. Git for Windows moved on to OpenSSL v1.1.x and I only kept the earlier DLLs around for the transition period, failing to notice that librhash does not really need it (and in fact, the 32-bit version hangs nowadays if you provide it with the OpenSSL v1.0.x library). It took me long enough to diagnose that.

Your contribution has been my highlight of the past few weeks. So please do not be sorry at all.

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.

4 participants