DARC: Add support for Sovrin DID Verification #2197

gnarula · 2020-02-05T15:35:28Z

What this PR does

Adds support for Sovrin DID Resolution in Darcs using Indy CLI.
indy-cli is expected to be in $PATH. The pool configuration file is
assumed to be in os.UserConfigDir()/indy/genesis

DID Resolution depends on indy-cli which is expected to be in $PATH. The pool configuration file is assumed to be in `os.UserConfigDir()/indy/genesis` Signed-off-by: Gaurav Narula <gnarula94@gmail.com>

ineiti · 2020-02-05T16:10:25Z

Just to make sure: is it true that the darc-verification does not call out to the network.

Because it should always be possible to verify a signature even if there is no network. And old blocks might need to be replayed, so the signature verification should not depend on any services that are available only part-time.

gnarula · 2020-02-05T16:22:53Z

Just to make sure: is it true that the darc-verification does not call out to the network.

At the moment it does require a network call to resolve a DID to a DID Document (aka determining the public keys associated with a DID). One future improvement can be to cache the DID document.

I should add that using a cached copy comes with the risk of the conodes using obsolete public keys to verify messages.

Because it should always be possible to verify a signature even if there is no network. And old blocks might need to be replayed, so the signature verification should not depend on any services that are available only part-time.

The resolution for did:sov method relies on the Sovrin blockchain to resolve the DID document and is highly available. Please refer to https://github.com/sovrin-foundation/indyscan/blob/master/daemon/genesis-data.js for an example list of validator nodes to connect to.

I think an edge case would be a user having rotated their keys and the blocks with old signatures being replayed. I recall some discussions about retreiving DIDDoc at timestamp=t which should definitely be possible since DID Docs have a history of changes like DARCs ;) This isn't supported in this PR at the moment because indy-cli doesn't expose that functionality.

Signed-off-by: Gaurav Narula <gnarula94@gmail.com>

ineiti · 2020-02-06T07:04:09Z

I might still misunderstand what you're doing here. But from what I understand, this PR breaks one very big, important rule:

ClientTransactions must only depend on the global state and the information available in the block

If you break this rule, various things will happen:

replay is not possible anymore
some nodes will fail to confirm the ClientTransaction, while others might think it's OK
the evaluation of the ClientTransaction will be delayed because of network problems

This is the reason @jeffallen worked on the https://github.com/dedis/cothority/tree/master/authprox, to map external, centralised, network-accessed resources to something a ClientTransaction can trust.

I still might misunderstand what you're doing, but from what I understand now, you'll have to change this to be part of the authprox service!

jeffallen · 2020-02-06T13:53:57Z

Thanks for the feedback @ineiti . It is true that as I was helping Gaurav set his direction and plan this work I didn't take into consideration that ByzCoin needs to be self contained.

This work is fundamentally about adding a level of indirection between the Identity that signs a certain ByzCoin instruction and the public key that does the signing. The DID is the long-term name of a given identity, and DID resolution is the point where the DID is changed into the currently active public key. This code is currently structured close to right, in that it uses the DID as the signer's identity, and then does the resolution into a public key at the last possible moment before verifying.

The DID resolution step, in the case of Sovrin DID's, is handled by a permissioned blockchain. So what we are talking about is making a connection between two blockchains. The logical way to do that is for ByzCointo refer to a point in time (the block number) in the Sovrin chain.

So what I propose is to:

add an indication of which block number a certain DID should be resolved at to the Signer identity
in this PR or one coming soon to offer a second implementation of interface DIDResolver that can operate entirely in an offline manner, taking as input a genesis transaction (the trust root in the Sovrin world) and a set of blocks stored in a big file.

In this way, online nodes can resolve against the online Sovrin blockchain, and offline verifiers can still check that previous authentication decisions respected the DID holder's policy for which public key to use at the time when they sent in the tx.

jeffallen · 2020-02-06T14:45:43Z

Other comments for reviewers to be aware of:

