Implement monitoring verification #134

vqhuy · 2016-11-24T13:52:49Z

Merge #155, #170, address #173 first.

Need to consider comments in #144 when reviewing.

TODO:

Refactor ConsistencyChecks constructors.
Support multiple users' verification.

vqhuy · 2016-12-02T18:52:29Z

@arlolra @masomel @liamsi : Whoever has time: I think this is ready for a first review.

liamsi

First pass: A few remarks/suggestions/questions.

liamsi · 2016-12-03T06:48:23Z

protocol/message.go

@@ -226,7 +226,9 @@ func (msg *Response) validate() error {
 		}
 		return nil
 	case *DirectoryProofs:
-		// TODO: also do above assertions here
+		if len(df.AP) < 1 || len(df.AP) != len(df.STR) {


Suggestion: maybe change len(df.AP) < 1 to len(df.AP) == 0?

liamsi · 2016-12-03T07:09:44Z

protocol/consistencychecks.go

+	switch {
+	case !ok && str0.Epoch == 0 && ap0.ProofType() == m.ProofOfAbsence: /* prior history verification */
+	case ok && str0.Epoch == regEp && ap0.ProofType() == m.ProofOfAbsence: /* registration epoch */
+	case ok && str0.Epoch >= regEp+1 && ap0.ProofType() == m.ProofOfInclusion: /* after registration */


Maybe define wasUnameAbsent := ap0.ProofType() == m.ProofOfAbsence before the switch and use this var in this 3 cases (with a ! for last case)? Makes it easier to read (at least I think so; might be wrong)

Yes, it also saves us from some condition checks. ;)

liamsi · 2016-12-03T07:19:55Z

protocol/consistencychecks.go

+
+	for i := 1; i < len(dfs.STR); i++ {
+		str := dfs.STR[i]
+		ap := dfs.AP[i]


Do we verify that len(dfs.STR) == len(dfs.AP) before this for-loop/always before calling verifyMonitoring?

Sure. I changed a bit in HandleResponse to explicitly validate each response type instead of calling msg.validate().

liamsi · 2016-12-03T07:23:33Z

protocol/consistencychecks.go

+				return err
+			}
+		default:
+			panic("[coniks] Passt.")


Passt? Sounds very german ;-) I'm not sure I get what this is intending to say.

masomel

LGTM for the most part. My main comment is that the generic auditor code should be removed since this really belongs in #170. I also have a few minor questions.

masomel · 2017-06-03T01:35:03Z

protocol/auditor.go

@@ -0,0 +1,55 @@
+// This module implements a generic CONIKS auditor, i.e. the
+// functionality that clients and auditors need to verify
+// a server's STR history.


I thought we were going to introduce the generic auditor in #170. I would keep this PR about monitoring only.

Yes, I intended to have #170 merged first, and this will be rebase on top of that.

I see. I thought we were going to merge this PR first. It's certainly closer to being done. This still duplicates code from #170, though, so in either case, I think the generic auditor code should be removed from this PR since it will be added as part of #170.

OK, seeing our timeline for Milestone 0.2.0, I agree with you.

masomel · 2017-06-03T01:55:55Z

protocol/consistencychecks.go

@@ -118,9 +118,33 @@ func (cc *ConsistencyChecks) updateSTR(requestType int, msg *Response) error {
 		}
 		// Otherwise, expect that we've entered a new epoch
 		if err := cc.verifySTRConsistency(cc.SavedSTR, str); err != nil {
+			// FIXME: we are returning the error immediately
+			// without saving the inconsistent STR
+			// see: https://github.com/coniks-sys/coniks-go/pull/74#commitcomment-19804686


After seeing that commit comment from #74, I'm wondering if we even need this STR verification here because the only STR check we really care about in this case is the verifySTR check.

You mean because the registration and key lookup don't fetch the new STR as we discussed in #173 ?

Right. At least for now, we can implement this assuming that the latest STR will have been fetched as part of monitoring.

masomel · 2017-06-03T02:00:42Z

protocol/consistencychecks.go

+				return err
+			}
+		default:
+			panic("[coniks] Unexpected monitoring epoch")


Do we want to panic here? This case seems more like the result of a malformed server response.

It's also possibly that the client sent an invalid epoch, but I agree that we should return an error here.

masomel · 2017-06-03T02:16:06Z

protocol/consistencychecks_test.go

+	// monitor binding was inserted
+	d.Update()
+
+	// do a lookup before the clock tells us to do monitoring


It's not entirely clear to me what this test is doing. It's checking whether the binding can be looked up after it's been inserted? I think there should also be a test that checks if the binding returned to a lookup == the binding returned for monitoring

the binding returned to a lookup == the binding returned for monitoring

This check is implicit in lookupAndVerify and monitorAndVerify with alice, key parameters. But this should be removed because of #173.

Makes sense!

masomel · 2017-06-03T21:30:14Z

protocol/consistencychecks.go


 		case msg.Error == ReqSuccess && proofType == m.ProofOfAbsence:
 			if err := cc.verifyReturnedPromise(df, key); err != nil {
 				return err
 			}
