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.

Sign up for GitHub

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

Jump to bottom

Release version 2.0.0 #175

Merged
merged 6 commits into from
Mar 18, 2021
Merged

Release version 2.0.0 #175

merged 6 commits into from
Mar 18, 2021

Conversation

abergs
Copy link
Collaborator

@abergs abergs commented May 28, 2020 •
edited
Loading

This PR bumps the version to 2.0.0.

The reason for the major version is because the large number of changes, some that in theory could be breaking.

Preview releases are up on NuGet: https://www.nuget.org/packages/Fido2/2.0.0-preview2

Release notes:

Changes

💥 Breaking change

🚀 Features and enhancements

🐛 Bug Fixes

🧰 Maintenance & documentation

@abergs abergs added the chore Maintenance or stuff that needed to be done label May 28, 2020
@abergs abergs requested a review from aseigler May 28, 2020 14:04
github-actions[bot]
github-actions bot approved these changes May 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

github-actions[bot]
github-actions bot approved these changes May 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@abergs abergs mentioned this pull request May 28, 2020
Closed
@codecov
Copy link

codecov bot commented May 28, 2020 •
edited
Loading

Codecov Report

Merging #175 (3e4ded5) into master (acda23d) will increase coverage by 0.43%.
The diff coverage is n/a.

❗ Current head 3e4ded5 differs from pull request most recent head 9e1530a. Consider uploading reports for the commit 9e1530a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   71.30%   71.73%   +0.43%     
==========================================
  Files          67       68       +1     
  Lines        3042     3361     +319     
==========================================
+ Hits         2169     2411     +242     
- Misses        873      950      +77
Impacted Files Coverage Δ
...s/Objects/AuthenticationExtensionsClientOutputs.cs 10.00% <0.00%> (-90.00%) ⬇️
Src/Fido2.Models/Objects/GeoCoordinate.cs 0.00% <0.00%> (-40.57%) ⬇️
Src/Fido2/AuthenticatorAttestationResponse.cs 82.02% <0.00%> (-17.98%) ⬇️
...ido2.Models/AuthenticatorAttestationRawResponse.cs 85.71% <0.00%> (-14.29%) ⬇️
Src/Fido2/AttestationFormat/Packed.cs 66.66% <0.00%> (-9.74%) ⬇️
Src/Fido2/AuthenticatorAssertionResponse.cs 94.73% <0.00%> (-5.27%) ⬇️
Src/Fido2/AttestationFormat/FidoU2f.cs 86.41% <0.00%> (-4.63%) ⬇️
Src/Fido2.Models/Fido2Configuration.cs 38.46% <0.00%> (-3.21%) ⬇️
Src/Fido2/Objects/CredentialPublicKey.cs 83.89% <0.00%> (-0.68%) ⬇️
Src/Fido2/Fido2NetLib.cs 79.54% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d823eb...9e1530a. Read the comment docs.

@abergs abergs mentioned this pull request May 28, 2020
Closed
@abergs
Copy link
Collaborator Author

abergs commented Jun 10, 2020

@aseigler We are waiting for #166, right?

@aseigler
Copy link
Collaborator

@aseigler We are waiting for #166, right?

Yes please, hoping to have it resolved in the next couple of days.

@abergs
Copy link
Collaborator Author

abergs commented Jun 11, 2020

@aseigler No stress, preview package is out for consumers who need it.

@mackie1001
Copy link
Contributor

Thanks guys :)

@aseigler
Copy link
Collaborator

image

github-actions[bot]
github-actions bot approved these changes Jun 28, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@abergs
Copy link
Collaborator Author

abergs commented Jun 28, 2020

Published preview2 package: https://www.nuget.org/packages/Fido2/2.0.0-preview2

@mackie1001 Could you try and verify that preview works for you?

@daisukenishino2 daisukenishino2 mentioned this pull request Jun 29, 2020
Open
@aseigler
Copy link
Collaborator

aseigler commented Jul 8, 2020

Published preview2 package: https://www.nuget.org/packages/Fido2/2.0.0-preview2

@mackie1001 Could you try and verify that preview works for you?

Bump.

@abergs
Copy link
Collaborator Author

abergs commented Aug 27, 2020

@mackie1001 How has your experience been with the 2.0 preview?

@mackie1001
Copy link
Contributor

@abergs Apologies for the lack of response. I've not had the chance to pick it up as I've been focusing on another project at work. I will try and give it a go over the weekend.

@mackie1001
Copy link
Contributor

@abergs Overall it seems fine, however I have encountered an issue when trying to register my Windows Hello enabled laptop.

The exception when calling MakeNewCredentialAsync() is:

Asn1.AsnException: wrong number of sub-elements: 3 (expected: 1)
   at Asn1.AsnElt.CheckNumSub(Int32 n)
   at Fido2NetLib.AttestationFormat.Tpm.SANFromAttnCertExts(X509ExtensionCollection exts)
   at Fido2NetLib.AttestationFormat.Tpm.Verify()
   at Fido2NetLib.AuthenticatorAttestationResponse.<VerifyAsync>d__10.MoveNext()
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Fido2NetLib.Fido2.<MakeNewCredentialAsync>d__6.MoveNext()

