-
Notifications
You must be signed in to change notification settings - Fork 30
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
Changes from 42 commits
1a38aca
a5e78de
da139a7
61014bf
3cd61f1
6577d92
618d9c0
67188eb
2379d42
7b70110
b63eacf
5a923db
d9ed76d
146e91a
90e2517
e8178c1
badbafc
3bd8653
c2d107f
13d3eb2
0d0d312
8b1eeeb
3872e32
1f609a8
477cddf
c6d106c
9843f4a
9f212b1
fcebf89
7e54087
699858a
92cee27
a4e0731
c3a79e0
69e8454
781df29
1e33f46
187c866
0f71b06
d9a9548
88d021f
4865670
2a494b5
3acb2cc
8fbc629
e27b750
c59b570
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,10 +11,21 @@ import ( | |
) | ||
|
||
type directoryHistory struct { | ||
*AudState | ||
addr string | ||
signKey sign.PublicKey | ||
snapshots map[uint64]*DirSTR | ||
latestSTR *DirSTR | ||
snapshots []*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{ | ||
a, | ||
addr, | ||
make([]*DirSTR, 0), | ||
} | ||
h.updateVerifiedSTR([]*DirSTR{initSTR}) | ||
return h | ||
} | ||
|
||
// A ConiksAuditLog maintains the histories | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. auditor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
@@ -100,6 +116,7 @@ func (l ConiksAuditLog) Insert(addr string, signKey sign.PublicKey, | |
// create the new directory history | ||
h = newDirectoryHistory(addr, signKey, snaps[0]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please verify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// FIXME: remove this check --> caller calls Audit() before this function | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
You also said that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are multiple ways to approach this. Now, I do agree that passing the response message to So, given my previous statement about how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, what I'm suggesting is
I haven't implemented this yet, because it's part of the server-auditor protocol. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's "Go" things, you are declaring There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😕 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh gotcha! Good point, sorry for being slow :( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test to coverage this path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we move this check into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check only applies to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this path is already covered in tests that include https://github.com/coniks-sys/coniks-go/blob/generic-auditor/protocol/auditlog_test.go#L29 via https://github.com/coniks-sys/coniks-go/blob/generic-auditor/protocol/testutil.go#L51 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I got it.
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 { | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
|
||
|
@@ -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 | ||
} | ||
|
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I need to think about it some more, but I think that the only new type of auditor which would have to implement the |
||
AuditDirectory([]*DirSTR) error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/New/NewAuditor |
||
func NewAuditor(signKey sign.PublicKey, verified *DirSTR) *AudState { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect the return type here is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed about this at #170 (comment). For now, |
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please reorder these functions as:
|
||
// 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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should add a FIXME:
here or in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a FIXME? Why should we be saving inconsistent STRs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me think we should do:
I think we should have a TODO/FIXME here so we can come back later, when we address client recovery & whistle-blowing issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
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 { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can open a separate pull later, since I already have this. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// 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] | ||
} |
This file was deleted.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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