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 46 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
84 changes: 51 additions & 33 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
}

// caller validates that initSTR is for epoch 0
func newDirectoryHistory(addr string, signKey sign.PublicKey, initSTR *DirSTR) *directoryHistory {
a := NewAuditor(signKey, initSTR)
h := &directoryHistory{
AudState: a,
addr: addr,
snapshots: make(map[uint64]*DirSTR),
}
h.updateVerifiedSTR(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(newVerified)
h.snapshots[newVerified.Epoch] = 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,16 +87,16 @@ 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
func (l ConiksAuditLog) Insert(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 {
return ErrMalformedDirectoryMessage
Expand All @@ -100,6 +115,8 @@ 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
// 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,41 +129,43 @@ 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(h.VerifiedSTR(), str); err != nil {
return err
}

h.updateLatestSTR(snaps[i])
h.updateVerifiedSTR(snaps[i])
}

// Finally, add the new history to the log
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 {

// error if we want to update the entry for an addr we don't know
h, ok := l.get(dirInitHash)
if !ok {
return ErrAuditLog
}

if err := verifySTRConsistency(h.signKey, h.latestSTR, newSTR); err != nil {
// FIXME: remove this check --> caller calls Audit() before this
// function
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(newSTR)
return nil
}

Expand All @@ -170,15 +189,14 @@ func (l ConiksAuditLog) Update(dirInitHash [crypto.HashSizeByte]byte, newSTR *Di
// message.NewErrorResponse(ReqUnknownDirectory) tuple.
func (l ConiksAuditLog) GetObservedSTRs(req *AuditingRequest) (*Response,
ErrorCode) {

// make sure we have a history for the requested directory in the log
h, ok := l.get(req.DirInitSTRHash)
if !ok {
return NewErrorResponse(ReqUnknownDirectory), ReqUnknownDirectory
}

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

// NewAuditor instantiates a new auditor state from a persistance storage.
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
}

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

// checkSTRAgainstVerified checks an STR str against the a.verifiedSTR.
// 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.

// FIXME: we are returning the error immediately
// without saving the inconsistent STR
// see: https://github.com/coniks-sys/coniks-go/pull/74#commitcomment-19804686
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
}

// verifySTRRange 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 {
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.

// validate strs
if len(strs) == 0 {
return ErrMalformedDirectoryMessage
}

// 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
}
File renamed without changes.
20 changes: 0 additions & 20 deletions protocol/common.go

This file was deleted.

Loading