Refactor common STR verification functionality #170

masomel · 2017-03-12T22:10:57Z

Since the client and auditor both need to perform some common consistency checks on the STRs, all this functionality should be refactored into a generic auditor module. This will also require the implementation of STR verification over a range of epochs.

See #155 (comment)

…to auditor-protocol

…rror

liamsi · 2017-04-06T15:32:55Z

We will merge #155 first

…rror

# The first commit's message is: Add TODO to move all generic STR auditing code to a separate module # The 2nd commit message will be skipped: # Use single generic verifySTRConsistency to be used by client and auditor

…r response message generic

vqhuy · 2017-07-19T17:56:29Z

protocol/auditor.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
+


I think we should add a FIXME:

// FIXME: we are returning the error immediately // without saving the inconsistent STR // see: https://github.com/coniks-sys/coniks-go/pull/74#commitcomment-19804686

here or in AuditDirectory.

Why is this a FIXME? Why should we be saving inconsistent STRs?

This makes me think we should do:

Any STR that passes the checks in updateSTR will be stored and used by the client, regardless of whether the rest of the checks pass / fail, and that's fine since it contains proof of being issued.

I think we should have a TODO/FIXME here so we can come back later, when we address client recovery & whistle-blowing issues.

I generally agree with this, but I think auditors should save inconsistent STRs in a separate list. I also agree that this is a whistleblowing and recovery issue. We should open a new issue for this if we don't have one already.

We have #87 and #171. What I was saying is we add a TODO here so we don't forget it.

vqhuy · 2017-07-19T17:57:51Z

protocol/auditor.go

