Add verification methods #74

vqhuy · 2016-08-18T18:36:07Z

To be paired w/ #86

Temporarily ignore the cgo client library until we have a complete protocol/client library.

masomel · 2016-08-23T15:58:22Z

protocol/verification.go

+		}
+	}
+
+	// step 2. verify STR's signature


I think it makes more sense to verify the STR signature before verifying the TB's signature, since the TB's signature depends on the STR's signature.

Q: Which epoch did this lookup happen in? A: verifyFulfilledPromise must take a STR as its argument. This STR should be verified in updateSTR (e.g., epoch transitioning etc ...).

vqhuy · 2016-11-14T10:36:56Z

@masomel @arlolra @liamsi : I merged branch str2 into this and did some refactors. Please take a look :)
(Agrr, sorry for this long pull with many changes).

masomel

I like what you guys did, @c633 and @arlolra. I think the consistency checks make a lot more sense now. Most of my comments are more about code cleanup (e.g. redundant params).

masomel · 2016-11-14T17:33:07Z

protocol/consistencychecks.go

+		if err := cc.verifySTR(str); err == nil {
+			return nil
+		}
+		// Otherwise, expect that we've enterred a new epoch


s/enterred/entered

masomel · 2016-11-14T17:44:05Z

protocol/consistencychecks.go

+		// FIXME: check whether the STR was issued on time and whatnot.
+		// Maybe it has something to do w/ #81 and client transitioning between epochs.
+		// Try to verify w/ what's been saved
+		if err := cc.verifySTR(str); err == nil {


https://github.com/coniks-sys/coniks-go/blob/cgo-client/protocol/consistencychecks.go#L257 states that verifySTR() should only be called in the case that requestType is KeyLookupType, but here it's clearly called for a RegistrationType as well. I'm probably missing something, but won't verifySTR() return an ErrorBadSTR in that case? I'm unclear at which point the cc is initialized with an STR before the registration response is received that won't cause verifySTR() to fail. (I see that in the tests we sort of "cheat" and just pass in d.LatestSTR(), but this presumably won't be the case in practice).

Here is @arlolra explanantion:
#74 (comment)

I had originally written it like this, 80e453c

But Huy made this change, 8e97d88

I'm assuming because of #74 (comment)

However, we've since opened #106 and agreed to do TOFU for the time being, so that should probably be reverted too? Huy?

Maybe he was thinking we'd do a #119 first?

80e453c makes me feel like a lazy developer can always pass a nil pointer of savedSTR instead.

Since we don't have #119 yet, so yes, I agree it should be reverted too.

I appreciate your concern.

I see, thanks for this explanation.

masomel · 2016-11-14T17:48:33Z

protocol/consistencychecks.go

+			return nil
+		}
+		// Otherwise, expect that we've enterred a new epoch
+		if err := cc.verifySTRConsistency(cc.SavedSTR, str); err != nil {


Similarly to above, https://github.com/coniks-sys/coniks-go/blob/cgo-client/protocol/consistencychecks.go#L238 states that verifySTRConsistency() should only be called in the case of a RegistrationType or a MonitoringType. So I'm wondering why the registration and key lookup responses are being handled the same way.

Sorry, the comment is no longer true. See #74 (comment).

masomel · 2016-11-14T17:49:41Z

protocol/consistencychecks.go

-// This uses the pinning signing key in cc
+// verifySTRConsistency checks the consistency between 2 snapshots.
+// This should be called if the request is either registration or
+// monitoring. It uses the pinning signing key in cc


s/pinning/pinned

masomel · 2016-11-14T17:53:00Z

protocol/consistencychecks.go

-			return invalidProof, ErrorBadCommitment
-		}
-		proofType = proofOfInclusion
+		key = ap.Leaf.Value


I would add to the comment that we accept the received key as TOFU.

masomel · 2016-11-14T18:06:04Z

protocol/consistencychecks.go

-	case msg.Error == ErrorNameNotFound && proofType == proofOfAbsence:
-	case msg.Error == Success:
+	case msg.Error == ErrorNameNotFound && proofType == m.ProofOfAbsence:
+	case msg.Error == Success && proofType == m.ProofOfInclusion && tb == nil:


This case is going to change when we support key changes, so I would add a FIXME or TODO to indicate this.

masomel · 2016-11-14T18:13:46Z

protocol/consistencychecks.go

