Passing opaque user data up from `CredentialRepository`?

[iaik-jheher]

Not necessarily an issue, but the JavaDoc isn't entirely clear on this. (Or, if it is, I couldn't find it - apologies if so.)

I am trying to integrate passwordless authentication using client-side discoverable credentials into existing infrastructure, which imposes some limitations.

We intend to store (credential, user identifier) pairs, indexed by credentialId. We identify users internally using an identifier that is personal data, and which we thus cannot store directly in the user handle. As I understand it, the user handle's primary purpose is deduplication of resident credentials; therefore, we intended to specify a keyed hash digest (using a server private key) as the user handle. This is of course not reversible, but since we index all known credentials by their ID, this is not an issue for us.

I am now trying to map this behavior onto the CredentialRepository interface.

• getCredentialIdsForUsername: is returning an empty set permissible when using client-side discoverable credentials (e.g., this is only used to populate allowCredentials) or would this cause validation to fail?
• getUserHandleForUsername: is this used/required when using client-side discoverable credentials?
• getUsernameForUserHandle: is this used when validating the received credential, or can I safely return Optional.empty?
• lookup: I assume this is used when validating a credential; I would retrieve the credential by ID, re-calculate the user handle, and return Optional.empty on failure; does this match what the library expects?

[emlun]

• getCredentialIdsForUsername: is returning an empty set permissible when using client-side discoverable credentials (e.g., this is only used to populate allowCredentials) or would this cause validation to fail?

This both would and would not work. This method is used for two things:

• To set allowCredentials in startAssertion, if the StartAssertionOptions includes a username or user handle. In your use case using only discoverable credential flows (empty allowCredentials), that is not a problem, and verification will work even if getCredentialIdsForUsername() always returns empty.
• To set excludeCredentials in startRegistration. This is where things break. excludeCredentials is used by the client and authenticator to make sure that the user doesn't try to register the same authenticator twice, since having two credentials on the same authenticator is just confusing with no benefit. Thus I recommend setting excludeCredentials in each registration ceremony to include all of the user's credential IDs, which is what the library uses getCredentialIdsForUsername() for. This way users will see an error message like "This security key/phone/etc is already registered" if they try to register the same authenticator twice.

• getUserHandleForUsername: is this used/required when using client-side discoverable credentials?

This is used to check that the username and user handle refer to the same account, in case StartAssertionOptions contains a username and the assertion response also contains a user handle. StartAssertionOptions won't contain a username in your use case, so it should be fine for getUserHandleForUsername() to always return empty.

• getUsernameForUserHandle: is this used when validating the received credential, or can I safely return Optional.empty?

This is required at the moment, due to how the logic in finishAssertion is structured.

It's also used to set allowCredentials in the case when StartAssertionOptions contains a user handle but not a username, though that case isn't relevant to your use case.

• lookup: I assume this is used when validating a credential; I would retrieve the credential by ID, re-calculate the user handle, and return Optional.empty on failure; does this match what the library expects?

Yes, that sounds right. The library uses the lookup() result most importantly to verify the signature and validate the signature counter and backup bits, but also validates that the userHandle in the lookup() result matches the user handle in the assertion (either returned from a discoverable credential or looked up from StartAssertionOptions contents as mentioned above).

Does that help? Anything I missed?

[iaik-jheher]

Ah, I missed the excludeCredentials use case. Hm, this presents somewhat of a conundrum.

If I set username to the internal (personal-data) identifier, this allows me to implement getCredentialIdsForUsername, but would not allow me to calculate this username from the userHandle in my current model.

If I set username to be equal to the userHandle (or, well, encoding thereof) this makes getUsernameForUserHandle trivially possible, but getCredentialIdsForUsername might require additional data storage.

It is something I'll have to think about. Thanks for the detailed description!

[emlun]

If you can spare the bytes I would recommend storing the user handle as an opaque value in the user record, even if it's derived from other values. For example, should you ever need to rotate that hash key, you would no longer be able to re-derive user handles older than the new key (and thus validation for those discoverable credentials would fail under the current library implementation). Or you'd have to keep record of which hash key was used for which user account, at which point you might as well store the user handle directly instead.

[emlun]

I hope you got your questions answered! You're welcome to re-open the issue if you have more.

[iaik-jheher]

If you don't mind me requesting further input, since I'm really trying to avoid "fighting the library" here: Assume I have some opaque server-generated metadata that is attached to each credential. My credential repository's lookup method does a relatively expensive back-end query, and ends up retrieving this metadata alongside the "standard" credential information.

What would an "appropriate" way of passing this metadata out of the credential repository and through the library code be that ends up with the metadata accessible to my application logic?

My naive approach would've been to simply subclass RegisteredCredential, return instances of this subclass from my CredentialRepository implementation, then cast AssertionResult's getCredential() return back to the subclass. However, this plan is foiled by RegisteredCredential being marked final.

