Client-auditor protocol #155

masomel · 2016-12-15T19:16:59Z

Part of #151

TODO:

client-auditor requests and responses
auditor state
tests

vqhuy · 2017-02-17T03:53:00Z

My most concern here is how will this fit well with other auditor types (like Cosi and Blockchain). But I'm fine with this PR and happy to reorganize our code as things come later.

masomel · 2017-02-17T16:30:12Z

My most concern here is how will this fit well with other auditor types (like Cosi and Blockchain).

Right, all of these auditor types are very distinct, but I think CoSi auditors and blockchain auditors are more extensions to the auditing protocol instead of complete replacements.

But I'm fine with this PR and happy to reorganize our code as things come later.

I'm not sure what you have in mind. Is your issue that the auditor protocol is part of the core protocol? Because as I said above, I think it should be. If/when we have more auditing options, like you said, we can re-think whether this makes sense.

vqhuy · 2017-02-17T03:16:45Z

protocol/auditlog.go

+func newDirectoryHistory(signKey sign.PublicKey, str *m.SignedTreeRoot) *directoryHistory {
+	h := new(directoryHistory)
+	h.signKey = signKey
+	h.snapshots = make(map[uint64]*m.SignedTreeRoot)


Should str be included in snapshots as well?

Is there a big advantage to including the initially observed str in snapshots right away?

Hmm, no. I just thought it should be here at the first glance. But it doesn't matter :p

vqhuy · 2017-02-17T03:17:38Z

protocol/auditlog.go

+
+// IsKnownDirectory checks to see if an entry for the directory
+// address addr exists in the audit log l. IsKnownDirectory() does not
+//  validate the entries themselves. It returns true if an entry exists,


Nitpicking: double space here ;)

vqhuy · 2017-02-17T03:33:48Z

protocol/message.go

+		},
+	}, ReqSuccess
+}
+
 func (msg *Response) validate() error {


Should we update these assertions for ObservedSTR(s)?

Yes, I think this makes sense, especially if we're going to have some generic HandleSTRs function.

vqhuy · 2017-02-17T03:46:54Z

protocol/auditlog.go

+	}
+
+	if h := l.histories[req.DirectoryAddr]; h != nil {
+


I want to swap these 2 code blocks for consistency with directory.go: return error immediately if h==nil. The same for GetObservedSTR.

Can you please point to code in directory.go where this happens?

Here we go: https://github.com/coniks-sys/coniks-go/blob/master/protocol/directory.go#L201-L215. I meant we return NewErrorResponse earlier.

@masomel: Please fix this.

PTAL at 42da6b3

vqhuy · 2017-02-17T03:57:28Z

protocol/auditlog.go

+	return false
+}
+
+// FIXME: pass Request message as param


Do you mean Response? What do you think if we have a generic function like HandleResponse in consistencychecks.go and call Insert, Update, InsertRange for each case? I would like to call it HandleDirectorySTRs.

Ah, you're right, this should be Response. I also like the idea of having a generic HandleDirectorySTRs function since there is a lot of potentially shared STR verification code between the auditor and the client. But Insert, Update and InsertRange are specific to the auditor log, though, so I think it makes sense to keep those separate from consistencychecks.go.

But Insert, Update and InsertRange are specific to the auditor log, though, so I think it makes sense to keep those separate from consistencychecks.go.

Yes, of course.

vqhuy · 2017-03-02T04:00:29Z

Is your issue that the auditor protocol is part of the core protocol? Because as I said above, I think it should be.

Yes, I agree that too.

masomel · 2017-03-08T01:33:54Z

@c633 @liamsi @arlolra Can you please TAL at this?

vqhuy · 2017-03-08T03:07:41Z

protocol/consistencychecks.go

@@ -100,6 +100,50 @@ func (cc *ConsistencyChecks) HandleResponse(requestType int, msg *Response,
 	return CheckPassed
 }

+// handleDirectorySTRs is supposed to be used by CONIKS clients to
+// handle auditor responses, and by CONIKS auditors to handle directory responses.


Hmm, I'm not sure if this is supposed to be here since the file header states Implements all consistency check operations done by a CONIKS client on data received from a CONIKS directory.. Maybe we should move verifySTR and verifySTRConsistency to str.go? If you agree, we can add a TODO here.

