go testclient #93

liamsi · 2016-09-24T19:42:25Z

Minimalistic test-client. Part of #90.

currently registration & lookup is implemented

Differences to a real client implementation:

client doesn't use (twitter-)bot for registration (see discussion: Client cli #90 (comment))
thus, registration only works from the same machine as the server is running on
client does not store and verify the hash chain
client uses testutils package (which doesn't verify server's tls-cert. chain, for instance)

I would prefer to have the current code reviewed and create separate (smaller) issues/PRs for the above bullet-points.

TODOs:

~~integrate with Add verification methods #74 as soon as it is merged~~ Update:Add TODOs for parts of the code where the actual client struct/methods from Add verification methods #74 would be used (actually tried to make use of the consistency checks as much as possible)
Fix: json: cannot unmarshal object into Go value of type merkletree.AssocData (ugly hack ...)
Add any API documentation needed (formerly [WIP] Write client-side protocol doc #86).

liamsi · 2016-09-25T17:30:40Z

@c633 @masomel @arlolra Whoever has time: I think this is ready for a first review.

Start the server and play around with:

cd $GOPATH/src/github.com/coniks-sys/coniks-go/client/testclient/
go build 
./testclient register -n alice -k totally-fake-key -s /tmp/coniks.sock # adjust the socket according to your server config
./testclient lookup -n alice
./testclient lookup -n aaalice
# wait for epoch to finish and again try
./testclient lookup -n alice

vqhuy · 2016-09-26T03:30:49Z

keyserver/listener.go

@@ -107,7 +107,7 @@ func (server *ConiksServer) makeHandler(acceptableTypes map[int]bool) func(msg [
 		response, e := server.dir.HandleOps(req)
 		switch req.Type {
 		case KeyLookupType, KeyLookupInEpochType, MonitoringType:
-			server.RLock()
+			server.RUnlock()


Could you rebase this pull on top of master branch?

vqhuy · 2016-09-26T03:42:26Z

client/testclient/cmd/lookup.go

+			fmt.Println("Error while retrieving repsonse: " + err.Error())
+			os.Exit(-1)
+		}
+		response, errCode := client.UnmarshalResponse(p.KeyLookupType,


For L36 - L80, I think we will eventually have client.Verify(operationType, response) function that wraps all these verifications. This function would also need to handle this TODO (https://github.com/coniks-sys/coniks-go/blob/master/client/cgo/verification.go#L40-L45), much like what you did below. What do you think?

The same for registration code.

Makes sense! Do you think this should be done in this PR, too? (or in a separate one)

Maybe in a separate one. I can handle this.

Maybe in a separate one. I can handle this.

I could also take care of this (if you haven't started yet)

Thanks! I'm working on this in #74. How do you feel about https://github.com/coniks-sys/coniks-go/blob/f0e42ca6d6ce4b0ade502a769f4b8727d8c295d8/client/client.go?

I think we should have a Client struct in protocol package and make msg.Verify()'s arguments as its fields. (I'm working on this)

vqhuy · 2016-09-26T03:44:17Z

client/testclient/cmd/lookup.go

+			c := res.Verify(name, []byte(res.AP.Leaf.Value), 0, nil,
+				conf.SigningPubKey)
+			// verify auth. path:
+			if c != p.Passed || res.AP == nil ||


Shouldn't this verification be included in res.Verify? (see https://github.com/coniks-sys/coniks-go/blob/953f8fd0e24fb07f993c571bf38a4b7aed4db015/protocol/verification.go)

Yes, I guess this makes sense. Especially if we want a clear API someone can use as a reference to implement his own client.

If we do so, should we also need to verify the hash chain as well?

I think we can store the latest STR to client/config.toml (with an explanation that that field is being used for testing only, the developers should actually store the latest STR in some persistent storages) and whenever the test client runs, it reads the latest STR from this config file. What do you think?

If we do so, should we also need to verify the hash chain as well?

I think, yes we should.

I think we can store the latest STR to client/config.toml

Yeah, maybe for testing purposes that's a good idea!

masomel · 2016-09-26T17:52:41Z

@liamsi I'm trying to run the test client with the following command:

testclient register -config /disk/local/melara/coniks-testclient/config.toml -n alice -k totally-fake-key -s /tmp/coniks.sock

and get the following error: Failed to load config: open onfig: no such file or directory. The -config flag doesn't seem to be working (looks like it's interpreting -config as -c onfig). Is this expected?

liamsi · 2016-09-26T17:59:41Z

@masomel It seems like you have to call it with --config instead of -config if you choose the long flag name. I didn't know that either (I'm using the cobra framework for the first time, too). If you run it with the -h flag it shows -c, --config string Config file ....

masomel · 2016-09-26T18:05:14Z

client/testclient/cmd/common.go

+
+The client looks for a file called 'config.toml' in its current working directory.
+If you prefer the config-file to be named or stored somewhere different you can
+specify where to look for the config with the -config flag. For example:


-config should be --config or -c

Oops, fixed in: a1f8c6f

Maybe a mkConfig is fair enough? (see my comment #93 (comment)).

I do not fully understand your comment. I guess, you mean create a mkConfigjust for the client (makes sense); but how does that relate to storing the STR in the config, too?

Are you proposing that for boostraping the chain we should write the initial STR into the config file? If yes, mkConfig is called before the server is started and hence before the first STR is created.

I just want to have a generated template file for the test client.

Are you proposing that for boostraping the chain we should write the initial STR into the config file?

It's a good idea. But do we run the test client on the same machine as the key server?

I just want to have a generated template file for the test client.

OK, makes sense.

It's a good idea. But do we run the test client on the same machine as the key server?

Currently, the testclient only works if ran on the same machine as the key-server: #90 (comment)
Eventually, this will change.

Yup, great!

Done (mkConfig) as discussed in 2638588

masomel · 2016-09-26T18:19:38Z

client/testclient/cmd/register.go

+For more information consult: https://godoc.org/github.com/coniks-sys/coniks-go/bots
+
+Example call:
+  coniksclient register -name Alice@twitter -key fake_test_key`,


s/-name/--name and s/-key/--key

I should have sticked with the short version of flags in the usage string (which I tried myself) ;-) Thanks for testing it! Fixed in bd8d071

vqhuy · 2016-10-12T09:04:07Z

client/testclient/cmd/register.go

+			fmt.Println("Succesfully registered name: " + name)
+			// TODO Where should the client save seen STRs and the TB?
+			// (or should it wait till the next epoch to see if the
+			// TB was actually inserted?)


There maybe have more than one TB since the client could also receive the TB of her contacts, and we definitely have to check if the TB as been included as promised. Given that we will have a ConiksClient to store verified STRs and TBs (see 266d8cd#diff-75d470fe65b4ef49cd55654004d3edeaR29), I'm thinking that should we store both STRs and TBs in this struct?

There maybe have more than one TB since the client could also receive the TB of her contacts, and we definitely have to check if the TB as been included as promised.

The client struct looks definitely like a good idea and I agree this would be the good place to store STRs and TBs (at least in memory). The current ConiksClient also only stores one TB, or not?

It should store an array of TB. Wait me a sec.

vqhuy · 2016-11-24T03:33:24Z

client/testclient/cmd/lookup.go

+		}
+		req, err := createLookupRequest(name)
+		if err != nil {
+			fmt.Println("Couldn'r create request!")


Nit-picking: s/Couldn'r/Couldn't

vqhuy · 2016-11-24T03:34:13Z

client/testclient/cmd/update.go

+	"github.com/spf13/cobra"
+)
+
+// TODO figure out if this needs to be a separate command or we can simply re-use


Or we can temporarily remove this command?

Removed ...

vqhuy · 2016-11-24T03:36:37Z

client/testclient/cmd/register.go

+	Use:   "register",
+	Short: "Register a name-to-key binding.",
+	Long: `Register a new name-to-key binding on the CONIKS-server.
+Note that, for the registration to work, the test-client needs to be running on the same machine as the key-server.


Since #130 has landed, could we remove this note?

Also removed

* An alternative to what's here could be to have responses use a protocol.DirSTR, rather than merkletree.SignedTreeRoot, but that might be unsatisfying for other reasons.

- It may easier for ppl to invoke `coniksclient` from terminal when they install the test client with `go install`. - Add README.md for client

…pe merkletree.RawAd

arlolra · 2016-11-27T20:28:16Z

client/coniksclient/internal/cmd/register.go

+		var res []byte
+		if conf.RegAddress == "" {
+			// fallback to conf.Address if empty
+			conf.RegAddress = conf.Address


Can we define a regAddress := conf.RegAddress above and then set that here?

arlolra · 2016-11-27T20:39:21Z

client/coniksclient/internal/cmd/register.go

+}
+
+func createRegistrationMsg(name, key string) ([]byte, error) {
+	return json.Marshal(&p.Request{


This probably belongs in encoding.go

arlolra · 2016-11-27T20:43:45Z

client/coniksclient/internal/cmd/register.go

+				// TODO: Save the cc to verify the TB and for later
+				// usage (TOFU checks)
+			case p.ReqNameExisted:
+				// Key-change isn't currently supported; see:


We won't be here for a key change because we won't get a .CheckPassed

Probably we should finish #133 first.

Ok, can we remove this comment in favour of a pointer to that then?

arlolra · 2016-11-27T20:45:23Z

client/coniksclient/internal/cmd/lookup.go

+			case p.ReqSuccess:
+				key, err := response.GetKey()
+				if err != nil {
+					fmt.Println("Cannot the key from the response, error: " + err.Error())


Cannot get the key

arlolra · 2016-11-27T21:31:01Z

I think this looks good. You should squash and merge it ;)

liamsi · 2016-11-27T21:39:07Z

I think this looks good. You should squash and merge it ;)

I've followed the latest changes and I agree (I might be biased, though ;-). Thanks again for taking care and finishing/cleaning up this PR @c633 @arlolra!

liamsi force-pushed the client_go branch 3 times, most recently from 6e9a1fb to 026d786 Compare September 25, 2016 17:20

liamsi changed the title ~~[WIP] Client go~~ Client go Sep 25, 2016

liamsi changed the title ~~Client go~~ go testclient Sep 25, 2016

vqhuy reviewed Sep 26, 2016

View reviewed changes

liamsi force-pushed the client_go branch from 026d786 to e3503f4 Compare September 26, 2016 09:07

masomel reviewed Sep 26, 2016

View reviewed changes

masomel added this to the 0.1.0 milestone Sep 29, 2016

vqhuy reviewed Oct 12, 2016

View reviewed changes

liamsi added the client label Oct 15, 2016

masomel assigned liamsi Oct 19, 2016

masomel force-pushed the master branch from 46a16c4 to 058c6fd Compare October 25, 2016 19:05

vqhuy mentioned this pull request Nov 4, 2016

[WIP] Write client-side protocol doc #86

Closed

masomel mentioned this pull request Nov 10, 2016

Add doc.go to client #82

Closed

6 tasks

liamsi force-pushed the client_go branch from 19eb627 to adbd9e2 Compare November 18, 2016 21:50

liamsi changed the title ~~go testclient~~ [WIP]: go testclient Nov 18, 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 reviewed Nov 24, 2016

View reviewed changes

liamsi force-pushed the client_go branch 3 times, most recently from 7abbabc to ea9a12d Compare November 24, 2016 17:51

liamsi and others added 20 commits November 28, 2016 02:57

Super ugly workaround/hack for AssocData ...

b5e31b0

registration works again

e2ca7a3

Simple lookup also works again

bab4f9d

re-enable socket (renamed to address) as command line flag

11ecebf

some documentation

b775ab1

Policy unmarshalling should happen in the client

334d51d

* An alternative to what's here could be to have responses use a protocol.DirSTR, rather than merkletree.SignedTreeRoot, but that might be unsatisfying for other reasons.

Fix UnmarshallingResponse

c3addcc

Move testclient/cmd to internal

857756e

Change utils.WriteFile signature to make it more flexible

c2ffc66

Rename testclient -> coniksclient

1b8b383

- It may easier for ppl to invoke `coniksclient` from terminal when they install the test client with `go install`. - Add README.md for client

Fix UnmarshalResponse & refactor lookup.go, register.go

d34e1c3

Address should be read from config.toml

0748b7d

Don't handle real key-material

37d9a84

return error instead of nil

b4d985e

Fix decoding error: json: cannot unmarshal object into Go value of ty…

9706680

…pe merkletree.RawAd

Hotfix: tb.Verify TOFU

13a4d01

Remove TODO note in README

405d3e9

Fix some minor issues

7be0469

Add doc.go, update README and add some more documentations

892646e

Add TODO for DirSTR

fd98791

vqhuy force-pushed the client_go branch from 94d368d to fd98791 Compare November 27, 2016 19:58

FIx rebase artifacts

cd913b4

vqhuy force-pushed the client_go branch from 6c3d05b to cd913b4 Compare November 27, 2016 20:09

arlolra reviewed Nov 27, 2016

View reviewed changes

Move createXXX to encoding.go

4fad40a

arlolra closed this in 3f83809 Nov 27, 2016

arlolra deleted the client_go branch November 28, 2016 01:19

arlolra referenced this pull request Nov 28, 2016

Update client summary in readme

d6de94b

go testclient #93

go testclient #93

Conversation

liamsi commented Sep 24, 2016 • edited Loading

liamsi commented Sep 25, 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

vqhuy Oct 11, 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

masomel commented Sep 26, 2016 • edited Loading

liamsi commented Sep 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

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

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 27, 2016

liamsi commented Nov 27, 2016 • edited Loading

liamsi commented Sep 24, 2016 •

edited

Loading

liamsi commented Sep 25, 2016 •

edited

Loading

vqhuy Oct 11, 2016 •

edited

Loading

masomel commented Sep 26, 2016 •

edited

Loading

liamsi commented Sep 26, 2016 •

edited

Loading

liamsi commented Nov 27, 2016 •

edited

Loading