How to talk to Sovrin blockchain

calling out to indy-cli is a short-term hack to get us up and running, the focus of this PR needs to be on the flow of tx authorisation data from clients through darc authorisation, and into the ByzCoin ledger.

Why we are not using authprox

Authprox is for a situation where the transaction signer has a credential but is not able/willing to hold/protect a private key. So instead, the key is stored in a key escrow service for them, and is then used to sign messages on their behalf. In the case of authprox the key escrow is not centralised, but a collective authority that holds shares of a secret and then use their share to contribute a partial signature onto a statement like, "this user proved to me that IdP X recognises them".

In the sovereign identity use case, a ByzCoin transaction originator is holding on to their secret key, and ready to use it to sign. But they want to get the benefits of the existing key management infrastructure provided by other parts of the sovereign identity world.

jeffallen · 2020-02-06T14:47:39Z

darc/darc.go

+		if err != nil {
+			return nil
+		}
+		for _, pk := range doc.PublicKey {


This is a rather strange way of saying if len(doc.PublicKey) > 0 return doc.PublicKey[0] else return nil;

@gnarula : this for is a nice solution if you have a map. But as PublicKey is a slice, it's much simpler as jeff suggests.

jeffallen · 2020-02-06T14:50:35Z

darc/darc.go

@@ -987,6 +1034,11 @@ func (idp IdentityProxy) Equal(i2 *IdentityProxy) bool {
 	return idp.Data == i2.Data && idp.Public.Equal(i2.Public)
 }

+// Equal returns true if both IdentityDIDs are the same.
+func (iddid IdentityDID) Equal(iddid2 *IdentityDID) bool {
+	return iddid.DID == iddid2.DID


Probably the part after the # should be removed before comparing them. You will be able to use Go's URL parser for this, see https://play.golang.org/p/MZpRQCWJxec

jeffallen · 2020-02-06T14:52:02Z

darc/darc.go

+// Verify returns nil if the signature is correct, or an error if something
+// fails
+func (iddid IdentityDID) Verify(msg, s []byte) error {
+	// TODO: Cache the results and set a TTL


As discussed in the comments, this code will need to be adapted to resolve a given DID at a given point in time.

jeffallen · 2020-02-06T14:52:54Z

darc/darc.go

+		key := cothority.Suite.Point()
+		err := key.UnmarshalBinary(publicKey.Value)
+		if err != nil {
+			return fmt.Errorf("error unmarshalling key: %v", err)


This should be a continue, so that a malformed key earlier in a DID Doc does not stop a correct key later in the DID Doc from working.

jeffallen · 2020-02-06T14:54:30Z

darc/darc.go

+		}
+		err = schnorr.Verify(cothority.Suite, key, msg, s)
+		if err == nil {
+			return nil


Put a comment here so that old dumb guys like me recognise this is the happy path return point. Or re-write this so that the only return point is at the bottom of the function...

jeffallen · 2020-02-06T15:03:10Z

darc/darc_test.go

+
+	// overriding the global in the test
+	didResolver = &IndyCLIDIDResolver{
+		Path:            "./mock_resolver_outputs/test_resolver",


Instead of running a test_resolver from the unit test, teach IndyCLIDIDResolver that if it has an io.Reader set, then it should read from that instead of spawning indy-cli. Then you can have the canned results right here in the unit test.

jeffallen · 2020-02-06T15:04:13Z

darc/darc_test.go

+
+	patientDarc := NewDarc(patientRules, []byte("Patient Darc"))
+
+	signature := "4b61e1e81fb17145d9a560724f16902673a5b9c86cb5bd330d51722fcd66b9311b1821e319e9160062f88eab476a9e3bdc5e770f85b194eec91585fea24d400e"


Include instructions to the next poor sucker how to generate this, if needed.

jeffallen · 2020-02-06T15:22:34Z

darc/didresolver.go

+	"time"
+
+	"github.com/hyperledger/aries-framework-go/pkg/doc/did"
+	"github.com/mr-tron/base58"


please use the same base58 already used by aries-framework-go, so as to reduce the dependencies: github.com/btcsuite/btcutil/base58

See also: hyperledger-archives/aries-framework-go#1192

jeffallen · 2020-02-06T15:29:02Z

darc/didresolver.go

+func (r *IndyCLIDIDResolver) Resolve(id string) (*did.Doc, error) {
+	output, err := r.executeCli(id)
+	if strings.Contains(output, "NYM not found") {
+		return nil, errors.New("DID not found in the ledger")


Please follow the standard for use of xerror as proposed by @nkcr .

jeffallen · 2020-02-06T15:32:29Z

darc/mock_resolver_outputs/test_resolver

@@ -0,0 +1,45 @@
+#!/bin/bash


These files will go away, the content can be inside of the unit tests directly. (less files = more good, within reason)

ineiti · 2020-02-07T07:45:48Z

Old issue is described in #2203

New idea, after discussion:

create a sovrin_pool contract where everybody can submit new pool proofs. The contract stores the latest proof of the pool-servers, together with the public keys of the pool-servers
every Instruction signature with DIDs must contain the resolved DID-object. instruction.VerifyWithOptions checks this signature using the sovrin-pool contract, and passes it to darc.EvalExpr if it matches.

gnarula · 2020-03-05T15:39:28Z

Closing this in light of the discussion and the new idea suggested above

DARC: Add support for Sovrin DID Verification

ba30180

DID Resolution depends on indy-cli which is expected to be in $PATH. The pool configuration file is assumed to be in `os.UserConfigDir()/indy/genesis` Signed-off-by: Gaurav Narula <gnarula94@gmail.com>

gnarula requested a review from jeffallen February 5, 2020 15:35

gnarula added this to WIP in Cothority via automation Feb 5, 2020

gnarula moved this from WIP to Ready4Merge in Cothority Feb 5, 2020

gnarula added 2 commits February 5, 2020 17:27

DARC: Update proto

93152d3

Signed-off-by: Gaurav Narula <gnarula94@gmail.com>

Fix lint errors

44ac9c6

Signed-off-by: Gaurav Narula <gnarula94@gmail.com>

ineiti moved this from Ready4Merge to WIP in Cothority Feb 6, 2020

jeffallen reviewed Feb 6, 2020

View reviewed changes

gnarula closed this Mar 5, 2020

Cothority automation moved this from WIP to Closed Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DARC: Add support for Sovrin DID Verification #2197

DARC: Add support for Sovrin DID Verification #2197

gnarula commented Feb 5, 2020

ineiti commented Feb 5, 2020

gnarula commented Feb 5, 2020 •

edited

ineiti commented Feb 6, 2020

jeffallen commented Feb 6, 2020

jeffallen commented Feb 6, 2020

jeffallen Feb 6, 2020

ineiti Feb 7, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

jeffallen Feb 6, 2020

ineiti commented Feb 7, 2020 •

edited

gnarula commented Mar 5, 2020


		patientDarc := NewDarc(patientRules, []byte("Patient Darc"))

		signature := "4b61e1e81fb17145d9a560724f16902673a5b9c86cb5bd330d51722fcd66b9311b1821e319e9160062f88eab476a9e3bdc5e770f85b194eec91585fea24d400e"

DARC: Add support for Sovrin DID Verification #2197

DARC: Add support for Sovrin DID Verification #2197

Conversation

gnarula commented Feb 5, 2020

ineiti commented Feb 5, 2020

gnarula commented Feb 5, 2020 • edited

ineiti commented Feb 6, 2020

jeffallen commented Feb 6, 2020

jeffallen commented Feb 6, 2020

How to talk to Sovrin blockchain

Why we are not using authprox

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ineiti commented Feb 7, 2020 • edited

gnarula commented Mar 5, 2020

gnarula commented Feb 5, 2020 •

edited

ineiti commented Feb 7, 2020 •

edited