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

Implement assertion #14

Merged
merged 23 commits into from
Jun 15, 2020
Merged

Implement assertion #14

merged 23 commits into from
Jun 15, 2020

Conversation

duijf
Copy link
Collaborator

@duijf duijf commented Jun 14, 2020

WIP PR to implement assertion.

Updated Ruud's test fixtures. The tests still fail on actually verifying the signature.

@arianvp
Copy link
Owner

arianvp commented Jun 14, 2020

Maybe we should look at how https://github.com/duo-labs/webauthn/blob/master/protocol/webauthncose/webauthncose.go implements it

@arianvp
Copy link
Owner

arianvp commented Jun 14, 2020

This fucking standard is beyond parody!

The Signature in the cbor value is ASN.1 encoded

signature.

Using the function defined in [RFC8017], the signature is:
Signature = I2OSP(R, n) | I2OSP(S, n)
where n = ceiling(key_length / 8)

https://tools.ietf.org/html/rfc8152#section-8.1

@arianvp
Copy link
Owner

arianvp commented Jun 14, 2020

Sorry this one. https://www.w3.org/TR/webauthn/#signature-attestation-types

The COSE one isn't asn.1 but fixed size. But the one in websauthn is for backwards compatibility reasons. Wow :/

@arianvp arianvp force-pushed the assertion branch 3 times, most recently from d24a28f to 4c0911c Compare June 15, 2020 15:23
@arianvp
Copy link
Owner

arianvp commented Jun 15, 2020

@duijf I fixed the signature code!

I don't know what else needs to happen in this PR, so I give it back to you

@duijf
Copy link
Collaborator Author

duijf commented Jun 15, 2020

I managed to get this to work 🎉

Also found a bug in our cookie settings while I was at this: we didn't set the cookie path, so we would have separate sessions per URL of our application. I changed this to set all cookies on /

You can test this in your browser by keeping an eye on your devtools while doing:

  • Click the "Test authentication" button and see a 401 response.
  • Registering a new credential
  • Click the "Test authentication" button and see a 200 response.
  • Click "Login" and see a 400 bad request (because you're already signed in).
  • Clear all cookies in your devtools without refreshing the page.
  • Click the "Test authentication" button and see a 401 response.
  • Click "Login" and use your Yubikey.
  • Click the "Test authentication" button and see a 200 response.

(Admittedly, this is not a convincing demo and bad UX. We can clean that up later, the most important thing is that this works!)

@duijf duijf requested a review from arianvp June 15, 2020 20:22
@arianvp
Copy link
Owner

arianvp commented Jun 15, 2020

Cool. Just tried and it works!

How about we add a userName field to the Login form as well; we can then look up the userHandle in the database and do a login even after a refresh

However we can also clean up the "UX" of the demo in a later PR.

@duijf
Copy link
Collaborator Author

duijf commented Jun 15, 2020

How about we add a userName field to the Login form as well; we can then look up the userHandle in the database and do a login even after a refresh

This sounds nice! I would like to wait on #15 to land before we spend time on this though

@duijf
Copy link
Collaborator Author

duijf commented Jun 15, 2020

Rebased and resolved conflicts. Am going to merge this

@duijf duijf merged commit 3ab9d7e into master Jun 15, 2020
@duijf duijf deleted the assertion branch June 15, 2020 21:12
@duijf duijf mentioned this pull request Jun 19, 2020
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.

None yet

2 participants