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

Document that in the $HealthWallet API, you can only have one DID per account #21

Closed
jmandel opened this issue Dec 10, 2020 · 16 comments
Closed

Comments

@jmandel
Copy link
Member

jmandel commented Dec 10, 2020

If you call $HealthWallet.connect a 2nd time, this replaces the originally-bound DID with a new one.

@daliboz
Copy link

daliboz commented Jan 15, 2021

@jmandel how does this work with multi-device (multi-app) or lost devices?

@jmandel
Copy link
Member Author

jmandel commented Jan 15, 2021

I think the answer is: if you're binding a DID to your account, you'd bind a new one and then call $HealthWallet.issueVc again.

@madaster97
Copy link

madaster97 commented Jan 21, 2021

Support Multiple Wallets (Multi-Device and Multi-App)

I think EHR-Issuers and their Patients will want to support multiple wallets per Patient, just having multiple DIDs linked to an account.

To support this, the EHR will need to know which Holder a VC is being issued for, since this will determine what DID document/JWE encryption key is used by the issuer.

This is easy enough to support for download of fhir-backed-vc files, since the issuer portal can just ask the patient which wallet the file is destined for. The portal could also try and be smart about this, where it can sense what device the user is logged in with and only show wallets that it knows came from that device (maybe registering an IP address for the device during the SIOP flow).

For supporting this with $HealthWallet resources, the best answer would be to include a holderDid parameter in the POST /Patient/:id/$HealthWallet.issueVc API call.

Here's an example payload:

{
    "resourceType": "Parameters",
    "parameter": [{
        "name": "credentialType",
        "valueUri": "https://healthwallet.cards#covid19"
    },
    {
        "name": "holderDid",
        "valueUri": "did:ion:<<identifer for holder>>"
    }]
}

Note this also covers the niche scenario of a patient having the same wallet installed on multiple devices. Those wallets will have the same client_id, BUT have separate DIDs (unless they're sharing private keys, but that seems out-of-scope). Without this parameter, EHRs wouldn't be able to recognize the difference between these two wallets issuing a $HealthWallet.issueVc call.

@madaster97
Copy link

madaster97 commented Jan 21, 2021

Lost Devices / Wallet Key Rotation

@daliboz for your question on lost devices, I think there are two useful tools/operations:

  1. SideTree Update Operation - Maybe not for a lost device, but if the signing/encryption key for the device (and NOT the update/recovery keys) were compromised, then the wallet could just remove those old keys and replace them in the DID Doc. This has the following implications for issuers:
    • The issuer should be able to rely on the <did-suffix> portion of a did:METHOD:<did-suffix>:<long-form-suffix-data> to be a UNIQUE IDENTIFIER for the DID. Practically, having this ID be unique across a single patient's Wallet DIDs would suffice.
    • For a wallet that needs to update its DID document, EHRs could use the <did-suffix> as an identifier to register that it knows the DID presented, but that's it's corresponding DID document has been edited.
    • To get these new DID docs to an issuer, a SIOP flow or $HealthWallet.connect operation could be used. Issuers would need to check if the presented <did-suffix> already exists on the patient/user, and simply replace the registered public keys for that suffix/wallet.
  2. Brand new SIOP flow/$HealthWallet.connect Call - This is the nuclear option, only used if the device is lost OR all private keys compromised. The lab will generate a new DID/Wallet link to a patient, WITHOUT deleting the old one. There needs to be some out-of-band method for removing a wallet from an lab/issuer account, such as a patient-managed UI in the lab portal. Issuers will likely implement this either way.

@jmandel
Copy link
Member Author

jmandel commented Jan 21, 2021

The goal is to make sure the specification is usable even without dids ever being anchored to a blockchain. So I would rather leave things like lost devices out of scope and only provide the "nuclear" option for v1 (i.e., the user can link a new did if they need to).

There are definitely paths that we can pursue to do better over time, but I don't want to get distracted by them at this stage. Is there something critical that won't work if we just leave this out of scope initially @daliboz ?

@daliboz
Copy link

daliboz commented Jan 21, 2021

@jmandel I worry how we would inform the user that by connecting a new app (or new device/same app) that they're about to invalidate/remove previous associations - and how that would affect any issued vc as well (obviously no new VCs would go out for the old app). Is the assumption that we can prompt the user during the connect phase? Losing or getting a new device is a pretty common occurrence.

@jmandel
Copy link
Member Author

jmandel commented Jan 21, 2021

how that would affect any issued vc

"Connecting a new did" to an issuer shouldn't affect (shouldn't invalidate!) any existing VCs already issued

