Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor common STR verification functionality #170

Merged
merged 47 commits into from
Sep 1, 2017
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
1a38aca
Add client-auditor messages
masomel Dec 14, 2016
a5e78de
Add client-auditor messages
masomel Dec 15, 2016
da139a7
Create audit log structure, query API finished
masomel Dec 21, 2016
61014bf
Merge branch 'auditor-protocol' of github.com:coniks-sys/coniks-go in…
masomel Feb 8, 2017
3cd61f1
Add/Update docs to include auditor, add ReqUnknownDirectory auditor e…
masomel Feb 16, 2017
6577d92
Use single generic verifySTRConsistency to be used by client and auditor
masomel Feb 16, 2017
618d9c0
Add tests for audit log, debug audit log
masomel Mar 8, 2017
67188eb
Add assertions to validate auditor messages on client
masomel Mar 8, 2017
2379d42
Add generic STR response handler
masomel Mar 8, 2017
7b70110
Add TODO to move all generic STR auditing code to a separate module
masomel Mar 10, 2017
b63eacf
[WIP] Refactor generic auditing functionality
masomel Mar 10, 2017
5a923db
Add client-auditor messages
masomel Dec 14, 2016
d9ed76d
Create audit log structure, query API finished
masomel Dec 21, 2016
146e91a
Add/Update docs to include auditor, add ReqUnknownDirectory auditor e…
masomel Feb 16, 2017
90e2517
Use single generic verifySTRConsistency to be used by client and auditor
masomel Feb 16, 2017
e8178c1
Add tests for audit log, debug audit log
masomel Mar 8, 2017
badbafc
Add assertions to validate auditor messages on client
masomel Mar 8, 2017
3bd8653
Add generic STR response handler
masomel Mar 8, 2017
c2d107f
# This is a combination of 2 commits.
masomel Mar 10, 2017
13d3eb2
Fix documentation
masomel Mar 12, 2017
0d0d312
Use DirSTR instead of merkletree.SignedTreeRoot in auditlog
masomel Mar 12, 2017
8b1eeeb
Remove all references to auditor-directory communication, make audito…
masomel Apr 4, 2017
3872e32
STRList -> STRHistoryRange
masomel Apr 5, 2017
1f609a8
Fail sooner in GetObservedSTRs
masomel Jun 1, 2017
477cddf
Revert changes to protocol/str.go
masomel Jun 3, 2017
c6d106c
Always request epoch range in AuditingRequest, fix Insert() bug
masomel Jun 26, 2017
9843f4a
Index audit log by hash of directory's initial STR
masomel Jul 3, 2017
9f212b1
Insert latest STR into snapshot map right away
masomel Jul 3, 2017
fcebf89
Fix go vet error
masomel Jul 4, 2017
7e54087
Change audit log index from byte array to string
masomel Jul 4, 2017
699858a
Add test case for getting an STR after an audit log update
masomel Jul 4, 2017
92cee27
Refactor common functions
vqhuy Jul 5, 2017
a4e0731
Merge branch 'auditor-protocol' into generic-auditor
masomel Jul 5, 2017
c3a79e0
Create generic auditor interface
masomel Jul 5, 2017
69e8454
Fix go fmt
masomel Jul 5, 2017
781df29
Merge branch 'master' into generic-auditor
masomel Jul 13, 2017
1e33f46
Fix some merging issues
masomel Jul 13, 2017
187c866
Refactor generic auditing functionality
vqhuy Jun 15, 2017
0f71b06
Renaming STR verification functions, use generic auditor functionalit…
masomel Jul 13, 2017
d9a9548
h.verifiedSTR -> h.VerifiedSTR()
masomel Jul 18, 2017
88d021f
Refactor common STR verification for STR ranges from server
masomel Jul 19, 2017
4865670
Fix
masomel Jul 19, 2017
2a494b5
Fix documentation and formatting, revert to use map for auditlog snap…
masomel Aug 2, 2017
3acb2cc
Merge branch 'master' into generic-auditor
masomel Aug 2, 2017
8fbc629
Minor fixes
masomel Aug 18, 2017
e27b750
Minor fix
vqhuy Aug 19, 2017
c59b570
Typo
vqhuy Aug 25, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 52 additions & 31 deletions protocol/auditlog.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,21 @@ import (
)

type directoryHistory struct {
*AudState
addr string
signKey sign.PublicKey
snapshots map[uint64]*DirSTR
latestSTR *DirSTR
snapshots []*DirSTR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also addressing this as part of #182

}

// caller validates that initSTR is for epoch 0
func newDirectoryHistory(addr string, signKey sign.PublicKey, initSTR *DirSTR) *directoryHistory {
a := NewAuditor(signKey, initSTR)
h := &directoryHistory{
a,
addr,
make([]*DirSTR, 0),
}
h.updateVerifiedSTR([]*DirSTR{initSTR})
return h
}