It looks like the TPM logic has been beefed up considerably since 1.1.

The error occurs on line 279 of Tpm.cs:

exp.CheckNumSub(1);

I've reproduced it with the following settings in the custom registration in the demo app:

Attestation type: Direct
Autheticator: Platform (TPM)
User verification: discouraged

The authenticator is "Windows Hello Hardware Authenticator", AAGUID: 08987058-cadc-4b81-b6e1-30de50dcbe96

Playing around a bit I managed to tweak the code in line with the sample on page 35 of this doc: https://trustedcomputinggroup.org/wp-content/uploads/Credential_Profile_EK_V2.0_R14_published.pdf

// SEQUENCE
30 49
    // SET
    31 16
        // SEQUENCE
        30 14
            // OBJECT IDENTIFER tcg-at-tpmManufacturer (2.23.133.2.1)
            06 05 67 81 05 02 01
            // UTF8 STRING id:54434700 (TCG)
            0C 0B 69 64 3A 35 34 34 33 34 37 30 30
    // SET
    31 17
        // SEQUENCE
        30 15
            // OBJECT IDENTIFER tcg-at-tpmModel (2.23.133.2.2)
            06 05 67 81 05 02 02
            // UTF8 STRING ABCDEF123456
            0C 0C 41 42 43 44 45 46 31 32 33 34 35 36
    // SET
    31 16
        // SEQUENCE
        30 14
            // OBJECT IDENTIFER tcg-at-tpmVersion (2.23.133.2.3)
            06 05 67 81 05 02 03
            // UTF8 STRING id:00010023
            0C 0B 69 64 3A 30 30 30 31 30 30 32 33

var exp = generalName.GetSub(0); actually ends up being the top level // SEQUENCE from that sample and not the level above like the current code assumes.

I think the mistake was thinking the GeneralName to directoryName relationship was "has a" rather than "is a".

I'll raise a PR ASAP but I'd appreciate some help verifying the fix using another real world TPM implenentation - e.g. OSX maybe?

@aseigler
Copy link
Collaborator

aseigler commented Aug 29, 2020 •
edited
Loading

Switching the ASN.1 decoder/encoder was definitely a big piece of this update. Definitely will try to reproduce the scenario you hit as well.

There is no other real world TPM implementation right now, only Windows 10. Apple doesn't TPM, the only real hope for TPM would be Linux someday if someone wants it bad enough.

@mackie1001 mackie1001 mentioned this pull request Aug 29, 2020
Merged
@mackie1001
Copy link
Contributor

@aseigler I assumed all TPM-based implementations used tpm but it looks like I was wrong. Windows Hello Software Authenticator uses packed too.

@aseigler
Copy link
Collaborator

@aseigler I assumed all TPM-based implementations used tpm but it looks like I was wrong. Windows Hello Software Authenticator uses packed too.

Yes, a platform authenticator attestation request on Windows 10 can result in packed or TPM depending on several factors.

@mackie1001
Copy link
Contributor

FYI I've now tested this on Windows 10 1909 and 2004 and get the same result.

mackie1001 and others added 3 commits October 13, 2020 08:51
@mackie1001 @aseigler
Fixed SANFromAttnCertExts to correctly extract the TPM properties fro…
ae07c31 
…m the Subject Alternative Name certificate extension (#187)

* Fixed SANFromAttnCertExts to correctly extract the TPM properties from the Subject Alternative Name certificate extension

* Variable name typo

* Add check and fix for non-conformant SAN attribute in AIK cert, write test for same.

Co-authored-by: Alex Seigler <[email protected]>
@mackie1001
MDS error handling and cache-friendlieness refactoring (#188)
37e5d3c 
* Improved error handling and logging for MDS errors along with a refactoring of how the TOC JWT alg is passed around to better serve the cached use-case

* Updated test
@aseigler
Merge branch 'master' into release-2.0.0
dc04119
github-actions[bot]
github-actions bot approved these changes Mar 18, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

github-actions[bot]
github-actions bot approved these changes Mar 18, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

github-actions[bot]
github-actions bot approved these changes Mar 18, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@aseigler aseigler merged commit f894070 into master Mar 18, 2021
@aseigler aseigler deleted the release-2.0.0 branch March 18, 2021 01:05
@yackermann
Copy link
Contributor

Congrats guys!

@aseigler aseigler mentioned this pull request May 21, 2021
Closed
@redhook62 redhook62 mentioned this pull request Jul 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aseigler aseigler aseigler approved these changes

@github-actions github-actions[bot] github-actions[bot] approved these changes

Assignees
No one assigned
Labels
chore Maintenance or stuff that needed to be done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abergs @aseigler @mackie1001 @yackermann