Implement registration function by vqhuy · Pull Request #29 · coniks-sys/coniks-go

vqhuy · 2016-07-22T09:28:25Z

Should resolve Implement registration #28

arlolra · 2016-07-22T14:09:17Z

This file should be in server/, no?

My intention is it just a sample usage of the library.

Ok, then please rename it to main.example.go or some such, and add a comment at the top of the file indicating the intention.

I still think it should be in server/ though, since it's a sample usage of this package.

Also, the server/README.md seems to describe it that way.

The layering approach is also interesting and I can be convinced of it.

I think there's a little more layering we can do, though I agree that any server executable we build should be considered to be a reference implementation. Other developers might want to have a different server architecture, and we should allow for that. As @c633 and I had originally thought, I think the protocols package should also include a high-level API for the CONIKS protocols and transparency operations: registration, lookups, key changes, updating the directory and generating (and publishing) new STRs, all of these with the option to store them to a DB. The handlers in the reference implementation could then simply use this API. How does this sound?

But how I originally thought about these was that they'd be separate processes, and the server would only accept registrations from the privileged places (ie. only registrations from localhost)

Also agree on this one. This also just seems like a configuration in the server, no?

But how I originally thought about these was that they'd be separate processes, and the server would only accept registrations from the privileged places (ie. only registrations from localhost)

I would like to count @liamsi in, since we all agree on this.

This also just seems like a configuration in the server, no?

I don't think so, since the server also has to accept lookup requests, too. I just have only 2 solutions for this:

We can check if the registration requests are originated from the localhost or not. (something like conn.RemoteAddr() == conn.LocalAddr()). But I don't know whether there is any type of attacks that can fake the remote address?

Sign the request with registration bot's private key, and the key server verifies the request using the bot's public key. This way, the bot and the key server can run on 2 separated machines.

What do you think?

But how I originally thought about these was that they'd be separate processes, and the server would only accept registrations from the privileged places (ie. only registrations from localhost)

I would like to count @liamsi in, since we all agree on this.

I also thought about it in exactly this way. But I also agree on the security concerns @c633 raised below.

We can check if the registration requests are originated from the localhost or not. (something like conn.RemoteAddr() == conn.LocalAddr()). But I don't know whether there is any type of attacks that can fake the remote address?

I fear this alone doesn't give you any reasonable security. We have to find ways to additionally secure that (firewall/network setup, TLS). But all this makes only sense if we want both processes also to be able to run on different machines (on the same machine they could use other means to communicate with each other).

I fear this alone doesn't give you any reasonable security. We have to find ways to additionally secure that (firewall/network setup, TLS). But all this makes only sense if we want both processes also to be able to run on different machines (on the same machine they could use other means to communicate with each other).

That's why I suggest the 2) option.

I had assumed we'd accept registrations on a separate port, and have the firewall restrict access to it. But 2) is also a viable option. Does anyone have a strong preference?

arlolra · 2016-07-22T16:42:22Z

See #29 (comment)

Should I put it into a sub-directory, namely bin? /cc @masomel

Isn't bin conventionally the place where compiled binaries are placed? Maybe naming the package something like keyserver or coniksServer would be more descriptive.

~~If so, I would move all the source files except main.example.go into coniksserver subpackage.~~
Never mind. I got your idea.

@arlolra @masomel : What do you think about 8be171fe5d2019717636019f32e5df888b3a0ea5 and 78db200a2a10b46ece5c5a973d384e3bbe330921?

masomel · 2016-07-22T18:38:53Z

If we split the reusable server code and the executable implementation into separate packages, I would include this module in the "reusable" package.

Looks like this is being discussed as part of the #29 (comment) thread.

I think it is done in 78db200a2a10b46ece5c5a973d384e3bbe330921

masomel · 2016-07-25T18:20:51Z

Now that we've split up the code, I think the description here should be something like "Key server executable" or "Key server reference implementation".

masomel · 2016-07-27T18:18:58Z

Seems like you're cutting off just one letter from directorSize. If you want to abbreviate, maybe just call it something like dirSize?

Oh sorry. :(

Btw, dirSize sounds good!

No worries at all!

arlolra · 2016-08-11T21:17:37Z

+	signKey sign.PrivateKey, dirSize uint64,
+	useTBs bool, capacity uint64) *ConiksDirectory {
+	d := new(ConiksDirectory)
+	d.policies = merkletree.NewPolicies(epDeadline, vrfKey)


d.SetPolicies(epDeadline, vrfKey)

arlolra · 2016-08-12T03:05:26Z

This pull is very big and unwieldy.

I think you should break it up into a few new pulls,

The changes to merkletree/policy.go and utils/util.go, which seem uncontroversial and can be merged right away.
The protocol/ package
The keyserver/ package, but when you make this pull, the base you choose is the pull for the protocol/ package so that we only see the diff between those two.

Then we can close this conversation with 180+ comments that's threatening to crash my browser.

vqhuy · 2016-08-12T03:10:38Z

Thanks for all your feedback so far! I'm working on this!

I think you should break it up into a few new pulls,

Yes, I totally agree!

Then we can close this conversation with 180+ comments that's threatening to crash my browser.

Me too. That's my bad. I'm sorry!

masomel · 2016-08-12T22:31:15Z

The keyserver/ package, but when you make this pull, the base you choose is the pull for the protocol/ package so that we only see the diff between those two.

One more question, are we going to wait to open this pull after the protocol pull is merged? It seems only pulls for the protocol and the policy/util stuff were created.

arlolra reviewed Jul 22, 2016
View reviewed changes

vqhuy force-pushed the server-registration branch from 07f39af to e731ef9 Compare July 22, 2016 16:38

arlolra reviewed Jul 22, 2016
View reviewed changes

vqhuy changed the title ~~Implement registration function~~ [WIP] Implement registration function Jul 22, 2016

masomel reviewed Jul 22, 2016
View reviewed changes

vqhuy force-pushed the server-registration branch from fd73f1c to 8be171f Compare July 24, 2016 18:30

masomel reviewed Jul 25, 2016
View reviewed changes

masomel mentioned this pull request Jul 25, 2016

Public STR and TB fields for verification #35

Closed

vqhuy force-pushed the server-registration branch 4 times, most recently from a6c0c26 to 8e1b496 Compare July 27, 2016 08:26

masomel reviewed Jul 27, 2016
View reviewed changes

arlolra reviewed Aug 11, 2016
View reviewed changes

Refactor code

e10f76c

This was referenced Aug 12, 2016

Remove unnecessary Policies's method & add new util functions #63

Merged

Implement protocol: #64

Closed

vqhuy closed this Aug 12, 2016

vqhuy deleted the server-registration branch August 14, 2016 18:07

vqhuy mentioned this pull request Aug 16, 2016

Implement reference key server #71

Merged

Conversation

vqhuy commented Jul 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masomel Jul 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vqhuy Jul 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vqhuy Jul 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arlolra Aug 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arlolra commented Aug 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vqhuy commented Aug 12, 2016

Uh oh!

masomel commented Aug 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

masomel Jul 23, 2016 •

edited

Loading

vqhuy Jul 23, 2016 •

edited

Loading

vqhuy Jul 24, 2016 •

edited

Loading

arlolra Aug 11, 2016 •

edited

Loading

arlolra commented Aug 12, 2016 •

edited

Loading