-			cc.savePromise(uname, df.TB)
+			cc.TBs[uname] = df.TB


Why isn't RegEpoch[uname] set here, too?

Nevermind, I understand now. I was a bit confused by the FIXME comment, since it seemed to me that we were caching the issued epoch via the RegEpoch map. But I think it's best to address #116 in a separate PR.

masomel · 2017-06-03T21:40:04Z

protocol/consistencychecks.go

 // verifyFulfilledPromise verifies issued TBs were inserted
 // in the directory as promised.
 func (cc *ConsistencyChecks) verifyFulfilledPromise(uname string, str *DirSTR,
 	ap *m.AuthenticationPath) error {
-	if regEp, ok := cc.RegEpoch[uname]; ok && str.Epoch == regEp+1 {


I'm assuming this check will be added back in when we address #116?

Keeping a RegEpoch map for other contacts seems not very nice for me, but yes, this should be fully addressed in a separate PR for #116.

* Resolve a TODO in verifyFulfilledPromise() * Part of #144

vqhuy self-assigned this Nov 24, 2016

vqhuy added this to the 0.2.0 milestone Nov 24, 2016

vqhuy added the protocol label Nov 24, 2016

vqhuy force-pushed the dfs-verify branch 2 times, most recently from 2d3bfee to 63f3df3 Compare November 25, 2016 12:43

vqhuy force-pushed the fix-reg branch 8 times, most recently from 6cd5fe7 to 5420200 Compare November 28, 2016 16:00

vqhuy closed this Nov 29, 2016

vqhuy changed the base branch from fix-reg to master November 29, 2016 06:57

vqhuy reopened this Nov 29, 2016

vqhuy mentioned this pull request Dec 2, 2016

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

Open

vqhuy force-pushed the dfs-verify branch 2 times, most recently from ebe41b8 to 4331068 Compare December 2, 2016 18:50

vqhuy force-pushed the dfs-verify branch 3 times, most recently from 5b4062f to e11c468 Compare December 2, 2016 19:00

liamsi reviewed Dec 3, 2016

View reviewed changes

vqhuy force-pushed the dfs-verify branch 3 times, most recently from fe3fdc2 to dcd2e3a Compare December 3, 2016 08:27

vqhuy changed the title ~~Implement monitoring verification~~ [WIP] Implement monitoring verification Dec 21, 2016

vqhuy force-pushed the dfs-verify branch from 0c5a61f to 1c07a42 Compare April 6, 2017 12:07

vqhuy requested a review from arlolra April 6, 2017 12:08

vqhuy requested a review from masomel April 6, 2017 12:08

vqhuy changed the title ~~[WIP] Implement monitoring verification~~ Implement monitoring verification Apr 6, 2017

masomel reviewed Jun 3, 2017

View reviewed changes

vqhuy force-pushed the dfs-verify branch 3 times, most recently from 3e4dcc5 to 7236859 Compare June 3, 2017 18:03

masomel reviewed Jun 3, 2017

View reviewed changes

masomel approved these changes Jun 9, 2017

View reviewed changes

vqhuy force-pushed the dfs-verify branch from 5d21f7c to d2b6650 Compare June 15, 2017 07:49

Implement monitoring verification

3db03d7

* Resolve a TODO in verifyFulfilledPromise() * Part of #144

vqhuy force-pushed the dfs-verify branch from d2b6650 to 3db03d7 Compare June 15, 2017 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement monitoring verification #134

Implement monitoring verification #134

vqhuy commented Nov 24, 2016 •

edited

Loading

vqhuy commented Dec 2, 2016 •

edited

Loading

liamsi left a comment

liamsi Dec 3, 2016

liamsi Dec 3, 2016

vqhuy Dec 3, 2016

liamsi Dec 3, 2016

vqhuy Dec 3, 2016

liamsi Dec 3, 2016 •

edited

Loading

masomel left a comment

masomel Jun 3, 2017

vqhuy Jun 3, 2017

masomel Jun 3, 2017

vqhuy Jun 4, 2017 •

edited

Loading

masomel Jun 3, 2017

vqhuy Jun 3, 2017

masomel Jun 3, 2017

masomel Jun 3, 2017

vqhuy Jun 3, 2017

masomel Jun 3, 2017

vqhuy Jun 3, 2017

masomel Jun 3, 2017

masomel Jun 3, 2017

masomel Jun 3, 2017

masomel Jun 3, 2017

vqhuy Jun 4, 2017

Implement monitoring verification #134

Are you sure you want to change the base?

Implement monitoring verification #134

Conversation

vqhuy commented Nov 24, 2016 • edited Loading

vqhuy commented Dec 2, 2016 • edited Loading

liamsi 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

liamsi Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

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

vqhuy Jun 4, 2017 • edited Loading

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 commented Nov 24, 2016 •

edited

Loading

vqhuy commented Dec 2, 2016 •

edited

Loading

liamsi Dec 3, 2016 •

edited

Loading

vqhuy Jun 4, 2017 •

edited

Loading