Implement auditor-server protocol #182

masomel · 2017-08-18T14:17:16Z

Part of #151

TODO:

auditor-server request message
implement Audit() and change auditlog initialization
auditor-server tests
add auditor request handling in directory

vqhuy · 2017-08-21T14:54:55Z

protocol/auditlog.go

+
+	// Since the str[0] is pinned in the audit log
+	// expect that STR[0].Epoch is at least 1
+	if strs.STR[0].Epoch < 1 {


Wouldn't this be covered in AuditDirectory()? And I suppose an auditor should be able to request an STR range starting from epoch 0.

You're right, this case is covered in the str.Epoch == verifiedSTR.Epoch case in AuditDirectory(). What I wanted to avoid is re-inserting str[0], but if the checks pass, it wouldn't matter anyway.

vqhuy · 2017-08-21T14:57:33Z

protocol/auditor.go

@@ -105,7 +106,7 @@ func (a *AudState) checkSTRAgainstVerified(str *DirSTR) error {
 		return CheckBadSTR
 	}

-	return nil


I prefer returning nil here.

Do you mean only for this function because it's only a helper function, or in all the cases in which I replaced nil with CheckPassed? I feel like it would definitely make sense to return CheckPassed from AuditDirectory() at least.

I mean only for this function ;)

vqhuy · 2017-09-03T16:36:50Z

protocol/auditlog.go

 // Audit checks that a directory's STR history
-// is linear and updates the auditor's state
+// is linear and updates the audtor's state


vqhuy · 2017-09-03T16:37:22Z

protocol/auditlog.go

-	// TODO: Implement as part of the auditor-server protocol
-	return CheckPassed
+	if err := msg.validate(); err != nil {
+		return err.(ErrorCode)


It could be just return err.

vqhuy · 2017-09-03T16:40:27Z

protocol/auditlog.go

-func (l ConiksAuditLog) Insert(addr string, signKey sign.PublicKey,
+// InitHistory() is called by an auditor when it initializes its state
+// from disk (either first-time startup, or after reboot).
+func (l ConiksAuditLog) InitHistory(addr string, signKey sign.PublicKey,
 	snaps []*DirSTR) error {
 	// make sure we're getting an initial STR at the very least
 	if len(snaps) < 1 || snaps[0].Epoch != 0 {


I think we should compare snaps[0] with the pinned STR, no?

snaps[0] is the pinned STR. As the documentation describes, this function is called when the auditor first starts up, so the very first time that auditor is launched, it will read the pinned STR from disk and pass it into InitHistory through the snaps parameter.

Ahh sorry, I missed that part. Thanks!

No problem! I can make this clearer in the documentation.

vqhuy · 2017-09-03T16:43:05Z

protocol/consistencychecks.go

@@ -133,7 +133,7 @@ func (cc *ConsistencyChecks) updateSTR(requestType int, msg *Response) error {
 		// The initial STR is pinned in the client
 		// so cc.verifiedSTR should never be nil
 		// FIXME: use STR slice from Response msg
-		if err := cc.AuditDirectory([]*DirSTR{str}); err != nil {
+		if err := cc.AuditDirectory([]*DirSTR{str}); err != CheckPassed {


I will remove this CheckPassed later (nil is more idiomatic Go), so I think we can leave it as it was.

~~I think for CONIKS, though, CheckPassed is a clearer return value for the AuditDirectory check than nil.~~

It seems I misunderstood. If you're removing CheckPassed altogether, that seems fine to me.

vqhuy · 2017-09-03T16:53:01Z

protocol/directory.go

+// the epoch range [startEpoch, endEpoch], where startEpoch
+// and endEpoch are the epoch range endpoints indicated in the client's
+// request. If req.endEpoch is greater than d.LatestSTR().Epoch or
+// omitted in req, the end of the range will be set to d.LatestSTR().Epoch.


We haven't checked if req.endEpoch is omitted in "MonitoringRequest", so should we change the range request to something like [Epoch, Range] where Epoch is StartEpoch and Range is the maximum number of epochs to return?

vqhuy · 2017-09-03T16:53:57Z

protocol/message.go

@@ -135,7 +149,8 @@ type DirectoryProof struct {
 // STR representing a range of the STR hash chain. If the range only
 // covers the latest epoch, the list only contains a single STR.
 // A CONIKS auditor returns this DirectoryResponse type upon an
-// AudutingRequest.
+// AudutingRequest from a client, and a CONIKS directory returns


masomel mentioned this pull request Aug 18, 2017

Refactor common STR verification functionality #170

Merged

masomel requested a review from vqhuy August 21, 2017 01:17

masomel self-assigned this Aug 21, 2017

masomel changed the title ~~[WIP] Implement auditor-server protocol~~ Implement auditor-server protocol Aug 21, 2017

vqhuy changed the base branch from master to generic-auditor August 21, 2017 13:56

vqhuy reviewed Aug 21, 2017

View reviewed changes

vqhuy mentioned this pull request Aug 22, 2017

/protocol organizational refactoring #183

Merged

masomel changed the base branch from generic-auditor to master September 1, 2017 14:20

vqhuy reviewed Sep 3, 2017

View reviewed changes

vqhuy requested review from arlolra and liamsi September 3, 2017 17:11

vqhuy force-pushed the master branch from 9332452 to f82d820 Compare September 3, 2017 20:30

masomel added 4 commits September 3, 2017 23:11

Implement Audit(), add auditor-server message, update auditlog tests

957b2a1

Implement GetSTRHistory in directory, add corresponding tests

994b8be

Cover all AuditDirectory failure cases

65262bf

Allow auditor to request epoch 0 from directory

7f3f14f

vqhuy force-pushed the auditor-server branch from ac3a89e to d13b1ea Compare September 3, 2017 21:15

Typo

76ae044

vqhuy force-pushed the auditor-server branch from d13b1ea to 76ae044 Compare September 3, 2017 21:18

vqhuy approved these changes Sep 3, 2017

View reviewed changes

masomel added 2 commits September 19, 2017 12:02

Make EndEpoch mandatory in STRHistoryRequest message

51cdf9c

Clarify usage of InitHistory() in documentation

1927e77

masomel merged commit 153edc2 into master Sep 21, 2017

vqhuy deleted the auditor-server branch September 22, 2017 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement auditor-server protocol #182

Implement auditor-server protocol #182

masomel commented Aug 18, 2017 •

edited

Loading

vqhuy Aug 21, 2017

masomel Aug 21, 2017

vqhuy Aug 21, 2017 •

edited

Loading

masomel Aug 21, 2017

vqhuy Aug 21, 2017

vqhuy Sep 3, 2017

vqhuy Sep 3, 2017

vqhuy Sep 3, 2017 •

edited

Loading

masomel Sep 3, 2017

vqhuy Sep 3, 2017

masomel Sep 3, 2017

vqhuy Sep 3, 2017

masomel Sep 3, 2017 •

edited

Loading

vqhuy Sep 3, 2017

vqhuy Sep 3, 2017

Implement auditor-server protocol #182

Implement auditor-server protocol #182

Conversation

masomel commented Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Aug 21, 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 Sep 3, 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

masomel Sep 3, 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 commented Aug 18, 2017 •

edited

Loading

vqhuy Aug 21, 2017 •

edited

Loading

vqhuy Sep 3, 2017 •

edited

Loading

masomel Sep 3, 2017 •

edited

Loading