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

Client-auditor protocol #155

Merged
merged 26 commits into from
Jul 7, 2017
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
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
92c6a98
Use slice for initial history inserted, remove IsKnownDirectory
masomel Jul 7, 2017
073aa70
dirName --> addr, some documentation fixes
masomel Jul 7, 2017
e3cb343
Small fixes
masomel Jul 7, 2017
e886828
Revert "Change audit log index from byte array to string"
masomel Jul 7, 2017
3e340b9
More fixes for reverting
masomel Jul 7, 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
214 changes: 214 additions & 0 deletions protocol/auditlog.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
// This module implements a CONIKS audit log that a CONIKS auditor
// maintains.
// An audit log is a mirror of many CONIKS key directories' STR history,
// allowing CONIKS clients to audit the CONIKS directories.

package protocol

import (
"encoding/hex"

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

type directoryHistory struct {
dirName string
signKey sign.PublicKey
snapshots map[uint64]*DirSTR
latestSTR *DirSTR
}

// A ConiksAuditLog maintains the histories
// of all CONIKS directories known to a CONIKS auditor,
// indexing the histories by the hash of a directory's initial
// STR (specifically, the hash of the STR's signature as a string).
// Each history includes the directory's domain name as a string, its
// public signing key enabling the auditor to verify the corresponding
// signed tree roots, and a map with the snapshots for each observed
// epoch.
type ConiksAuditLog map[string]*directoryHistory

// 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 {
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 either remove this check or panic here.

Copy link
Member

Choose a reason for hiding this comment

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

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

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 refactoring LGTM.

return ""
}
return hex.EncodeToString(crypto.Digest(initSTR.Signature))
}

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

// caller validates that initSTR is for epoch 0
func newDirectoryHistory(dirName string, signKey sign.PublicKey, initSTR *DirSTR) *directoryHistory {
h := new(directoryHistory)
h.dirName = dirName
h.signKey = signKey
h.snapshots = make(map[uint64]*DirSTR)
h.updateLatestSTR(initSTR)
return h
}

// NewAuditLog constructs a new ConiksAuditLog. It creates an empty
// 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)
Copy link
Member

Choose a reason for hiding this comment

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

return make(map[string]*directoryHistory)

return l
}

// IsKnownDirectory checks to see if an entry for the directory
// (indexed by the hash of its initial STR dirInitHash) exists
// in the audit log l. IsKnownDirectory() does not
// validate the entries themselves. It returns true if an entry exists,
// and false otherwise.
func (l ConiksAuditLog) IsKnownDirectory(dirInitHash string) bool {
h := l[dirInitHash]
Copy link
Member

Choose a reason for hiding this comment

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

_, ok := l[dirInitHash]
return ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Still getting used to some Go idioms...

if h != nil {
return true
}
return false
}

// Insert creates a new directory history for the key directory addr,
// verifies the consistency of the STR history so far, and inserts it
// into the audit log l if the checks pass.
// The directory history is initialized with the key directory's
// signing key signKey, and a list of STRs representing the
// directory's STR history so far (as a map of epochs to STRs).
// Insert() returns an ErrAuditLog if the auditor attempts to create
// a new history for a known directory, an ErrMalformedDirectoryMessage
// if oldSTRs is malformed, a CheckBadSignature or CheckBadSTR if there
// is an inconsistency in the history given in hist, 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.
// 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, thanks for pointing this out!

func (l ConiksAuditLog) Insert(addr string, signKey sign.PublicKey,
hist map[uint64]*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.

hist is confusing here ... maybe snaps instead?


// make sure we're getting an initial STR at the very least
if len(hist) < 1 && hist[0].Epoch != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Can we change it to a 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.

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.

return ErrMalformedDirectoryMessage
}

// compute the hash of the initial STR
dirInitHash := computeInitSTRHash(hist[0])

// error if we want to create a new entry for a directory
// we already know
if l.IsKnownDirectory(dirInitHash) {
return ErrAuditLog
}

// create the new directory history
h := newDirectoryHistory(addr, signKey, hist[0])

// add each STR into the history
// start at 1 since we've inserted the initial STR above
startEp := uint64(1)
endEp := uint64(len(hist))

