-
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 all 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 |
---|---|---|
@@ -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 | ||
} | ||
|
||
// 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 { | ||
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 | ||
} | ||
|
||
// 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 { | ||
// 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 | ||
|
||
// 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 | ||
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 |
||
// 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 | ||
} |
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.
Please verify
snaps[0]
against thepinnedSTR
before inserting.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.
Audit()
is going to be called beforeInsert()
, sosnaps[0]
will have been verified at this point.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.
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 innewDirectoryHistory
. What do you think?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.
Similar to my comment in #170 (comment), I don't think that it's
newDirectoryHistory
's job to verifysnaps[0]
, especially if it's being called in the context ofAudit()
.