+// or the appropriate consistency check error if any of the checks fail,
+// or str's epoch is anything other than the same or one ahead of
+// a.verifiedSTR.
+func (a *AudState) checkSTRAgainstVerified(str *DirSTR) error {


Please reorder these functions as:

compareWithVerified() verifySTRConsistency() checkSTRAgainstVerified() verifySTRRange()

vqhuy · 2017-07-19T18:00:37Z

protocol/auditor.go

+// the given prevSTR and the first STR in the given range, and
+// then verifies the consistency between each subsequent STR pair.
+func (a *AudState) verifySTRRange(prevSTR *DirSTR, strs []*DirSTR) error {
+


Nitpicking: can we remove this empty line? Here and some other places.

vqhuy · 2017-07-19T18:02:01Z

protocol/auditor.go

+		if str == nil {
+			// FIXME: if this comes from the auditor, this
+			// should really be an ErrMalformedAuditorMessage
+			return ErrMalformedDirectoryMessage


I also encountered this problem, and I think we should just change this error and ErrMalformedClientMessage to ErrMalformedResquest and ErrMalformedResponse.

I can open a separate pull later, since I already have this.

Sounds good to me.

vqhuy · 2017-07-19T18:04:13Z

protocol/consistencychecks.go

@@ -45,18 +43,48 @@ func NewCC(savedSTR *DirSTR, useTBs bool, signKey sign.PublicKey) *ConsistencyCh
 	if !useTBs {
 		panic("[coniks] Currently the server is forced to use TBs")
 	}
+	aud := NewAuditor(signKey, savedSTR)
 	cc := &ConsistencyChecks{


It is easier for readers when we use field:value initializers.

I agree. This isn't currently consistent in all of the code, so we should fix this everywhere.

vqhuy · 2017-07-19T18:09:38Z

protocol/consistencychecks.go

+// the cc.verifiedSTR.
+// CheckEquivocation() is called when a client receives a response to a
+// message.AuditingRequest from an auditor.
+func (cc *ConsistencyChecks) CheckEquivocation(msg *Response) error {


IMHO, we should open a separate pull for this feature, since I think we should cover something like the number of chosen auditors, and design a set of APIs for the /protocol level.

The number of chosen auditors is independent of the core equivocation check protocol -- that's what this is. What other APIs do we need for equivocation checking on the client?

How about taking an array of auditors' Response and passing them to CheckEquivocation? I have no strong opinion about this, though.

I thought about doing this, although that I think that already imposes a requirement on the client to check multiple auditors. I think that detail should be implemented at the application layer to make the use of auditors more flexible in the client.

vqhuy · 2017-07-19T18:13:42Z

If it's possible, please remove the empty line after the function name.
Please update the documentation in the comments (some function names are uppercase, or not matched with the new name).

vqhuy · 2017-07-20T02:41:58Z

protocol/auditlog.go

-	signKey   sign.PublicKey
-	snapshots map[uint64]*DirSTR
-	latestSTR *DirSTR
+	snapshots []*DirSTR


Can we keep using the map as before? Is there any advantages over using a map?
I'm afraid that we may accidentally insert 2 same DirSTR into snapshots if we use slice.

This is a possibility, but the slice makes it much easier to append a whole batch of STRs. I wanted to avoid having to loop through a range twice, once for the verification, and one for the insertion into snapshots. But maybe this wouldn't be a huge performance issue?

Looking at current code, there are insertion and search operations, which are O(1), so I think it wouldn't be a huge performance issue.

Also addressing this as part of #182

…shots

vqhuy · 2017-08-11T10:33:07Z

protocol/consistencychecks.go

+	// TODO: if the auditor has returned a more recent STR,
+	// should the client update its savedSTR? Should this
+	// force a new round of monitoring?
+	return cc.checkSTRAgainstVerified(LatestSTRInRange(strs.STR))


Last-minute nitpicking: should we remove LatestSTRInRange and just do

latestSTRInRange := strs[len(strs)-1] return cc.checkSTRAgainstVerified(latestSTRInRange)

?
(or even pass strs[len(strs)-1] directly to checkSTRAgainstVerified)

LatestSTRInRange is only used here, right? I added this when I using a slice for the snapshots instead of a map, but it's no longer necessary now. I'll remove it.

vqhuy · 2017-08-11T11:12:47Z

protocol/auditor.go

+// range if the message contains more than one STR.
+// AuditDirectory() returns the appropriate consistency check error
+// if any of the checks fail, or nil if the checks pass.
+func (a *AudState) AuditDirectory(strs []*DirSTR) error {


@masomel @arlolra @liamsi Should we check null parameters in the function or assume that the caller will validate the parameters before calling?

Hmm, I generally assume that the caller hasn't validated the parameters. In the case of AuditDirectory, both ConsistencyChecks and AuditLog, both will call it inside of Audit, so in that case I think it would be safe to assume that the caller did a validation beforehand. OTOH, adding a parameter validation here is trivial, so I think we can add it here to be safe.

vqhuy · 2017-08-11T11:18:13Z

protocol/auditor.go

+
+// Auditor provides a generic interface allowing different
+// auditor types to implement specific auditing functionality.
+type Auditor interface {


Maybe we should add a TODO "this interface is used nowhere, we should use it everywhere in ConsistencyChecks and AuditLog instead of AudState." ?

I agree that ConsistencyChecks and AuditLog should "extend" Auditor rather than AudState, but to me this problem is more about how we can express what we mean.

Ideally, what I would have liked to do here is declare a generic auditor type/interface, as one would in Java for example, and have the AuditLog and ConsistencyChecks extend that class and implement something like an abstract AuditDirectory method. I think what we have now comes closest to this. Maybe the solution would be to rename AudState to Auditor and rename Auditor to something like AbstractAuditor? Please feel free to suggest an alternative approach if there's a more Go-like way of expressing what I had in mind.

Also, thinking ahead to implementing a CoSi-compatible auditor, AFAICT, it should be able to directly use AudState (or whatever we call it) and implement AuditDirectory() like we've done so far.

I agree, though I have no idea what it should be :(. Thus, I suggest we add a TODO here or open an issue so we can address it later.

Tbh, the way it's implemented right now was according to a suggestion you made to make AudState implement Auditor and have the other two use AudState so that other kinds of auditors could implement something different. I'm a bit confused why you're suggesting now that we should change this again ;)

I need to think about it some more, but I think that the only new type of auditor which would have to implement the Auditor interface would be a blockchain based auditor. What we have right now does express what we intended, so I don't think we need to change what we have. Maybe @arlolra and @liamsi can weigh in on this, too?

arlolra · 2017-08-22T02:54:09Z

protocol/auditor.go

+var _ Auditor = (*AudState)(nil)
+
+// NewAuditor instantiates a new auditor state from a persistance storage.
+func NewAuditor(signKey sign.PublicKey, verified *DirSTR) *AudState {


I would expect the return type here is Auditor

We discussed about this at #170 (comment). For now, AudState has some methods which shouldn't belong to the Auditor interface.

arlolra · 2017-08-22T03:07:49Z

protocol/auditlog.go

-func (h *directoryHistory) updateLatestSTR(newLatest *DirSTR) {
-	h.snapshots[newLatest.Epoch] = newLatest
-	h.latestSTR = newLatest
+// updateVerifiedSTR inserts a new range of STRs into a directory history;


range of STRs

?

arlolra · 2017-08-22T20:10:30Z

protocol/auditlog.go

-	h.updateLatestSTR(initSTR)
-	return h
+// Audit checks that a directory's STR history
+// is linear and updates the audtor's state


Thanks @c633 for taking care of this!

masomel · 2017-08-31T23:00:13Z

Am I right to assume that this is ready to be merged? No new comments in over a week.

masomel added 11 commits December 14, 2016 15:46

Add client-auditor messages

1a38aca

Add client-auditor messages

a5e78de

Create audit log structure, query API finished

da139a7

Merge branch 'auditor-protocol' of github.com:coniks-sys/coniks-go in…

61014bf

…to auditor-protocol

Add/Update docs to include auditor, add ReqUnknownDirectory auditor e…

3cd61f1

…rror

Use single generic verifySTRConsistency to be used by client and auditor

6577d92

Add tests for audit log, debug audit log

618d9c0

Add assertions to validate auditor messages on client

67188eb

Add generic STR response handler

2379d42

Add TODO to move all generic STR auditing code to a separate module

7b70110

[WIP] Refactor generic auditing functionality

b63eacf

masomel added the auditor label Mar 12, 2017

masomel added this to the 0.2.0 milestone Mar 12, 2017

masomel assigned masomel and vqhuy Mar 12, 2017

vqhuy mentioned this pull request Mar 13, 2017

Client-auditor protocol #155

Merged

3 tasks

masomel mentioned this pull request Jun 3, 2017

Implement monitoring verification #134

Open

2 tasks

vqhuy added a commit that referenced this pull request Jun 8, 2017

Revert #170

5d21f7c

masomel added 11 commits July 3, 2017 21:17

Add client-auditor messages

5a923db

Create audit log structure, query API finished

d9ed76d

Add/Update docs to include auditor, add ReqUnknownDirectory auditor e…

146e91a

…rror

Use single generic verifySTRConsistency to be used by client and auditor

90e2517

Add tests for audit log, debug audit log

e8178c1

Add assertions to validate auditor messages on client

badbafc

Add generic STR response handler

3bd8653

# This is a combination of 2 commits.

c2d107f

# The first commit's message is: Add TODO to move all generic STR auditing code to a separate module # The 2nd commit message will be skipped: # Use single generic verifySTRConsistency to be used by client and auditor

Fix documentation

13d3eb2

Use DirSTR instead of merkletree.SignedTreeRoot in auditlog

0d0d312

Remove all references to auditor-directory communication, make audito…

8b1eeeb

…r response message generic

vqhuy reviewed Jul 19, 2017

View reviewed changes

vqhuy reviewed Jul 20, 2017

View reviewed changes

masomel added 2 commits August 2, 2017 16:52

Fix documentation and formatting, revert to use map for auditlog snap…

2a494b5

…shots

Merge branch 'master' into generic-auditor

3acb2cc

vqhuy reviewed Aug 11, 2017

View reviewed changes

Minor fixes

8fbc629

Minor fix

e27b750

vqhuy approved these changes Aug 21, 2017

View reviewed changes

coniks-sys deleted a comment from vqhuy Aug 22, 2017

vqhuy mentioned this pull request Aug 22, 2017

/protocol organizational refactoring #183

Merged

arlolra reviewed Aug 23, 2017

View reviewed changes

Typo

c59b570

masomel merged commit 9332452 into master Sep 1, 2017

vqhuy deleted the generic-auditor branch September 3, 2017 20:11

vqhuy pushed a commit that referenced this pull request Sep 3, 2017

Refactor common STR verification functionality (#170)

f82d820

Refactor common STR verification functionality #170

Refactor common STR verification functionality #170

Conversation

masomel commented Mar 12, 2017 • edited Loading

liamsi commented Apr 6, 2017

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 Jul 19, 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

vqhuy Jul 19, 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

vqhuy commented Jul 19, 2017

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

masomel Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Aug 11, 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

masomel commented Aug 31, 2017

masomel commented Mar 12, 2017 •

edited

Loading

vqhuy Jul 19, 2017 •

edited

Loading

vqhuy Jul 19, 2017 •

edited

Loading

masomel Aug 11, 2017 •

edited

Loading

masomel Aug 11, 2017 •

edited

Loading