// This loop automatically catches if hist is malformed
// (i.e. hist is missing an epoch between 0 and the latest given)
for ep := startEp; ep < endEp; ep++ {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have a TODO: verify the prior STR history when it first joins here.

Copy link
Member Author

Choose a reason for hiding this comment

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

There sort of already is one on L107. But that FIXME is a bit buried at the bottom of the function.

Copy link
Member

Choose a reason for hiding this comment

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

My thought here is we will verify the consistency of the hash chain, so it maybe better if the index starts from startEp+1 (since the hist[startEp] is the initial STR), thus should we insert the initial STR into the map snapshot in newDirectoryHistory? It can be:

+func newDirectoryHistory(dirName string, signKey sign.PublicKey, str *DirSTR) *directoryHistory {
+	h := new(directoryHistory)
+	h.dirName = dirName
+	h.signKey = signKey
+	h.snapshots = make(map[uint64]*DirSTR)
+      h.snapshots[computeInitSTRHash(str)] = str
+	h.latestSTR = str
+	return 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 that the initial STR should be in snapshots since it's also pinned in the client, and the client verifies its directory's history with this pinned STR. So the auditor should mirror the entire history for each directory that it tracks. But I do like your idea that newDirectoryHistory should take the initial STR as a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, what I was suggesting is we should insert the initial STR at newDirectoryHistory and start verifying the str consistency with startEp+1.

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 got it. It makes total sense!

str := hist[ep]
if str == nil {
return ErrMalformedDirectoryMessage
}

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

Choose a reason for hiding this comment

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

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


if err != nil {
return err
}

h.snapshots[ep] = str
}

// Make sure to update the latestSTR
// in this particular call, the latestSTR has already been
// inserted into the snapshots map in the loop above
h.updateLatestSTR(hist[endEp-1])
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 it would be cleared if you moved this to the last step of the loop, so that rather than

h.snapshots[ep] = str

you have,

h.updateLatestSTR(str)

then you won't need the clarifying comment, and you won't be doing it redundantly when endEp == 1. It does mean a few unnecessary pointer updates, but I think it's worth it.

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 was considering making this change just today when I was working on #170. I totally agree with you that this step would be clearer if we just call updateLatestSTR() even if it means extra pointer updates.

l[dirInitHash] = h
Copy link
Member

Choose a reason for hiding this comment

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

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


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.
// FIXME: pass Response message as param
func (l ConiksAuditLog) Update(dirInitHash string, newSTR *DirSTR) error {

// error if we want to update the entry for an addr we don't know
if !l.IsKnownDirectory(dirInitHash) {
return ErrAuditLog
}

h := l[dirInitHash]
Copy link
Member

Choose a reason for hiding this comment

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

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


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

// update the latest STR
h.updateLatestSTR(newSTR)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return nil
}

// GetObservedSTRs gets a range of observed STRs for the CONIKS directory
// address indicated in the AuditingRequest req received from a
// CONIKS client, and returns a tuple of the form (response, error).
// The response (which also includes the error code) is sent back to
// the client. The returned error is used by the auditor
// for logging purposes.
//
// A request without a directory address, with a StartEpoch or EndEpoch
// greater than the latest observed epoch of this directory, or with
// at StartEpoch > EndEpoch is considered
// malformed and causes GetObservedSTRs() to return a
// message.NewErrorResponse(ErrMalformedClientMessage) tuple.
// GetObservedSTRs() returns a message.NewSTRHistoryRange(strs) tuple.
// strs is a list of STRs for the epoch range [StartEpoch, EndEpoch];
// if StartEpoch == EndEpoch, the list returned is of length 1.
// If the auditor doesn't have any history entries for the requested CONIKS
// directory, GetObservedSTRs() returns a
// 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
if !l.IsKnownDirectory(req.DirInitSTRHash) {
return NewErrorResponse(ReqUnknownDirectory), ReqUnknownDirectory
}

h := l[req.DirInitSTRHash]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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


// make sure the request is well-formed
if req.EndEpoch > h.latestSTR.Epoch || req.StartEpoch > req.EndEpoch {
return NewErrorResponse(ErrMalformedClientMessage),
ErrMalformedClientMessage
}

var strs []*DirSTR
for ep := req.StartEpoch; ep <= req.EndEpoch; ep++ {
str := h.snapshots[ep]
Copy link
Member

Choose a reason for hiding this comment

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

This is problematic if latestSTR isn't in the snapshots. See the discussion above.

strs = append(strs, str)
}

return NewSTRHistoryRange(strs)
}
Loading