I realized this as well. I've started working on creating a more generic auditor module that contains all the functionality clients and auditors need to verify/audit the STR consistency. What do you think? But I also think this belongs in a different PR since it requires for a verifySTRConsistencyInEpoch to be implemented. I'll add a TODO.

I know you were already working on this when implementing the monitoring protocol. Do you think it would make sense to implement all the STR-related verification separate from the monitoring?

Yeah, I agree! I'll separate these code and open PR for each of them.

I've started this in https://github.com/coniks-sys/coniks-go/tree/generic-auditor What do you think?

liamsi

A few first nitpicks/comments/questions while digging through the code... More comments on the general design later/tomorrow.
Also, there is a merge conflict in this pull (rebase onto master).

liamsi · 2017-03-12T15:25:25Z

protocol/auditlog.go

+
+// FIXME: pass Response message as param
+// masomel: will probably want to write a more generic function
+// for "catching up" on a history in case an auditor misses epochs


Before we merge this, the comments should follow golang's documentation convention (start with Insert). The FIXME can stay but shouldn't be the first thing one gets to read. The comment should probably briefly state what the difference/relation between Insert and Update is.

Ah yes, thanks for pointing this out!

liamsi · 2017-03-12T15:26:20Z

protocol/auditlog.go

+// FIXME: pass Response message as param
+func (l *ConiksAuditLog) Update(addr string, newSTR *m.SignedTreeRoot) error {
+
+	// panic if we want to update an entry for which we don't have


"... for which we don't know the directory" (?). Also, we don't really panic here, right?

No, we don't. I forgot to update the comment here.

liamsi · 2017-03-12T15:38:54Z

protocol/auditlog_test.go

+	res, err := aud.GetObservedSTR(&AuditingRequest{
+		DirectoryAddr: "test-server"})
+	obs := res.DirectoryResponse.(*ObservedSTR)
+	if err != ReqSuccess {


nitpicking: directly catch the error after it occurs (move this one line up)

Actually, I don't think that would work because I use res below for other operations. Or am I missing something?

Seems like this has been removed?

Yes, this has been updated.

liamsi · 2017-03-12T15:39:49Z

protocol/consistencychecks.go

@@ -2,6 +2,7 @@
 // on data received from a CONIKS directory.
 // These include data binding proof verification,
 // and non-equivocation checks.
+// TODO: move all STR-verifying functionality to a separate module


Could we open an issue for that so we won't forget about it? Or is this planned to happen in this PR?

@c633 and I are already working on this in a separate branch, but we haven't opened the PR yet.

liamsi · 2017-03-12T15:40:25Z

protocol/consistencychecks.go

@@ -100,6 +101,50 @@ func (cc *ConsistencyChecks) HandleResponse(requestType int, msg *Response,
 	return CheckPassed
 }

+// handleDirectorySTRs is supposed to be used by CONIKS clients to


Case doesn't match with method signature (handleDirectorySTRs vs HandleDirectorySTRs).

vqhuy · 2017-03-13T04:31:53Z

protocol/message.go

+		}
+		return nil
+	case *ObservedSTRs:
+		if df.STR == nil || len(df.STR) < 1 {


This can be simplified: if len(df.STR) == 0

Won't this throw a null pointer error if we don't assert df.STR != nil?

vqhuy · 2017-03-13T04:39:42Z

protocol/consistencychecks.go

+// The signKey param either comes from a client's
+// pinned signing key in cc, or an auditor's pinned signing key
+// in its history.
+// In the case of a client-side consistency check, verifySTRConsistency()


We can remove this line of comment since we're no longer used cc anymore.

vqhuy · 2017-03-13T04:40:35Z

protocol/consistencychecks.go

-func (cc *ConsistencyChecks) verifySTRConsistency(savedSTR, str *DirSTR) error {
+// It uses the signing key signKey to verify the STR's signature.
+// The signKey param either comes from a client's
+// pinned signing key in cc, or an auditor's pinned signing key


Should just be pinned signing key, or an ... (no more cc here).

vqhuy · 2017-03-13T05:53:55Z

protocol/consistencychecks.go

+// HandleDirectorySTRs is supposed to be used by CONIKS clients to
+// handle auditor responses, and by CONIKS auditors to handle directory
+// responses.
+func HandleDirectorySTRs(requestType int, msg *Response, signKey sign.PublicKey,


Since it is being used nowhere and we are working on this in #170, I would like to remove it from this PR.

vqhuy · 2017-03-13T07:11:22Z

protocol/message.go

@@ -90,6 +92,34 @@ type MonitoringRequest struct {
 	EndEpoch   uint64 `json:"end_epoch"`
 }

+// An AuditingRequest is a message with a CONIKS key directory's address
+// as a string that a CONIKS client sends to a CONIKS auditor, or a CONIKS auditor


Is this supposed to be used for an auditor to request to a directory? If so, the DirectoryAddr field seems to be redundant.

When I first wrote this, it was supposed to be used by both clients and auditors, but this doesn't really make sense. We should have separate message types for auditor-server communication and client-auditor communication. So you're right, having the DirectoryAddr field only makes sense for client-auditor communication.

vqhuy · 2017-03-13T07:11:41Z

protocol/message.go

+
+// An AuditingInEpochRequest is a message with a key directory's address
+// as a string and an epoch as a uint64 that a CONIKS client sends to
+// a CONIKS auditor, or a CONIKS auditor sends to a CONIKS directory,


Same as https://github.com/coniks-sys/coniks-go/pull/155/files#r105599786.

vqhuy · 2017-03-13T07:13:19Z

protocol/message.go

@@ -216,6 +262,40 @@ func NewMonitoringProof(ap []*m.AuthenticationPath,
 	}, ReqSuccess
 }

+// NewObservedSTR creates the response message a CONIKS auditor
+// sends to a client, or a CONIKS directory sends to an auditor,


If we use a pub-sub architecture, will this be used between an auditor and a directory?

I think in either case, pub-sub or request-response, the implementation of the corresponding auditor-directory message would be the same, so it would make sense to use the same function to return a list of requested STRs. Similarly, the response message type for a client-auditor request and an auditor-server request for STRs should be the same type. This being said, since this PR is only for client-auditor communication, I won't add relevant auditor-directory documentation yet.

liamsi · 2017-04-04T21:11:19Z

As long as `df` is not `nil` there would be no exception as `len` returns 0 if the argument is `nil` https://golang.org/pkg/builtin/#len Am 04.04.2017 11:05 nachm. schrieb "Marcela Melara" < [email protected]>:

…

***@***.**** commented on this pull request. ------------------------------ In protocol/message.go <#155 (comment)>: > @@ -229,6 +309,16 @@ func (msg *Response) validate() error { case *DirectoryProofs: // TODO: also do above assertions here return nil + case *ObservedSTR: + if df.STR == nil { + return ErrMalformedAuditorMessage + } + return nil + case *ObservedSTRs: + if df.STR == nil || len(df.STR) < 1 { Won't this throw a null pointer error if we don't assert df.STR != nil? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#155 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHGNZuK6qN3qpBuzeAFKIH11HG1wvTAAks5rsrCqgaJpZM4LOdfy> .

…r response message generic

vqhuy · 2017-07-04T05:30:12Z

protocol/auditlog.go

+// computeInitSTRHash is a wrapper for the digest function;
+// returns empty string if the STR isn't an initial STR (i.e. str.Epoch != 0)
+func computeInitSTRHash(initSTR *DirSTR) string {
+	if initSTR.Epoch != 0 {


I think we should either remove this check or panic here.

I refactored this function in 92cee27 (it would be used by the clients, too).
Feel free to revert if you don't like it.

This refactoring LGTM.

arlolra · 2017-07-05T20:52:10Z

protocol/auditlog.go

+// log; the auditor will add an entry for each CONIKS directory
+// the first time it observes an STR for that directory.
+func NewAuditLog() ConiksAuditLog {
+	l := make(map[string]*directoryHistory)


return make(map[string]*directoryHistory)

arlolra · 2017-07-05T20:54:13Z

protocol/auditlog.go

+)
+
+type directoryHistory struct {
+	name      string


What is name? I see below you're putting addr there, but where / why is this used?

This used to be dirName for directory domain name (i.e. its address), which is why we assign it addr below. My plan right now is to use this field when auditors query the directories they log every epoch.

arlolra · 2017-07-05T20:58:34Z

protocol/auditlog.go

+// validate the entries themselves. It returns true if an entry exists,
+// and false otherwise.
+func (l ConiksAuditLog) IsKnownDirectory(dirInitHash string) bool {
+	h := l[dirInitHash]


_, ok := l[dirInitHash] return ok

Still getting used to some Go idioms...

arlolra · 2017-07-05T21:04:42Z

protocol/auditlog.go

+// masomel: will probably want to write a more generic function
+// for "catching up" on a history in case an auditor misses epochs
+func (l ConiksAuditLog) Insert(addr string, signKey sign.PublicKey,
+	hist map[uint64]*DirSTR) error {


hist is confusing here ... maybe snaps instead?

arlolra · 2017-07-05T21:07:48Z

protocol/auditlog.go

+	hist map[uint64]*DirSTR) error {
+
+	// make sure we're getting an initial STR at the very least
+	if len(hist) < 1 && hist[0].Epoch != 0 {


hist is a map so it's possible that the length is greater than 1 but hist[0] returns nil. Since you're using it in several places below, cache it as initSTR, ok := hist[0] and then check ok here.

Good catch! Can we change it to a slice?

A slice would also make sense. Then we would have to check that the epoch for the STR in each element corresponds to the index, which seems fine to me.

arlolra · 2017-07-05T21:37:07Z

protocol/auditlog.go

+	// in this particular call, the latestSTR has already been
+	// inserted into the snapshots map in the loop above
+	h.updateLatestSTR(hist[endEp-1])
+	l[dirInitHash] = h


Maybe don't expose the internals of l and add a method, l.Add(dirInitHash, h) or something.

arlolra · 2017-07-05T21:38:19Z

protocol/auditlog.go

+		return ErrAuditLog
+	}
+
+	h := l[dirInitHash]


Same thing here, l.Get(dirInitHash) or similar.

arlolra · 2017-07-05T21:41:37Z

protocol/auditlog.go

+
+		// verify the consistency of each new STR before inserting
+		// into the audit log
+		err := verifySTRConsistency(signKey, h.snapshots[ep-1], str)


See the comment below, but this would then become (signKey, h.lastestSTR, str)

arlolra · 2017-07-05T21:43:44Z

protocol/auditlog.go

+	}
+
+	// update the latest STR
+	h.updateLatestSTR(newSTR)


With the refactor suggested above, these two operations (verifySTRConsistency, updateLatestSTR) can be combined into an h.internalUpdate(newSTR).

Because of what I'm doing for #170, I need to see if it makes sense to refactor these operations both on the client and auditor. I'll probably hold off on that for now.

arlolra · 2017-07-05T21:45:40Z

protocol/auditlog.go

+		return NewErrorResponse(ReqUnknownDirectory), ReqUnknownDirectory
+	}
+
+	h := l[req.DirInitSTRHash]


Or, maybe we should embrace that it's a map and always just use h, ok := and drop the IsKnownDirectory method.

We should, see https://github.com/coniks-sys/coniks-go/pull/155/files#r125792869

vqhuy · 2017-07-07T16:33:00Z

protocol/auditlog.go

+// dirInitHash from the ConiksAuditLog.
+// Get() also returns a boolean indicating whether the requested dirInitHash
+// is present in the log.
+func (l ConiksAuditLog) Get(dirInitHash string) (*directoryHistory, bool) {


Can we unexport this function? Same with Set.

vqhuy · 2017-07-07T16:42:08Z

protocol/auditlog.go

+	snaps []*DirSTR) error {
+
+	// make sure we're getting an initial STR at the very least
+	if len(snaps) < 1 && snaps[0].Epoch != 0 {


Should it be len(snaps) < 1 || snaps[0].Epoch != 0?

Oops yes, this should be an OR. Thank you for catching that.

vqhuy · 2017-07-07T16:46:54Z

protocol/auditlog.go

+	// (i.e. snaps is missing an epoch between 0 and the latest given)
+	for i := 1; i < len(snaps); i++ {
+		str := snaps[i]
+		if str == nil || str.Epoch != uint64(i) {


The verifySTRConsistency would make sure that str.Epoch == i, so we can remove the check here.

This check is only needed because snaps is a slice, and it's being checked in the loop. This property isn't inherent in the consistency check. Plus, verifySTRConsistency already does this check implicitly for one STR when it calls VerifyHashchain.

I do think that this property is inherent in the consistency check, since we are verify using the initial STR, and it would fail if str.Epoch != latestSTR.Epoch + 1, which means str.Epoch == i has to be true.

What I meant is that i == str.Epoch is only meaningful because we expect the snaps slice to have this property. i isn't meaningful to consistency checks in the general sense, so I don't see why we should impose this particular assumption on the consistency checks. And VerifyHashchain called in verifySTRConsistency already does the str.Epoch != latestSTR.Epoch + 1 check.

vqhuy · 2017-07-07T16:47:46Z

protocol/auditlog.go

+
+		// verify the consistency of each new STR before inserting
+		// into the audit log
+		err := verifySTRConsistency(signKey, h.latestSTR, str)


if err := verifySTRConsistency(signKey, h.latestSTR, str); err != nil { return err }

This reverts commit 7e54087. Conflicts: protocol/auditlog.go

masomel self-assigned this Dec 15, 2016

masomel added the protocol label Dec 15, 2016

masomel added this to the 0.2.0 milestone Dec 15, 2016

masomel added the auditor label Dec 15, 2016

masomel force-pushed the auditor-protocol branch 2 times, most recently from cb64fbc to da139a7 Compare December 21, 2016 22:39

masomel changed the title ~~[WIP] Auditing messages~~ Auditing messages Feb 16, 2017

masomel requested review from arlolra, vqhuy and liamsi February 16, 2017 20:23

masomel force-pushed the auditor-protocol branch from 8d48156 to 6577d92 Compare February 16, 2017 20:27

vqhuy reviewed Mar 2, 2017

View reviewed changes

vqhuy reviewed Mar 8, 2017

View reviewed changes

liamsi reviewed Mar 12, 2017

View reviewed changes

masomel force-pushed the auditor-protocol branch from 6b368f6 to dfd5de8 Compare March 12, 2017 22:06

masomel mentioned this pull request Mar 12, 2017

Refactor common STR verification functionality #170

Merged

vqhuy reviewed Mar 13, 2017

View reviewed changes

masomel changed the title ~~Auditing messages~~ Client-auditor protocol Mar 16, 2017

masomel added 9 commits July 3, 2017 21:17

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

STRList -> STRHistoryRange

3872e32

Fail sooner in GetObservedSTRs

1f609a8

Revert changes to protocol/str.go

477cddf

Always request epoch range in AuditingRequest, fix Insert() bug

c6d106c

Index audit log by hash of directory's initial STR

9843f4a

Insert latest STR into snapshot map right away

9f212b1

masomel force-pushed the auditor-protocol branch from 1db660b to 9f212b1 Compare July 4, 2017 01:18

masomel added 3 commits July 3, 2017 21:22

Fix go vet error

fcebf89

Change audit log index from byte array to string

7e54087

Add test case for getting an STR after an audit log update

699858a

vqhuy reviewed Jul 4, 2017

View reviewed changes

Refactor common functions

92cee27

vqhuy approved these changes Jul 5, 2017

View reviewed changes

arlolra reviewed Jul 5, 2017

View reviewed changes

masomel added 2 commits July 7, 2017 12:16

Use slice for initial history inserted, remove IsKnownDirectory

92c6a98

dirName --> addr, some documentation fixes

073aa70

vqhuy reviewed Jul 7, 2017

View reviewed changes

masomel added 3 commits July 7, 2017 13:00

Small fixes

e3cb343

Revert "Change audit log index from byte array to string"

e886828

This reverts commit 7e54087. Conflicts: protocol/auditlog.go

More fixes for reverting

3e340b9

vqhuy approved these changes Jul 7, 2017

View reviewed changes

masomel merged commit ae837f6 into master Jul 7, 2017

vqhuy deleted the auditor-protocol branch July 7, 2017 18:23

Client-auditor protocol #155

Client-auditor protocol #155

Conversation

masomel commented Dec 15, 2016 • edited Loading

vqhuy commented Feb 17, 2017

masomel commented Feb 17, 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

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 Mar 2, 2017

masomel commented Mar 8, 2017

Choose a reason for hiding this comment

masomel Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

masomel Jun 1, 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

vqhuy Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

masomel Mar 13, 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

liamsi commented Apr 4, 2017 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

masomel commented Dec 15, 2016 •

edited

Loading

masomel Mar 8, 2017 •

edited

Loading

masomel Jun 1, 2017 •

edited

Loading

vqhuy Mar 13, 2017 •

edited

Loading

masomel Mar 13, 2017 •

edited

Loading

vqhuy Jul 7, 2017 •

edited

Loading

masomel Jul 7, 2017 •

edited

Loading