-func (cc *ConsistencyChecks) verifyReturnedPromise(requestType int,
-	responseCode ErrorCode, df *DirectoryProof,
+// These above checks should be performed before calling this method.
+func (cc *ConsistencyChecks) verifyReturnedPromise(df *DirectoryProof,


Update the comment: https://github.com/coniks-sys/coniks-go/blob/cgo-client/protocol/consistencychecks.go#L286 states that verifyReturnedPromise() operates based on the request type, which no longer is true.

masomel · 2016-11-14T18:15:29Z

protocol/consistencychecks.go

-func (cc *ConsistencyChecks) verifyReturnedPromise(requestType int,
-	responseCode ErrorCode, df *DirectoryProof,
+// These above checks should be performed before calling this method.
+func (cc *ConsistencyChecks) verifyReturnedPromise(df *DirectoryProof,
 	uname string, key []byte) error {


The uname parameter seems to be unused, and the ap.LookupIndex will have already been validated when verifyReturnedPromise() is called, right? We can drop this param.

masomel · 2016-11-14T18:25:48Z

protocol/consistencychecks.go

 // The client should create a new ConsistencyChecks instance only once,
 // when she registers her binding with a ConiksDirectory.
 // This ConsistencyChecks instance then will be used to verify
 // the returned responses from the ConiksDirectory.
 type ConsistencyChecks struct {
+	oldSTR   *m.SignedTreeRoot // keep a copy of SavedSTR before it gets updated


Is the idea for oldSTR to be a workaround for keeping track when the client has transitioned between epochs? It only seems to be relevant in verifyFulfilledPromise(), which we still expect to change when we address #116, right?

Yeah, right. Hope it can be used when we address #116.
EDIT: Reverted it in 09784a6.

masomel · 2016-11-14T18:34:10Z

protocol/consistencychecks.go

-		if !tb.Verify(ap) {
+	if tb, ok := cc.TBs[uname]; ok {
+		if str.Epoch != cc.oldSTR.Epoch+1 {
+			return ErrorBadPromise


Doesn't this case being true indicate that we didn't verify a TB during the next epoch (e.g. because we didn't do a lookup for this uname)? That doesn't make it a bad (i.e. invalid) TB per se, so I'm not sure if we should be returning an error here, or if we should just delete the old TB for now (until we have #116 implemented).

Sorry, I thought that we could use KeyLookup for monitoring until we support it. I should revert this commit and add a FIXME here as @arlolra did.

I thought that we could use KeyLookup for monitoring until we support it.

Oh, I see. I think this would actually be fine.~~,and just add a FIXME. Let me look at the new changes, though.~~

- Re-arrange methods' order. - Remove redundant params. - Remove comments which are no longer true. - Add more tests

vqhuy · 2016-11-15T01:00:02Z

Most of my comments are more about code cleanup (e.g. redundant params).

I re-organized code in ea46aaa. Thanks for your advice about that!

arlolra · 2016-11-15T02:06:43Z

protocol/consistencychecks.go

+
+	proofType := ap.ProofType()
+	switch {
+	case msg.Error == ErrorNameExisted && proofType == m.ProofOfInclusion && tb == nil:


Any reason you preferred this as tb == nil instead of the useTBs in 4a345e8?

I think tb==nil is more appropriate in this case, since we expect a nill tb regardless of the value of cc.useTBs. In other hand, we probably need to use a TB extension (this way or other ways), so cc.useTBs makes not much sense to me. Or maybe that's the thinking I have when working on #89.

we expect a nil tb regardless of the value of cc.useTBs.

Yes, that's true, but we're trying to eliminated the use of TBs here and only reference them in updateTB. I think it's ok to omit it and just have msg.Error == ErrorNameExisted && proofType == m.ProofOfInclusion.

However, I was more concerned with msg.Error == ErrorNameExisted && proofType == m.ProofOfAbsence, which is only valid if useTB is true.

Yes, you're totally right. I was mixed between 2 tasks.

Fixed in d7581cc.

arlolra · 2016-11-15T03:14:52Z

protocol/consistencychecks.go

+	case msg.Error == ErrorNameNotFound && proofType == m.ProofOfAbsence:
+	// FIXME: This would be changed when we support key changes
+	case msg.Error == Success && proofType == m.ProofOfInclusion && tb == nil:
+	case msg.Error == Success && proofType == m.ProofOfAbsence:


Same w/ this one msg.Error == Success && proofType == m.ProofOfAbsence && cc.useTBs.

arlolra · 2016-11-15T03:43:39Z

merkletree/proof.go

+type ProofType int
+
+const (
+	undeterminedProof ProofType = iota


Is undeterminedProof still relevant? Doesn't seem like it's being used.

It is being used in https://github.com/coniks-sys/coniks-go/pull/74/files/d7581cc23856723abcb0162f00cdec9d8b302905#diff-7e6b87a14023c0ebd22fb0b547fb3832R102 to avoid repeating bytes.Equal() calls.

Ok, sorry, I missed that.

arlolra · 2016-11-15T04:01:46Z

protocol/consistencychecks.go

+	}
+
+	// And update the saved STR
+	cc.SavedSTR = str


If we had any other request types, this would overwrite with nil, right.

Can we all a default case that panics?

We already panic in HandleResponse, is that enough?

I'd prefer to have it in both places, like checkConsistency.

arlolra · 2016-11-15T04:04:34Z

protocol/consistencychecks.go

+		case msg.Error == Success && proofType == m.ProofOfInclusion:
+			if err := cc.verifyFulfilledPromise(uname, str, ap); err != nil {
+				return err
+			}


Can we move the delete(cc.TBs, uname) to here?

Yes, it makes sense.

arlolra · 2016-11-15T04:09:20Z

After those two last things, please squash and merge this.

vqhuy · 2016-11-15T04:15:46Z

After those two last things, please squash and merge this.

Thanks!
@masomel @liamsi : Any thoughts before this is merged?

arlolra · 2016-11-15T17:44:19Z

Merged in d92753d.

masomel · 2016-11-15T18:46:08Z

Yay!!

vqhuy · 2016-11-18T14:52:01Z

Sorry, it seems like @arlolra missed 16949b2 in d92753d. Pushed directly to master in 2e86b64.

arlolra · 2016-11-18T17:01:30Z

Crap, sorry everyone :(

vqhuy force-pushed the cgo-client branch 2 times, most recently from 7ed7bbe to f91dc4c Compare August 20, 2016 10:42

vqhuy changed the base branch from master to lookup August 20, 2016 11:58

vqhuy force-pushed the cgo-client branch from 9b3b486 to 3cdef9f Compare August 20, 2016 17:53

vqhuy changed the title ~~[WIP] A stab at idea of cgoDoClientBusiness~~ A stab at idea of cgoDoClientBusiness Aug 20, 2016

vqhuy force-pushed the lookup branch 2 times, most recently from 60f803d to 498f6e8 Compare August 22, 2016 06:19

vqhuy force-pushed the cgo-client branch from 71761d5 to 9f49b27 Compare August 22, 2016 06:23

vqhuy force-pushed the lookup branch from 498f6e8 to 6756339 Compare August 22, 2016 06:24

vqhuy force-pushed the cgo-client branch 5 times, most recently from 23b04d4 to 4e9f659 Compare August 22, 2016 15:23

vqhuy force-pushed the lookup branch from 6756339 to a70b35c Compare August 22, 2016 22:53

arlolra closed this Aug 23, 2016

arlolra reopened this Aug 23, 2016

vqhuy changed the base branch from lookup to master August 23, 2016 02:56

vqhuy force-pushed the cgo-client branch 5 times, most recently from 7fec73a to a7c6680 Compare August 23, 2016 13:59

vqhuy changed the base branch from master to registration-bots August 23, 2016 15:01

masomel reviewed Aug 23, 2016
View reviewed changes

vqhuy force-pushed the cgo-client branch from 4e31b62 to 9f7871a Compare August 27, 2016 05:23

arlolra force-pushed the registration-bots branch from c2e9d08 to 3a20ee8 Compare August 27, 2016 14:58

vqhuy force-pushed the registration-bots branch from 3a20ee8 to c89c5d5 Compare August 27, 2016 15:50

vqhuy added 2 commits November 14, 2016 17:22

Fix fulfilled TB verification.

25a356b

Q: Which epoch did this lookup happen in? A: verifyFulfilledPromise must take a STR as its argument. This STR should be verified in updateSTR (e.g., epoch transitioning etc ...).

Add comment for HandleResponse

fec6cff

masomel requested changes Nov 14, 2016

View reviewed changes

vqhuy added 2 commits November 15, 2016 07:55

Re-organize code

ea46aaa

- Re-arrange methods' order. - Remove redundant params. - Remove comments which are no longer true. - Add more tests

Revert 25a356b

09784a6

vqhuy added 2 commits November 15, 2016 08:11

tb.Verify() instead (credit to @liamsi)

d2ed2c9

Revert 8e97d88

33e5f74

arlolra reviewed Nov 15, 2016

View reviewed changes

Fix 31c5b35

d7581cc

arlolra reviewed Nov 15, 2016

View reviewed changes

Refactor

16949b2

arlolra approved these changes Nov 15, 2016

View reviewed changes

masomel approved these changes Nov 15, 2016

View reviewed changes

arlolra closed this Nov 15, 2016

arlolra deleted the cgo-client branch November 15, 2016 17:44

vqhuy mentioned this pull request Nov 24, 2016

Implement monitoring verification #134

Open

2 tasks

arlolra mentioned this pull request Dec 2, 2016

Monitoring: Fix inconsistency occurring when client misses an epoch in between #144

Open

vqhuy mentioned this pull request Mar 13, 2017

Recovering from unintended inconsistencies #171

Open

Add verification methods #74

Add verification methods #74

Conversation

vqhuy commented Aug 18, 2016 • edited

Choose a reason for hiding this comment

vqhuy commented Nov 14, 2016

masomel left a 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

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

vqhuy Nov 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Nov 15, 2016 • edited

Choose a reason for hiding this comment

vqhuy commented Nov 15, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra commented Nov 15, 2016

vqhuy commented Nov 15, 2016

arlolra commented Nov 15, 2016

masomel commented Nov 15, 2016

vqhuy commented Nov 18, 2016

arlolra commented Nov 18, 2016

vqhuy commented Aug 18, 2016 •

edited

vqhuy Nov 15, 2016 •

edited

masomel Nov 15, 2016 •

edited