Is there some other "supported" way to pass opaque data alongside a RegisteredCredential up from the credential repository? I'm trying to avoid having to make a second redundant back-end query, of course.

[emlun]

Hm, good questions. There is no "appropriate" way to do this right now, but it certainly seems like it would be a common need, so I am in favour of adding one. A simple approach that immediately springs to mind is to add a new field like extraData: Optional<Object> to RegisteredCredential, which is unused by the library but passed through to the AssertionResult. But I'll think some more on it and see if it can be done in a more type-safe manner that's also backwards compatible - I would like to add a type parameter for the extraData type, but that would break existing CredentialRepository implementations. But maybe it could be a new, optional interface method instead, or something like that.

The reason the values types are marked final is in part to prevent subclasses from inadvertently breaking security assumptions made by the library. This honestly isn't very relevant for how the library uses RegisteredCredential, but I'm still wary of dropping the final for now.

[emlun]

In the meantime while we figure that out, there does exist a workaround. It's a horrible hack, definitely qualifying as "fighting the library", but it does seem to work: you can smuggle extra data in the CBOR of the publicKeyCose. For example, instead of setting this publicKeyCose value:

CBOR (hex):
a5010203262001215820bc5495267ae12c98a0d053ea92a05497a270d59146e20e87d5a5776362a6a204225820102ecbdacadd9aa7957b5536c9256e97e917b541895d1d9f63f65f222977de03

Parsed:
{1: 2, 3: -7, -1: 1, -2: h'BC5495267AE12C98A0D053EA92A05497A270D59146E20E87D5A5776362A6A204', -3: h'102ECBDACADD9AA7957B5536C9256E97E917B541895D1D9F63F65F222977DE03'}

you could set this:

CBOR (hex):
a6010203262001215820bc5495267ae12c98a0d053ea92a05497a270d59146e20e87d5a5776362a6a204225820102ecbdacadd9aa7957b5536c9256e97e917b541895d1d9f63f65f222977de03656578747261a211182a63666f6f63626172

Parsed:
{1: 2, 3: -7, -1: 1, -2: h'BC5495267AE12C98A0D053EA92A05497A270D59146E20E87D5A5776362A6A204', -3: h'102ECBDACADD9AA7957B5536C9256E97E917B541895D1D9F63F65F222977DE03', "extra": {17: 42, "foo": "bar"}}

For example, your application code could do something like this:

import com.upokecenter.cbor.CBORObject;
CBORObject extra = CBORObject.NewMap();
extra.set("foo", CBORObject.FromObject("bar"));

CBORObject cose = CBORObject.DecodeFromBytes(publicKeyCose.getBytes());
cose.set("extra", extra);
ByteArray newPublicKeyCose = new ByteArray(cose.EncodeToBytes);

and then extract the data back out from AssertionResult.getCredential().getPublicKeyCose(). Of course, CBOR supports opaque binary values (for example the -2 and -3 values above), so you can avoid having to convert your data to a CBOR structure if you can instead serialize it to a string or byte string.

Again, this is a horrible hack, but it could work in a pinch.

[iaik-jheher]

Optional<T> getExtraData(Class<T>) sounds like a potential solution for the type safety issue, mapping an internal ClassCastException to an empty Optional.

[emlun]

Alright, sorry for the delay, but I've now had some time to try sketching some things out along the lined described in #289 (comment). I'm not sure where this will go in the end, but I like some aspects of this prototype at least.

Please take a look at the experimental/credential-repository-v2 branch. In short, this adds a pair of new CredentialRepositoryV2<C extends CredentialRecord> (name to be finalized) and UsernameRepository interfaces, and the RelyingParty builder now branches the resulting type depending on if you call the .credentialRepository() method with an instance of CredentialRepository or CredentialRepositoryV2<C>. In the latter case, the resulting
type also changes from RelyingParty to RelyingPartyV2<C>.

A couple of things, including some other types, change with RelyingPartyV2:

• CredentialRepositoryV2<C> is parameterized with the type of your credential record, and so is AssertionResultV2<C>, so AssertionResultV2.getCredential() now returns that generic type C instead of the concrete type RegisteredCredential. C must implement the new interface CredentialRecord but may also contain any other methods you wish, and retains its full type at compile time.
• Usernames are no longer supported by default, but can be supported by setting a .usernameRepository() in the RelyingPartyV2 builder. Right now this mostly affects runtime behaviour - finishAssertion() won't attempt to look up a username if this isn't set. I would like for the StartAssertionOptions.username parameter to not exist unless a UsernameRepository is set, but haven't worked out the type infrastructure for that yet.

So, although this is far from finished just yet - does this look like a path forward that would help your use case?

I'm having the next two weeks off so I might not respond within that time, but I'll continue with this when I'm back.