Better DPK support

[agektmr]

• Rename devicePubKeyToStore to unregisteredDevicePubKey as it can so be used to determine that this is a new device and it should be rejected.
• Simplify DPK interface by returning a JSON serialized object as unregisteredDevicePubKey: DevicePublicKeyAuthenticatorOutputJSON rather than a binary DPK authenticator output devicePubKeyToStore: DevicePublicKeyAuthenticatorOutput.
• Expose DevicePublicKeyAuthenticatorOutputJSON for RP to store DPKs.

[MasterKale]

One thing I just discovered when trying to use alpha.1 is that I can't specify the devicePubKey extension when generating registration options. The extensions option for generateRegistrationOptions() still references AuthenticationExtensionsClientInputs:

https://github.com/MasterKale/SimpleWebAuthn/blob/beta/packages/server/src/registration/generateRegistrationOptions.ts#L25

Can you update these options to also reference AuthenticationExtensionsClientInputsFuture like in generateAuthenticationOptions()?

[agektmr]

Fixed. Hope this is what you meant.

[MasterKale]

What about instead using the convertAAGUIDToString() helper here to set a human-readable AAGUID like "adce0002-35bc-c60a-648b-0b25f1f05503" (arguably superior for humans to debug from), then adding a new "convertAAGUIDStringToBuffer()" method to convert a properly-formatted aaguid back to a Buffer for comparison the next time the user auth's?

If you go this route then I'd also like to see convertAAGUIDToString() renamed accordingly, probably to "convertAAGUIDBufferToString()"

[MasterKale]

Nice update 👍

[MasterKale]

The rename is fine with me, but I think it's confusing that VerifiedRegistrationResponse will now include unencoded Buffer's within registrationInfo, and base64url-encoded values within extensionOutputs.unregisteredDevicePubKey with this change.

Is it a matter of how difficult it becomes to store a DPK? Thinking about this a bit more, what about making unregisteredDevicePubKey a single Buffer that can be easily stored in a DB column, then passed back in during authentication?

I just looked at the L3 draft for what comes back for devicePubKey:

dictionary AuthenticationExtensionsDevicePublicKeyOutputs {
    ArrayBuffer authenticatorOutput;
    ArrayBuffer signature;
};

Could these authenticatorOutput and signature Buffer's be stored as-is? If so, maybe this method can simply return the two values back for the RP to store. Then generateAuthenticationOptions() could take those same two Buffer's back and the library handles splitting them apart.

I don't want to suggest a path forward until I feel like I've had a better idea of what you've tried so far.

[agektmr]

This is exactly the feedback I was looking for. I was torn between returning a buffer or a string because this depends on the fundamental concept of this library's "simplicity". I thought returning a buffer is better for consistency, but returning a string is far simpler.
My demo application stores these as strings in the database, so if it is returned as a buffer, the demo app has to encode and decode by themselves, which I thought was an unecessary operation.

But it's also true that how the data might be stored is up to he developer, so returning the data as the original format makes most sense.

I'm looking for one more feedback related to this, but will write that separately.

[agektmr]

RE: unregisteredDevicePubKey
I've got feedback that the demo app should show a data/time on the DPK that's last used. It's difficult in current state, because verifyAuthenticationResponse won't return the matched DPK. unregisteredDevicePubKey is returned only if the incoming DPK isn't registered yet.

What do you think is a good design to achieve this? I'm inclined to renaming unregisteredDevicePubKey to matchedDevicePubKey and let the RP determine whether the DPK is new or already registered, but this is fundamentally a repeat of what verifyAuthenticationResponse do.

I have some other ideas but I would like to hear your opinion on this.

[MasterKale]

But it's also true that how the data might be stored is up to he developer, so returning the data as the original format makes most sense.

This is why I switched to returning Buffer's from the verify methods; I used to encode to base64url but realized one day that it imposes a storage cost that an RP dev might not want to incur. As you mentioned too returning raw data allows the dev to choose the storage scheme that works best for their use case, which is why I'd like to see Buffer's here too.

[MasterKale]

What do you think is a good design to achieve this? I'm inclined to renaming unregisteredDevicePubKey to matchedDevicePubKey and let the RP determine whether the DPK is new or already registered, but this is fundamentally a repeat of what verifyAuthenticationResponse do.

Hmm, what about a single dpk property that is always the DPK object type, and then add a new "recognitionResult" property that is assigned one of type DPKRecognitionResult = "recognized" | "unrecognized"; as an enum-like indicating whether dpk is known, or a new one? 🤔

[MasterKale]

Thinking about it some more, I'd have an optional "dpk" property set to an object with a guaranteed DPK bytes and recognition result so that you would always have a recognition result for a DPK when devicePubKey returns something.

Does that make sense? I'm on mobile or I'd write it out. If I'm still not making sense then later I can get on a computer and pseudo-code what I mean.

Edit: Something like this:

export type RegistrationExtensionOutputs = {
  devicePubKey?: {
    value: Buffer; // Or maybe `DevicePublicKeyAuthenticatorOutput`?
    recognitionResult: "recognized" | "unrecognized";
  };
}

[agektmr]

I like that idea. I'll push a commit later.

[agektmr]

Implementing it, I realized this requires the developer to iterate through the stored DPK if it returns "recognized" and implement a very similar logic by themselves. What if we allow the input DPK to be inherited by an overridden type with some additional properties like id and last_used so that returned DPK object has sufficient information for the DPK to recognize which one it is?

[MasterKale]

Something to consider is this PR on WebAuthn repo with updates to the spec's credential record abstraction: w3c/webauthn#1812

DPK functionality in here may want to use similar naming when this lands.

[MasterKale]

Hey @agektmr, given that the concept of DPK is getting folded into "supplemental public keys" (see w3c/webauthn#1957) I think it's time to close this out. Thank you very much for spending time to try and get DPK support into here, perhaps we can work on this again when things settle down again. 🙇