coniks-sys/coniks-go

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

liamsi opened this issue · 13 comments

liamsi commented

Code in question:

var (
	key = []byte("key")
)
// [...]

func doRequestAndVerify(d *ConiksDirectory, cc *ConsistencyChecks,
	requestType int, name string) error {
	switch requestType {
	// ...
	case KeyLookupType:
		request := &KeyLookupRequest{
			Username: name,
		}
		res, _ := d.KeyLookup(request)
		return cc.HandleResponse(requestType, res, name, key)
        // ...
	}
	// ...
}

https://github.com/coniks-sys/coniks-go/blob/master/protocol/consistencychecks_test.go#L20-L24

Shouldn't HandleResponse take the returned key contained in the result res instead; although it should be the same value (key = []byte("key")) it feels wrong that this test-method basically ignores the result of the lookup. A client doing a key-lookup usually wouldn't know the key in advance. Sorry for noticing so late!

liamsi commented

/cc @c633

vqhuy commented

HandleResponse takes key to verify the binding and it TOFU in case of key lookup (see https://github.com/coniks-sys/coniks-go/blob/master/protocol/consistencychecks.go#L218-L222).
I'm not sure if we need to have a ConsistencyCheck's method to extract the lookup result from a response. Probably worth to discuss it tonight.

liamsi commented

HandleResponse takes key to verify the binding and it TOFU in case of key lookup (see https://github.com/coniks-sys/coniks-go/blob/master/protocol/consistencychecks.go#L218-L222).

Not sure I understand what you are saying. You mean the consistency check in the lookup case and calling HandleResponse is only there to check if the key has changed (after the first request)?

I'm not sure if we need to have a ConsistencyCheck's method to extract the lookup result from a response.

The tests suggest so: https://github.com/coniks-sys/coniks-go/blob/master/protocol/consistencychecks_test.go#L20-L24 (the case "key-lookup" is handled by a consistency check method) but maybe I'm missing sth. and this is only to verify that the lookup is still the same as in the first request as your TOFU remark suggests.

Probably worth to discuss it tonight.

Yes 👍

vqhuy commented

How is this going, @liamsi ?

The current ConsistencyChecks also ignored the msg.Error (e.g., ErrNameExisted and ErrNameNotFound etc), so basically, the client doesn't know whether its registration is succeeded or not. I think it is in the same situation with this issue.

Maybe have an extra protocol/client.go? Probably just need func (msg *Response) GetKey() in protocol/message.go (see e5ba1171dc61a4427d6ad3bdefdeb3a9ae302e5a), since the current ConsistencyChecks is doing only one thing that is verifying the response and I think it's quite good.

liamsi commented

How is this going, @liamsi ?

The wire-format which seems to have changed so that UnmarshalReponse doesn't return a Response struct that HandleResponse can handle (it always returns ErrMalformedDirectoryMessage even if the response looks valid). Hence, I got stuck debugging and understanding what has changed and if I'm doing right.
I need to adapt the unmarshalling first (should be fairly easy) and I hope I can finish #93 later today/tomorrow.

The changes you made in e5ba117 and #133 look useful on first sight.

vqhuy commented

UnmarshalResponse returns a DirectoryResponse while HandleResponse expects a Response (which includes DirectoryResponse and ErrorCode). Sorry for this inconvenience:(

liamsi commented

UnmarshalResponse returns a DirectoryResponse while HandleResponse expects a Response (which includes DirectoryResponse and ErrorCode). Sorry for this inconvenience:(

This was rather easy to find out and not really a problem. I helped myself by resp := &p.Response{errCode, response} (wrapping it again).

vqhuy commented

Cool! Is the problem still remaining?

liamsi commented

After some back and forth I had a new one (even earlier, in UnmarshalResponse):
json: cannot unmarshal object into Go value of type merkletree.AssocData
But I didn't really look into that one yet.

  • 👍1
masomel commented

You mean the consistency check in the lookup case and calling HandleResponse is only there to check if the key has changed (after the first request)?

I'm not sure if this question has been answered, but based on https://github.com/coniks-sys/coniks-go/blob/master/client/coniksclient/internal/cmd/lookup.go#L54 and the changes in #133, it seems like the test client won't check if the key has changed after the first lookup since it's always nil. I think that the key obtained in L59 should be "pinned" in the client's persistent storage, so, while we don't have key changes, the client can ensure that the keys for its known contacts are consistent. Or is this behavior we no longer want?

vqhuy commented

I think it's a TODO? We can do it when we introduce the hooks for the backend storage and monitoring verification. Moving this to the next release seems fine to me.

masomel commented

I was under the impression that the kvdb storage backend hooks would only be for the server, since the client probably won't use a full db. I'm also not sure that storing looked-up keys for later consistency checking is dependent on monitoring being implemented.

vqhuy commented

Yep, probably not. Please ignore that comment.

  • 👍1