// A ConiksAuditLog maintains the histories
Expand All @@ -27,21 +38,25 @@ type directoryHistory struct {
// chronological order.
type ConiksAuditLog map[[crypto.HashSizeByte]byte]*directoryHistory

// updateLatestSTR inserts a new STR into a directory history;
// assumes the STR has been validated by the caller
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

range of STRs

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

// assumes the STRs have been validated by the caller
func (h *directoryHistory) updateVerifiedSTR(newVerified []*DirSTR) {
h.Update(LatestSTRInRange(newVerified))
h.snapshots = append(h.snapshots, newVerified...)
}

// caller validates that initSTR is for epoch 0
func newDirectoryHistory(addr string, signKey sign.PublicKey, initSTR *DirSTR) *directoryHistory {
h := new(directoryHistory)
h.addr = addr
h.signKey = signKey
h.snapshots = make(map[uint64]*DirSTR)
h.updateLatestSTR(initSTR)
return h
// Audit checks that a directory's STR history
// is linear and updates the audtor's state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auditor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @c633 for taking care of this!

// if the checks pass.
// Audit() first checks the oldest STR in the
// STR range received in message against the h.verfiedSTR,
// and then verifies the remaining STRs in msg, and
// finally updates the snapshots if the checks pass.
// Audit() is called when an auditor receives new STRs
// from a directory.
func (h *directoryHistory) Audit(msg *Response) error {
// TODO: Implement as part of the auditor-server protocol
return CheckPassed
}

// NewAuditLog constructs a new ConiksAuditLog. It creates an empty
Expand Down Expand Up @@ -72,10 +87,11 @@ func (l ConiksAuditLog) get(dirInitHash [crypto.HashSizeByte]byte) (*directoryHi
// signing key signKey, and a list of snapshots snaps representing the
// directory's STR history so far, in chronological order.
// Insert() returns an ErrAuditLog if the auditor attempts to create
// a new history for a known directory, an ErrMalformedDirectoryMessage
// if oldSTRs is malformed, and nil otherwise.
// a new history for a known directory, and nil otherwise.
// Insert() only creates the initial entry in the log for addr. Use Update()
// to insert newly observed STRs for addr in subsequent epochs.
// Insert() assumes that the caller
// has called Audit() on snaps before calling Insert().
// 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
Expand All @@ -100,6 +116,7 @@ func (l ConiksAuditLog) Insert(addr string, signKey sign.PublicKey,
// create the new directory history
h = newDirectoryHistory(addr, signKey, snaps[0])
Copy link
Member

@vqhuy vqhuy Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please verify snaps[0] against the pinnedSTR before inserting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Audit() is going to be called before Insert(), so snaps[0] will have been verified at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment: #170 (comment). I think we still need to verify snaps[0] when creating a new directory history. We could include this verification in newDirectoryHistory. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment in #170 (comment), I don't think that it's newDirectoryHistory's job to verify snaps[0], especially if it's being called in the context of Audit().


// FIXME: remove this check --> caller calls Audit() before this function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we will pass Response message as param, should we assume that the callers will call Audit() before this function? Or is it better to embed the Audit() call in this function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think it makes more sense to separate the logic, so we should keep the function signature as-is and only pass the Response to the Audit() function.

Copy link
Member

@vqhuy vqhuy Aug 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the function signature as-is, the caller has to de-serialize the Response twice (in Audit and before calling Insert), then wouldn't it make things more complicated? I mean the implementor has to do

Audit(response)
deSerialize(response)
Insert(...)

You also said that Audit will "finally update the snapshots if the checks pass", which means you cannot call Audit before this function (actually we cannot call Audit before this function since we call newDirectoryHistory inside this function).
I think this correct logic is

Insert() {
  h := newDirectoryHistory()
  h.Audit()
  h.updateVerifiedSTR() //optional if we don't do this in Audit
  log.set(h)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are multiple ways to approach this. Insert shouldn't do anything but do an actual insertion into the audit log. I don't understand why newDirectoryHistory must be called before Audit, those are independent operations. The current logic is: "Did the audit pass? Yes, so let's insert the snapshots into our log/update our log". This totally fits with my previous statement, what it's saying is that insertion/updates to the log should happen after auditing checks pass. This is the logic that's currently implemented.

Now, I do agree that passing the response message to Insert would require the implementor to de-serialize twice, and that's not elegant. My intention was to keep the function signatures consistent with the ones in directory.go, where Insert also takes the Response. but I never felt like this really made sense in the auditor's case.

So, given my previous statement about how Audit() should update the snapshots if the checks pass, Audit() should really be calling Insert() and Update, not the other way around. In fact, at the application level, it would really make more sense to have the auditor simply call Audit() on a new server response and have the insertion/update be abstracted away from the application layer. Then, we can unexport Insert and Update, and pass the snapshots directly instead of the Response. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what I'm suggesting is

Audit(Reponse r) {
  pass = AuditDirectory()
  if pass {
    if new directory
      insert(snaps)
    else
      update(snaps)
  }
}

I haven't implemented this yet, because it's part of the server-auditor protocol.

Copy link
Member

@vqhuy vqhuy Aug 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why newDirectoryHistory must be called before Audit, those are independent operations.

It's "Go" things, you are declaring Audit as a method of directoryHistory, you cannot call Audit without instantiating a variable of directoryHistory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways, your suggestion makes a lot of senses. Sorry for wasting your time 😕

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's "Go" things, you are declaring Audit as a method of directoryHistory, you cannot call Audit without instantiating a variable of directoryHistory.

Oh gotcha! Good point, sorry for being slow :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing this as part of #182

// add each STR into the history
// start at 1 since we've inserted the initial STR above
// This loop automatically catches if snaps is malformed
Expand All @@ -112,26 +129,28 @@ func (l ConiksAuditLog) Insert(addr string, signKey sign.PublicKey,

// verify the consistency of each new STR before inserting
// into the audit log
if err := verifySTRConsistency(signKey, h.latestSTR, str); err != nil {
if err := h.verifySTRConsistency(snaps[i-1], str); err != nil {
return err
}

h.updateLatestSTR(snaps[i])
}

// Finally, add the new history to the log
if len(snaps) > 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to coverage this path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this check into updateVerifiedSTR, and just call h.updateVerifiedSTR(snaps)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check only applies to Inserts because snaps[0] is inserted into h.snapshots in newDirectoryHistory and the rest of the range is added separately, while in all other cases (i.e. Update) we will always add the entire range at once. So moving this check into updateVerifiedSTR would have to distinguish between anInsert and an Update, which I think makes things messy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I got it.

snaps[0] is inserted into h.snapshots in newDirectoryHistory and the rest of the range is added separately

If we switch to use a map of snapshots as before, this could be avoided.

h.updateVerifiedSTR(snaps[1:])
}
l.set(dirInitHash, h)

return nil
}

// Update verifies the consistency of a newly observed STR newSTR for
// the directory addr, and inserts the newSTR into addr's directory history
// if the checks (i.e. STR signature and hash chain verifications) pass.
// Update() returns nil if the checks pass, and the appropriate consistency
// check error otherwise. Update() assumes that Insert() has been called for
// addr prior to its first call and thereby expects that an entry for addr
// exists in the audit log l.
// Update inserts a newly observed STR newSTR into the log entry for the
// directory history given by dirInitHash (hash of direcotry's initial STR).
// Update() assumes that Insert() has been called for
// dirInitHash prior to its first call and thereby expects that an
// entry for addr exists in the audit log l, and that the caller
// has called Audit() on newSTR before calling Update().
// Update() returns ErrAuditLog if the audit log doesn't contain an
// entry for dirInitHash
// FIXME: pass Response message as param
func (l ConiksAuditLog) Update(dirInitHash [crypto.HashSizeByte]byte, newSTR *DirSTR) error {

Expand All @@ -141,12 +160,14 @@ func (l ConiksAuditLog) Update(dirInitHash [crypto.HashSizeByte]byte, newSTR *Di
return ErrAuditLog
}

if err := verifySTRConsistency(h.signKey, h.latestSTR, newSTR); err != nil {
// FIXME: remove this check --> caller calls Audit() before this function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

if err := h.verifySTRConsistency(h.VerifiedSTR(), newSTR); err != nil {
return err
}

// update the latest STR
h.updateLatestSTR(newSTR)
// FIXME: use STR slice from Response msg
h.updateVerifiedSTR([]*DirSTR{newSTR})
return nil
}

Expand Down Expand Up @@ -178,7 +199,7 @@ func (l ConiksAuditLog) GetObservedSTRs(req *AuditingRequest) (*Response,
}

// make sure the request is well-formed
if req.EndEpoch > h.latestSTR.Epoch || req.StartEpoch > req.EndEpoch {
if req.EndEpoch > h.VerifiedSTR().Epoch || req.StartEpoch > req.EndEpoch {
return NewErrorResponse(ErrMalformedClientMessage),
ErrMalformedClientMessage
}
Expand Down
174 changes: 174 additions & 0 deletions protocol/auditor.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
// This module implements a generic CONIKS auditor, i.e. the
// functionality that clients and auditors need to verify
// a server's STR history.

package protocol

import (
"fmt"
"reflect"

"github.com/coniks-sys/coniks-go/crypto"
"github.com/coniks-sys/coniks-go/crypto/sign"
)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@masomel masomel Aug 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@masomel masomel Aug 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

AuditDirectory([]*DirSTR) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I really like this name.

}

// AudState verifies the hash chain of a specific directory.
type AudState struct {
signKey sign.PublicKey
verifiedSTR *DirSTR
}

var _ Auditor = (*AudState)(nil)

// New instantiates a new auditor state from a persistance storage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/New/NewAuditor

func NewAuditor(signKey sign.PublicKey, verified *DirSTR) *AudState {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the return type here is Auditor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

a := &AudState{
signKey: signKey,
verifiedSTR: verified,
}
return a
}

// VerifiedSTR returns the newly verified STR.
func (a *AudState) VerifiedSTR() *DirSTR {
return a.verifiedSTR
}

// Update updates the auditor's verifiedSTR to newSTR
func (a *AudState) Update(newSTR *DirSTR) {
a.verifiedSTR = newSTR
}

// CompareWithVerified checks whether the received STR is the same as
// the verified STR in the AudState using reflect.DeepEqual().
func (a *AudState) compareWithVerified(str *DirSTR) error {
if reflect.DeepEqual(a.verifiedSTR, str) {
return nil
}
return CheckBadSTR
}

// CheckSTRAgainstVerified checks an STR str against the a.verifiedSTR.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/CheckSTRAgainstVerified/checkSTRAgainstVerified

// If str's Epoch is the same as the verified, CheckSTRAgainstVerified()
// compares the two STRs directly. If str is one epoch ahead of the
// a.verifiedSTR, CheckSTRAgainstVerified() checks the consistency between
// the two STRs.
// CheckSTRAgainstVerified() returns nil if the check passes,
// 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 {
Copy link
Member

@vqhuy vqhuy Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reorder these functions as:

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

// 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

switch {
case str.Epoch == a.verifiedSTR.Epoch:
// Checking an STR in the same epoch
if err := a.compareWithVerified(str); err != nil {
return err
}
case str.Epoch == a.verifiedSTR.Epoch+1:
// Otherwise, expect that we've entered a new epoch
if err := a.verifySTRConsistency(a.verifiedSTR, str); err != nil {
return err
}
default:
return CheckBadSTR
}

return nil
}

// VerifySTRConsistency checks the consistency between 2 snapshots.
// 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 its consistency state,
// or an auditor's pinned signing key in its history.
func (a *AudState) verifySTRConsistency(prevSTR, str *DirSTR) error {
// verify STR's signature
if !a.signKey.Verify(str.Serialize(), str.Signature) {
return CheckBadSignature
}
if str.VerifyHashChain(prevSTR) {
return nil
}

// TODO: verify the directory's policies as well. See #115
return CheckBadSTR
}

// VerifySTRHistory checks the consistency of a range
// of a directory's STRs. It begins by verifying the STR consistency between
// 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 {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

prev := prevSTR
for i := 0; i < len(strs); i++ {
str := strs[i]
if str == nil {
// FIXME: if this comes from the auditor, this
// should really be an ErrMalformedAuditorMessage
return ErrMalformedDirectoryMessage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

}

// verify the consistency of each STR in the range
if err := a.verifySTRConsistency(prev, str); err != nil {
return err
}

prev = str
}

return nil
}

// AuditDirectory validates a range of STRs received from a CONIKS directory.
// AuditDirectory() checks the consistency of the oldest STR in the range
// against the verifiedSTR, and verifies the remaining
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// check STR against the latest verified STR
if err := a.checkSTRAgainstVerified(strs[0]); err != nil {
return err
}

// verify the entire range if we have received more than one STR
if len(strs) > 1 {
if err := a.verifySTRRange(strs[0], strs[1:]); err != nil {
return err
}
}

return nil
}

// ComputeDirectoryIdentity returns the hash of
// the directory's initial STR as a byte array.
// It panics if the STR isn't an initial STR (i.e. str.Epoch != 0).
func ComputeDirectoryIdentity(str *DirSTR) [crypto.HashSizeByte]byte {
if str.Epoch != 0 {
panic(fmt.Sprintf("[coniks] Expect epoch 0, got %x", str.Epoch))
}

var initSTRHash [crypto.HashSizeByte]byte
copy(initSTRHash[:], crypto.Digest(str.Signature))
return initSTRHash
}

// LatestSTRInRange returns the STR in the last element of a
// range of STRs. The range is assumed to be sorted
// by increasing order of STR epoch.
func LatestSTRInRange(strs []*DirSTR) *DirSTR {
return strs[len(strs)-1]
}
File renamed without changes.
20 changes: 0 additions & 20 deletions protocol/common.go

This file was deleted.

Loading