CheckBadBinding when registration #133

vqhuy · 2016-11-23T08:51:57Z

This somehow relates to Should the doRequestAndVerify ignore the result's returned key for the KeyLookupType case? #127

arlolra · 2016-11-26T19:33:51Z

protocol/consistencychecks.go

+			tbKey := key
+			if msg.Error == ReqNameExisted {
+				tbKey = df.TB.Value
+			}


I don't agree with this.

First, why only the TB case? What about if you try to register a different key from one that's already made its way into a signed root? That would return a ReqNameExisted and a differing key, and similarly fail to verify the auth path.

And, rightly so. It might be nice if the returned error was ErrBindingsDiffer or something, but this is still a case we want to fail a check on, no?

What about if you try to register a different key from one that's already made its way into a signed root?

Ah, right. Sorry for being hasty on this. I did have a test for this but didn't think it thoroughly :(

We are all use TOFU here, so I'm not sure if returning ReqNameExisted along with CheckPassed makes more sense?

Can you elaborate? Because, I think that's what we want. The CheckPassed is telling us that the ReqNameExisted is valid, and that the server proved it to us. The client might still need to act on ReqNameExisted.

I think the question is when we want to fail this check? After all the checks are passed or right away in e.g., verifyAuthPath.

Alright, why do you want this to fail later?

Should KeyChange be used here instead?

Ideally, but we don't have that yet.

In this case, how does client distinguish its own registration with other client's registration?

I'm going to assume you actually do mean "client" here (ie. the "user" is trying to reregister for some legitimate reason.) I imagine, in both cases, the client would notify the user that a different binding already exists for that account, and we'd let the user disambiguate the situation. If they understood that they're trying to initiate a key change, they'd do that, or otherwise report that something is amiss.

In other words, I'm not sure the client itself can distinguish which situation it's in. I think this is getting in the realm of UI, and that's something we've hardly tackled to this point.

The case where the registration succeeds but we get an ErrBindingsDiffer is a little more clearcut though, but I'm not sure why any deceitful server would bother doing this.

Also, if it gets ReqNameExisted and ErrBindingsDiffer because of other user's registration, will calling GetKey() and re-verifying with HandleResponse(new_key) make sense?

Can you clarify what you mean by "other user's registration"?

Can you clarify what you mean by "other user's registration"?

I meant the user name is already registered by someone else. I was thinking of what the client should do when getting an ErrBindingsDiffer.
UPDATE:

The case where the registration succeeds but we get an ErrBindingsDiffer is a little more clearcut though, but I'm not sure why any deceitful server would bother doing this.

~~Maybe the server is actually trying to equivocate?~~ I don't know ... Is this one?

Also, on request, if the server does the registration but sneaks in some other key, it would return Success but we'd get ErrBindingsDiffer.

How can someone else register your username? Doesn't that imply they have access to your account? Again, I think this is a case covered above where the user themself needs to be notified.

Maybe the service doesn't use a bot though. Maybe I'm just overthinking about this.

Again, I think this is a case covered above where the user themself needs to be notified.

Yep, I agree with this.

Maybe the service doesn't use a bot though.

Regardless of the particular solution (bots being one), I believe the server should use an authenticated way of registering bindings.

arlolra · 2016-11-26T19:36:44Z

protocol/consistencychecks_test.go

-		return cc.HandleResponse(requestType, res, name, key)
-	case MonitoringType:
-	case KeyLookupInEpochType:
+func registerAndVerify(d *ConiksDirectory, cc *ConsistencyChecks,


These functions should really be returning the error from the directory operation we're ignoring. See all the FIXMEs already in this file.

Did you mean errors included in Errors map? Are they already being returned in HandleResponse?

I mean below, where you're removing // FIXME: Check that we got an ReqNameExisted. I don't think that's ok.

Since this assertion should be checked in directory_test.go, I think it can be removed.

You're saying that since it's covered there, it's redundant here? That's true.

As a counterpoint though, the code currently looks like,

// test error name existed if registerAndVerify(d, cc, alice, key) != CheckPassed { t.Fatal("Unexpected verification result") }

As someone reading it for correctness, it's really not obvious that CheckPassed means that ReqNameExisted. To me, CheckPassed just means that the response was consistent. It doesn't tell me what it was though.

I guess my point is the purpose of consistencychecks_test.go is to test the consistency verification.

You're saying that since it's covered there, it's redundant here

What I'm trying to say is that since we already have a test for this there, we can quite confident about the checks here. Maybe my sense was wrong :(

I'm not saying you're wrong; just that the tests would be more understandable to me that way. There's nothing in registerAndVerify(d, cc, alice, key) returning CheckPassed that indicates to me that the error name existed. The registration could have equally well succeeded. We know it couldn't, because of the tests elsewhere, as you said, but that just isn't clear when trying to read this linearly.

Fair enough. Sorry for wasting your time on this :(.

Sorry for wasting your time on this :(

You certainly did not waste my time. I'm happy that you're challenging my position; please continue to do so. That's how we improve.

arlolra · 2016-11-26T19:38:35Z

protocol/message.go

+	case DirectoryProof:
+		return df.AP.Leaf.Value
+	case DirectoryProofs:
+		return df.AP[len(df.AP)-1].Leaf.Value


Is this going to work well with KeyLookupInEpoch ranges?

In the current discussion on KeyLookupInEpoch, we think it's okay to just return only one AP for the lookup (#70 (comment)).

Ok, it's just not obvious to me that AP[len(df.AP)-1] is the right thing, especially when the documentation there says,

latest signed tree roots

and in the KeyLookupInEpoch it probably means most recent?

But, more to the point, GetKey isn't being used yet, so do we really need it? If we do, can we add it when there's a test or use?

Oops, I will fix the doc.

This would be used in #93, but I should add a test for it, too.

Okay, I added tests and also fixed GetKey in 60a16502c71f4f6ed1e027c63a9bf755f5eee787. Thanks for asking me to write tests.

arlolra · 2016-11-26T19:40:40Z

protocol/tb.go

@@ -40,5 +40,5 @@ func (tb *TemporaryBinding) Serialize(strSig []byte) []byte {
 // the existed binding (TOFU).
 func (tb *TemporaryBinding) Verify(index, value []byte) bool {
 	return bytes.Equal(tb.Index, index) &&
-		(value != nil && bytes.Equal(tb.Value, value))
+		(value == nil || (value != nil && bytes.Equal(tb.Value, value)))


When is value nil? Also, if we do need this, it should be (value == nil || bytes.Equal(tb.Value, value)).

Ok, I see what's happening here now.

In verifyAuthPath we do,
https://github.com/coniks-sys/coniks-go/blob/master/protocol/consistencychecks.go#L225-L229

Maybe we should do the same in verifyReturnedPromise?

Sure. I thought that too.

arlolra · 2016-11-28T02:48:39Z

client/coniksclient/internal/cmd/register.go

+			case p.ReqNameExisted:
+				fmt.Println(`Are you trying to update your binding? Unfortunately, KeyChange hasn't been supported yet.`)
+			case p.ReqSuccess:
+				fmt.Println("Oops! Probably the server is trying to sneak in some other key.")


"Oops! The server snuck in some other key."

arlolra · 2016-11-28T02:49:11Z

client/coniksclient/internal/cmd/register.go

+				if err != nil {
+					fmt.Println("Cannot get the key from the response, error: " + err.Error())
+				}
+				fmt.Println("You got [" + string(recvKey) + "] instead of [" + string(key) + "]")


"[ ... ] was registered instead of [ ... ]"

arlolra · 2016-11-28T02:56:39Z

protocol/consistencychecks.go

+			return CheckBadLookupIndex
+		}
+	} else {
+		if !ap.VerifyBinding([]byte(uname), key) {


Shouldn't you do the same as below and check key != nil && and remove setting key above?

arlolra · 2016-11-28T02:57:20Z

protocol/consistencychecks.go

-
-	return nil
+	if ap.VerifySTR(str.TreeHash) {
+		return nil


Can you return CheckBadAuthPath here and then nil as the final case?

arlolra · 2016-11-28T03:01:19Z

merkletree/proof.go

+}
+
+// Verify combines VerifyBinding(), VerifyIndex(), and VerifySTR(),
+// and abstracts away the detail of the failure.
 // This should be called after the VRF index is verified successfully.
 func (ap *AuthenticationPath) Verify(key, value, treeHash []byte) bool {


This is dead code now, no?

Seems so to me, too.

Hmm, a separate API of the merkletree package, no?

masomel

I only have a minor nitpick and a question about VerifyBinding()

masomel · 2016-11-28T05:38:04Z

client/coniksclient/internal/cmd/register.go

+		case p.CheckBindingsDiffer:
+			switch response.Error {
+			case p.ReqNameExisted:
+				fmt.Println(`Are you trying to update your binding? Unfortunately, KeyChange hasn't been supported yet.`)


nitpicky: s/hasn't been/isn't

masomel · 2016-11-28T05:51:57Z

merkletree/proof.go

+// of the proof node of the authentication path.
+//
+// This should be called only when ap is a proof of inclusion.
+func (ap *AuthenticationPath) VerifyBinding(key, value []byte) bool {


I feel like this should also actually check that ap.Leaf.Index == ap.LookupIndex. Correct me if I'm wrong, but I think we got rid of this check entirely since we introduced a.ProofType()?

Yes, rather than the comment,

// This should be called only when ap is a proof of inclusion.

why not just assert ap.ProofType() == ProofOfInclusion as well?

Hmm, I think I was trying to eliminate some duplicate computations here. Now, I got the idea.

why not just assert ap.ProofType() == ProofOfInclusion as well?

This particular check is already done at the cc.verifyAuthPath level, isn't it? I'm referring to the explicit comparison of the leaf's index and the ap.LookupIndex. I don't see this check on the client-side anywhere anymore, and I don't think it's enough to just ensure that the server set ap.ProofType() = ProofOfInclusion.

Is what we have in 5420200 right?

the server set ap.ProofType() = ProofOfInclusion.

I'm not sure what it means but are you saying that the ap.ProofType is set by the server? It is computed by the client when it receives an auth path (see https://github.com/coniks-sys/coniks-go/blob/master/merkletree/proof.go#L118).

Oh, ProofType() needs documentation, though.

Okay, I'll add it in #132 .

Sorry, one more question: Should the server be setting proofType = undeterminedProof explicitly in merkltree.Get? It seems like a server could set proofType to whatever it wanted and ap.ProofType() wouldn't validate that setting except for in the case of undeterminedProof.

Yes, since ap.proofType is unexported, it shouldn't be transmitted over the wire. So setting proofType from the server-side has no impact on the client.

Got it, that makes sense. Thanks!

arlolra · 2016-11-28T17:59:02Z

merkletree/proof_test.go

@@ -7,6 +7,16 @@ import (
 	"github.com/coniks-sys/coniks-go/utils"
 )

+func verifyAuthPath(ap *AuthenticationPath, key, value []byte, treehash []byte) bool {


I'm wondering if, rather than inlining the ap.Verify and tb.Verify and ending up w/ a duplicated verifyAuthPath, it would be better to change the return type of those methods to error?

Hmm, then the merkletree package has to import protocol/error.go, right?

No, you'd do an error type conversion in protocol.

if error == merkletree.BindingsDiffer{ return CheckBindingsDiffer }

Done in 75cf9d7. What do you think?

arlolra · 2016-11-28T19:10:06Z

merkletree/proof.go

 			}
 		}
-	} else { // proof of inclusion
+		if ap.Leaf.Value != nil {
+			return ErrBindingsDiffer


Is this true? The returned leaf doesn't have to be empty.

I suppose (https://github.com/coniks-sys/coniks-go/blob/master/merkletree/merkletree.go#L101-L102). This asserts that the client won't get a invalid key even it misuses GetKey().

Ok, thank you, I forgot we did that. Is it worth putting a comment here for people like me who don't remember so well?

Sure.

people like me who don't remember so well.

I doubt about this :P

arlolra · 2016-11-28T19:30:30Z

merkletree/proof.go

-		if !ap.verifyBinding(key, value) {
-			return false
+		if !bytes.Equal(ap.Leaf.Value, value) ||
+			!ap.Leaf.Commitment.Verify(key, value) {


Should there be a separate error type for the commitment verification failing?

arlolra · 2016-11-28T19:32:35Z

protocol/consistencychecks.go

 		return CheckBadAuthPath
+	default:


Would it be safer if the default was to panic and add a case for nil?

Definitely. Thanks!

arlolra · 2016-11-28T19:59:40Z

Ok, LGTM. A general comment is that we probably need greater test coverage around these error cases in the merkletree, since there wasn't much updating to do there.

vqhuy · 2016-11-28T20:03:54Z

Yes, the tests are on their way.

vqhuy · 2016-11-29T01:58:04Z

I add some more tests in 4a21bea.

arlolra · 2016-11-29T02:35:31Z

Thanks! Looks good.

vqhuy · 2016-11-29T03:50:58Z

Merged in 2c3561a.

vqhuy force-pushed the fix-reg branch from 3361430 to c2c208c Compare November 23, 2016 08:52

vqhuy added bug protocol labels Nov 23, 2016

vqhuy added this to the 0.1.0 milestone Nov 23, 2016

vqhuy self-assigned this Nov 23, 2016

liamsi mentioned this pull request Nov 23, 2016

Should the doRequestAndVerify ignore the result's returned key for the KeyLookupType case? #127

Closed

vqhuy mentioned this pull request Nov 25, 2016

go testclient #93

Closed

3 tasks

vqhuy force-pushed the fix-reg branch from 7d75325 to 3e975c6 Compare November 25, 2016 12:39

arlolra requested changes Nov 26, 2016

View reviewed changes

vqhuy force-pushed the fix-reg branch 2 times, most recently from 687b44f to 60a1650 Compare November 27, 2016 10:45

vqhuy mentioned this pull request Nov 27, 2016

Fix tb.Verify() #136

Closed

vqhuy changed the title ~~Hotfix: Registration collisions~~ [WIP] CheckBadBinding when registration Nov 27, 2016

arlolra removed the bug label Nov 27, 2016

vqhuy force-pushed the fix-reg branch 4 times, most recently from f25cb1a to 75d6c3b Compare November 28, 2016 02:00

Try to fix verifyAuthPath

ab4aa1d

vqhuy force-pushed the fix-reg branch from 75d6c3b to ab4aa1d Compare November 28, 2016 02:01

vqhuy changed the title ~~[WIP] CheckBadBinding when registration~~ CheckBadBinding when registration Nov 28, 2016

arlolra reviewed Nov 28, 2016

View reviewed changes

masomel requested changes Nov 28, 2016

View reviewed changes

vqhuy force-pushed the fix-reg branch 2 times, most recently from 32aa969 to 6cd5fe7 Compare November 28, 2016 15:58

Fix AP's verification & typos

5420200

vqhuy force-pushed the fix-reg branch from 6cd5fe7 to 5420200 Compare November 28, 2016 16:00

arlolra reviewed Nov 28, 2016

View reviewed changes

masomel approved these changes Nov 28, 2016

View reviewed changes

Fix AP verification

75cf9d7

arlolra reviewed Nov 28, 2016

View reviewed changes

Add some comments

21fb194

arlolra reviewed Nov 28, 2016

View reviewed changes

Fix

bae9a05

vqhuy force-pushed the fix-reg branch from f0b80e1 to bae9a05 Compare November 28, 2016 19:47

arlolra approved these changes Nov 28, 2016

View reviewed changes

Add tests

4a21bea

vqhuy closed this Nov 29, 2016

vqhuy deleted the fix-reg branch November 29, 2016 05:49

CheckBadBinding when registration #133

CheckBadBinding when registration #133

Conversation

vqhuy commented Nov 23, 2016

Choose a reason for hiding this comment

vqhuy Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra Nov 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra commented Nov 28, 2016

vqhuy commented Nov 28, 2016

vqhuy commented Nov 29, 2016

arlolra commented Nov 29, 2016

vqhuy commented Nov 29, 2016

vqhuy Nov 27, 2016 •

edited

Loading

vqhuy Nov 27, 2016 •

edited

Loading

arlolra Nov 26, 2016 •

edited

Loading

arlolra Nov 27, 2016 •

edited

Loading

vqhuy Nov 27, 2016 •

edited

Loading

masomel Nov 28, 2016 •

edited

Loading