how we would inform the user that by connecting a new app (or new device/same app) that they're about to invalidate/remove previous associations

I'm not sure this has important user-facing consequences. If a user has >1 device each with a different mobile wallet, then each wallet can do "$HealthWallet.connect-and-then-$HealthWallet.issueVc" whenever it needs to check for updated credentials; wallets can choose to prompt the user or can remember previous decisions like "Share my DID with Issuer 123", and this should keep working as long as each client is still authorized with __HealthWallet scopes.


In other words: if we were going to "tackle" this problem, it'd be by saying:

  • Issuers accumulate a list of connected DIDs for each user, adding to the list each time $HealthWallet.connect is completed
  • Issuers expose a $HealthWallet.clearAll() and/or $HealthWallet.disconnect(oneDid) operations to allow finer-grained management

My take is we can always migrate to this model in the future based on actual need. But we can do fine without it in v1.

@madaster97
Copy link

@jmandel @daliboz I don't think the expectation is that calling a $HealthWallet.connect clears any previous associations.

I think its relevant to better define what a wallet is. See details here. With that in mind, here's how I'd see a revocation/lost device workflow playing out.

Revocation Workflow

The above definition of a "Wallet" implies that a given wallet should only have to call $HealthWallet.connect ONCE, when they initially register their DID with that user's lab account (and per patient, see linked issue). Given Josh's comment about NOT anchoring to the block-chain, this is what I see being the workflow for a lost device:

  1. When setting up wallet 1 via SMART on FHIR, the wallet calls $HealthWallet.connect ONCE to register it's DID with that user's lab account
  2. Some time later, VCs are issued to that wallet via a $HealthWallet.issueVc call. Note the user could logout of the app, reconduct a SMART on FHIR launch later and NOT need to call connect again to be issued VCs. Issuers should be able to remember DIDs, and not tie them to a specific access_token (by, for instance, encoding DID state into that token on a connect call.
  3. The user loses their device. At this point, it's up to the user to go into their lab/issuer portal and unregister the existing wallet.
    • One assumption here is that a lost device has no way of re-securing it's private keys. If those keys weren't actually stored locally, and the user can basically back-up their wallet onto another device, then revocation isn't needed.
  4. When re-installing their new wallet, wallet 2 would use $HealthWallet.connect just like wallet 1 did, and re-bind a brand new DID to the user/patient account. It would be nice if wallet 2 could somehow access/remember the issuers linked to wallet 1, and suggest the patient

@jmandel
Copy link
Member Author

jmandel commented Jan 21, 2021

I don't think the expectation is that calling a $HealthWallet.connect clears any previous associations.

I mean, we're defining "the expectation" in the spec -- or we should be. I'm not hearing a value proposition that leads me to think we need complex semantics here. (Obviously we could introduce these semantics and capabilities, but doing so has an implementation cost; my aim at this stage is to keep the surface area as small as it can reasonably be.)

@madaster97
Copy link

That's a good point. I think the value proposition here is to not limit if/how issuers support multiple wallets per user.

I think the change we should make is to have the holderDid a required parameter for $HealthWallet.issueVc, that way issuers can reliably know what wallet is making the request even in multi-wallet scenarios.

This leaves it up to the issuer to handle the multiple wallets question, and we would not explicitly define any other expectation of the $HealthWallet operations.

In this, I'm mostly trying to have parity between the different workflows in this spec, and I think adding a holderDid field achieves that with no need for extra guidance to issuers.

@jmandel
Copy link
Member Author

jmandel commented Jan 22, 2021

Thanks again for the precision here. I'm open to this addition of holderDid as a property on issueVc, with

  • the pre-condition that any DID passed in this param must have been previously linked by calling connect
  • an explicit note that issuers MAY choose to maintain only a single linked wallet at a time, and MAY clear out previous links each time connect is called

(One other thought is that we might want to specify a "Wallet Name" as part of the connect operation, so if issuers want to show users a list of connected wallets -- maybe with "disconnect" buttons 😉 -- there's a way to label the options. On further consideration, this is well enough managed through existing SMART app authorization.)

@p2-apple
Copy link
Collaborator

p2-apple commented Jan 22, 2021

This is a tricky topic. I think an important consideration is that the user should never have to manually manage her DIDs, this topic is way too confusing for most users. I believe we have 2 sensible approaches:

Only ever keep one DID associated with a user account

This means calling $HealthWallet.connect with a DID different from the one uploaded before will replace whatever was on the server. In the 2+ device scenario, where private keys are not synced and where each device has a unique DID, this will ping-pong between DIDs. From my POV this shouldn't be an issue because:

  1. Each device still has valid VCs
  2. I know at least one server implementation intends to generate and sign the VCs when $HealthWallet.issueVc is called, so even if the account-associated DID changes frequently it's not additional load on a server
  3. The scenario where a user has 2+ active devices, each with its unique DID, is sufficiently rare

Servers maintain a list of associated DIDs

This means an account can be associated with as many DIDs as the user will upload. Each device will need to specify which DID should be used for VC delivery on $HealthWallet.issueVc. This means the server will either only package & sign the VCs on delivery or it maintains a JWS for every DID associated with the account. It seems this will result in about the same workload on the server as only allowing one DID and signing on request.

The downside to this approach I believe is that DIDs will accumulate when users change/lose phones and few users will ever care or even know that they can disassociate DIDs with their account.

So I agree with Josh, at least for v1 it seems preferable that the expectation is that $HealthWallet.connect will replace a DID if it's different from what's already associated with the account. It's entirely possible that I'm missing something, however, so happy to discuss this further!

@jmandel
Copy link
Member Author

jmandel commented Jan 22, 2021

@p2-apple thanks for this. Would like to get your thoughts on allowing the behavior where issuers maintain multiple links, but not specifying that they must. (Basically, @madaster97's proposal to add a holderDid field to the issueVc operation, but leave it up to issuers to decide whether to "clear out" previous DIDs each time connect is called.) It strikes me as a reasonable middle ground, but I'm still wary of the added complexity.

@p2-apple
Copy link
Collaborator

This comes down to whether or not a client has to specify holderDid when requesting VCs. It will somewhat obviate the need for the no-did-bound OO.

What I like about this is that it makes the flow more deterministic, i.e. a device does not receive VCs bound to whichever DID was uploaded last. One can imagine a race condition where two devices with different DIDs attempt to download VCs at the same time, so this suggestion would prevent such a scenario. I like that!

The question then becomes what do servers do that do NOT support 2+ bound DIDs. We will need a new OO to let the client know that it first has to issue $HealthWallet.connect before it can request VCs for that DID.

I'm leaning towards making this change because it enables issuers to choose how many bound DIDs they want to support with a more robust API story.

@jmandel
Copy link
Member Author

jmandel commented Jan 22, 2021

This comes down to whether or not a client has to specify holderDid when requesting VCs. It will somewhat obviate the need for the no-did-bound OO.

The question then becomes what do servers do that do NOT support 2+ bound DIDs. We will need a new OO to let the client know that it first has to issue $HealthWallet.connect

Yeah, I think we basically reaplace the "No DID Bound" OperationOutcome to a "That DID isn't bound" error, and everything works pretty deterministically from there.

Good!

@jmandel
Copy link
Member Author

jmandel commented Feb 13, 2021

Wallet binding is no longer part of the spec since #64 .

@jmandel jmandel closed this as completed Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants