webauthn-open-source / fido2-lib

A node.js library for performing FIDO 2.0 / WebAuthn server functionality
https://webauthn.io
MIT License
394 stars 118 forks source link

Provide More Options Public Key Exporting and Argument Formatting #143

Open knightcode opened 8 months ago

knightcode commented 8 months ago

We have to store a public key in the interim time between the result of attestationResult and any subsequent calls to assertionResult. And I'm looking to for the most flexible yet compact storage format, i.e. a format that doesn't lock me into this library if it stops getting maintained and one that plays nice with subtle.importKey() or node:crypto.createPublicKey() in case that ever becomes useful in the future. And we have the added constraint that it has be rendered into a string in PEM format for the assertionResult() call.

It's this later constraint that I find problematic because I could give you the public key in a format that doesn't require a much preprocessing, namely that we encode it to PEM so that you can decode it from PEM.

Given that we have three options for acquiring the public key, namely result.authnrData.get( ___ ) with "credentialPublicKeyPem", "credentialPublicKeyCose", or "credentialPublicKeyJwk" as arguments to the Map call, my current solution is to get the PEM format, strip it down to a base64 string that can be parsed into a Buffer (Uint8Array) and then store the decoded bytes. Jwk would be an option that Subtle supports, but it's not as compact.

Then for assertionResult, I have to do this ugly thing:

const pubKeyStr = publicKeyBuffer.toString("base64");
const pubArr = pubKeyStr.match(/.{1,64}/g);
if (!pubArr) { throw .... }
const pemStr = "-----BEGIN PUBLIC KEY-----\n" + pubArr.join("\n") + "\n-----END PUBLIC KEY-----\n";

await fidoLib.assertionResult({
      id: new TextEncoder().encode(creds.id).buffer,
      rawId: new Uint8Array(creds.rawId),
      response: {
        clientDataJSON: response.clientDataJSON.toString("base64"),
        authenticatorData: new Uint8Array(response.authenticatorData).buffer,
        signature: response.signature.toString("base64"),
        userHandle: response.userHandle.toString("base64url")
      }
}, {
      challenge: lastChallenge.toString("base64"),
      origin: ...,
      factor: "either",
      publicKey: pemStr,
      prevCounter: ...,
      userHandle: response.userHandle.toString("base64url") // already assured of a match with rawId check (not shown)
});

As such, I would humbly request that assertResult be a little more open in the formats that it accepts. And some added documentation about what these formats can and should be would have saved me a ton of trial & error combined with digging through and instrumenting the source code. I don't think it would be too difficult to test the inputs for string or buffer types and act accordingly under some documented assumptions.

Side note, raw ArrayBuffers are the worst. With like any other type, I could give you a view of some segment of memory and avoid copying bytes around.

JamesCullum commented 8 months ago

Hey @knightcode , I'm not sure if I fully understand your requirement about not being locked into this library - it's an open-source repository and we use an open standard, so if you save the result that a standardized browser responds, any library would be able to continue working with it (not that saving attestationResults really is a best practice and a migration with an intermediate browser response is a good idea). Generally best practice is not to do any validation or mutation yourself, as you might miss something that the library would've caught or prevented.

In any case, we are an actively maintained community library that welcome every PR. If you can improve the Buffers and extend the format options, we'd be very happy about your contribution 👍

knightcode commented 8 months ago

@JamesCullum , I guess there's two concerns:

  1. centers around the data we have to store long term in the DB, specifically the public key. If I were to ever switch libraries, I would want to be able to feed those stored values to the new library. And so I want to ensure the maximum flexibility there to future-proof our implementation. I'm choosing the PEM format, but I just think it's wasteful to store the string. I don't think there's anything you need to do there... unless you want to consider a credentialPublicKeyDER option that returns buffer for us. (Side note: the MasterKale/SimpleWebAuthn project requires the public key be supplied in binary CBOR format, which is even more difficult to consider storing in the db)
  2. I already have binary data for most of the parameters for assertionResult, so that my request is for that method to accept strings or buffers.
JamesCullum commented 8 months ago

Thanks for the feedback - I'm still not sure if I fully get the point.

1) If you are saving a normal public key, you can compress and reformat it as needed, but as long as you can always reverse to the key, you have full compatibility.

2) Can you describe what you'd like your example code on top to look like? Maybe this makes it more clear to me what you'd be interested in.

Do I understand you correctly that you are not yet able to take care of the PR yourself? We can label it as "Help wanted" and see if someone has the same pain point and can implement it.

knightcode commented 8 months ago

Maybe I'll get time to work on this, but I don't think it'll be soon. Also, by no means am I an expert on packing certificates and public keys, e.g. I assume SimpleWebAuth had some argument for CBOR or anything else, but I have no idea what it was. And I couldn't find any js libraries that translated from ASN.1 to CBOR or back. Someone else could do better at that than me.

But this is what I would like to be able to write:

await fidoLib.assertionResult({
      id: creds.id, // Buffer or TypedArray or ArrayBuffer
      rawId: creds.rawId, // Buffer or TypedArray or ArrayBuffer
      response: {
        clientDataJSON: response.clientDataJSON, // base64 string | Buffer | TypedArray | ArrayBuffer
        authenticatorData: response.authenticatorData, // Buffer | TypedArray | ArrayBuffer
        signature: response.signature, // base64 string | Buffer | TypedArray | ArrayBuffer
        userHandle: response.userHandle // base64 string | Buffer | TypedArray | ArrayBuffer
      }
}, {
      challenge: lastChallenge, // base64 string | Buffer | TypedArray | ArrayBuffer
      origin: ...,
      factor: "either",
      publicKey: userAuthenticator.publicKey, // Object (JWK) | String (PEM) | Buffer/ArrayBuffer (COSE or DER if easy to differentiate)
      prevCounter: ...,
      userHandle: ... // base64 string | Buffer | TypedArray | ArrayBuffer
});

If assertionResult was this flexible with its inputs, I could marshall the credentials to the server in their existing binary format and reduce the call down to:

await fidoLib.assertionResult(clientsideCredentials, {
      challenge: lastChallenge,
      origin: ...,
      factor: "either",
      publicKey: userAuthenticator.publicKey,
      prevCounter: ...,
      userHandle: ...
});
JamesCullum commented 8 months ago

Thanks for the example, this really helped me understand what you are trying to achieve. I agree that more formats would always be nice, however I'm not sure if the difference between the code you currently need to use and the one you propose is different enough to warrant a change in the package - this sounds like a helper method would be the more appropriate solution.

The reason you have the data in this shape is because you want to implement non-standard use cases specific to your application and you do proprietary changes to execute this. It would be very difficult to implement and maintain a compatible interface for each of those - and the work and time would exceed the one of a simple helper method on your side by a wide margin.

We can use the voting function for this comment to see if there are enough users that would like to see exactly the same interfaces (thumbs up = yes). If it gets enough traction, there surely is a developer who can implement it.