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

Fix for self-signed #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

inorganik
Copy link

@inorganik inorganik commented Oct 22, 2019

Sorry for all the whitespace changes, I auto-formatted in vscode.

This fixes #12

@estensen
Copy link

estensen commented Nov 11, 2019

Could you also add the fix for the branch completed-demo?

The current state of your completed-demo branch allows to register with a built-in sensor on macOS, but if you log out you can only log in with a security key.

@inorganik inorganik changed the base branch from master to completed-demo November 11, 2019 17:21
@inorganik inorganik changed the base branch from completed-demo to master November 11, 2019 17:22
@inorganik
Copy link
Author

I could, that would require a separate PR. I'd like to see if the maintainers want to merge this first though.

If you want to merge it into your completed-demo branch, you could do

$ git remote add inorganik [email protected]:inorganik/webauthn-demo.git
$ git fetch inorganik
$ git checkout inorganik/master --  utils.js

which should keep you on your completed-demo branch but take my updated copy of utils.js

@NFhbar
Copy link

NFhbar commented Feb 24, 2020

Could you also add the fix for the branch completed-demo?

The current state of your completed-demo branch allows to register with a built-in sensor on macOS, but if you log out you can only log in with a security key.

@estensen Did you find a solution for this?

@estensen
Copy link

@NFhbar Yes, I got it to work! Had some browser issues. Think I ended up using Chrome. Don't know it browser support has been improved since.

@NFhbar
Copy link

NFhbar commented Feb 24, 2020

@NFhbar Yes, I got it to work! Had some browser issues. Think I ended up using Chrome. Don't know it browser support has been improved since.

Yeah, works on Chrome just fine, but the login keeps forcing security key and has no built-in-sensor option. You got a link I can look at your implementation of this?

@estensen
Copy link

@NFhbar you have to copy the changes from this branch to the branch completed-demo. These are the only changes. It's a bit hard to see because of the automatic linting.

 } else if (ctapMakeCredResp.fmt === 'packed') { // Self signed
        let authrDataStruct = parseMakeCredAuthData(ctapMakeCredResp.authData);
        if (!(authrDataStruct.flags & U2F_USER_PRESENTED))
            throw new Error('User was NOT presented durring authentication!');

        const clientDataHash = hash(base64url.toBuffer(webAuthnResponse.response.clientDataJSON))
        const publicKey = COSEECDHAtoPKCS(authrDataStruct.COSEPublicKey)
        const signatureBase = Buffer.concat([ctapMakeCredResp.authData, clientDataHash]);
        const PEMCertificate = ASN1toPEM(publicKey);

        const { attStmt: { sig: signature, alg } } = ctapMakeCredResp

        response.verified = // Verify that sig is a valid signature over the concatenation of authenticatorData
            // and clientDataHash using the attestation public key in attestnCert with the algorithm specified in alg.
            verifySignature(signature, signatureBase, PEMCertificate) && alg === -7

        if (response.verified) {
            response.authrInfo = {
                fmt: 'fido-u2f',
                publicKey: base64url.encode(publicKey),
                counter: authrDataStruct.counter,
                credID: base64url.encode(authrDataStruct.credID)
            }
        }

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.

Can not authenticate signature
3 participants