Make the usage of the registration bot configurable #130

vqhuy · 2016-11-18T21:02:40Z

Which do you prefer:

Set LocalAddress to an empty string to disable the registration bot
Or, have a IsUseBot field in ServerConfig?

masomel · 2016-11-18T21:41:46Z

Set LocalAddress to an empty string to disable the registration bot

This approach seems a little safer to me.

arlolra · 2016-11-18T23:58:14Z

Proposal: How about we allow a list of addresses to bind to in the config.toml? Something like,

[[addresses]]
address = "0.0.0.0:3000"
cert = "server.pem"
key = "server.key"

[[addresses]]
address = "0.0.0.0:6000"

[[addresses]]
address = "/tmp/coniks.sock"
allow_registration = true

masomel · 2016-11-19T04:02:11Z

How about we allow a list of addresses to bind to in the config.toml?

A few questions: How does the server know which address to choose for which type of connection (bot vs. client, testing vs deployment, etc.)? It also seems like the second address in the list is an unencrypted connection (as opposed to the first one with TLS settings) -- do we want to have a non-TLS option? What does the allow_registration = true entry in the third address mean, is this an alternative to a useBot entry in the configuration/what would allow_registration = false mean?

arlolra · 2016-11-19T05:45:00Z

How does the server know which address to choose for which type of connection (bot vs. client, testing vs deployment, etc.)?

Currently, the keyserver doesn't know anything about bots; it just listens on two addresses, with differing permission. The only change here is really to make that configurable, instead of hardcoded.

What does the allow_registration = true entry in the third address mean

I figured the safest default would be to disallow registration on all addresses, and for the unix socket you'd explicitly enable it.

It also seems like the second address in the list is an unencrypted connection (as opposed to the first one with TLS settings) -- do we want to have a non-TLS option?

Maybe not.

vqhuy · 2016-11-19T06:36:23Z

Does "disallow registration on all addresses" mean "all addresses can accept any request except registration"? Otherwise this totally makes sense to me.

arlolra · 2016-11-19T06:59:14Z

Yup. All the other request are of the "read" type. Registration is the only "write". If you want to think about it like that. So, by default, addresses are "read-only".

masomel · 2016-11-20T01:48:00Z

All the other request are of the "read" type. Registration is the only "write". If you want to think about it like that.

Fair point, I agree that this is a good way to think about it.

vqhuy · 2016-11-21T05:07:34Z

I gave it a shot in 88103d43a14cf9d19046df90da823025dae32b84.

arlolra · 2016-11-21T05:47:54Z

Nice work.

arlolra · 2016-11-21T16:24:10Z

keyserver/server_test.go

+
+	// test TCP network
+	addr := &Address{
+		Address:     "tcp:" + testutil.PublicConnection,


Maybe just change testutil.PublicConnection to include the protocol?

~~I think the latest is here: https://github.com/coniks-sys/coniks-go/pull/130/files#diff-d24d2023b15c72fdfbc6fec8561dcf0dR55~~ Sorry, my mistake.

Maybe just change testutil.PublicConnection to include the protocol?

Yes, it should be a constant instead.

arlolra · 2016-11-21T16:24:21Z

keyserver/server_test.go

+
+	// test Unix network
+	addr = &Address{
+		Address:           "unix:" + testutil.LocalConnection,


arlolra · 2016-11-21T16:47:57Z

keyserver/server.go

-		panic(err)
+	for i := 0; i < len(addrs); i++ {
+		addr := addrs[i]
+		ln, tlsConfig, perms := resolveAndListen(addr)


Do you think it's worth logging a warning if none of the addresses permit registration?

Yes, it makes sense. Added in 0f78e49ac4b22a8051818f30f103e7e74f2abf28. Thanks!

arlolra

LGTM

arlolra · 2016-11-22T04:22:10Z

Alright, good to merge.

vqhuy · 2016-11-22T05:28:34Z

Yes, wait me for one sec.

- Close #91

masomel · 2016-11-30T04:51:44Z

@c633 Maybe this is a silly question, how does one configure the server to use/not use the registration bot? It seems like the server default (i.e. in cmd/init.go) is still to use the bot. Or does this have to be configured manually after init?

arlolra · 2016-11-30T05:23:42Z

Maybe a better framing is to say that the default configuration is to only allow local registrations, and if you want to deviate, you need to manually edit. We leave the bot out of the picture.

masomel · 2016-11-30T05:35:11Z

Maybe a better framing is to say that the default configuration is to only allow local registrations, and if you want to deviate, you need to manually edit.

That works I guess (a flag for init may be more convenient, though I realize that may be difficult to implement). This isn't stated in the README or anywhere yet. Will have to add that.

vqhuy · 2016-11-30T05:55:38Z

Will have to add that.

Thanks!

vqhuy added this to the 0.1.0 milestone Nov 18, 2016

vqhuy added the server label Nov 18, 2016

arlolra assigned vqhuy Nov 19, 2016

vqhuy force-pushed the bot-switch branch 2 times, most recently from 58327ab to 88103d4 Compare November 21, 2016 05:06

vqhuy force-pushed the bot-switch branch 2 times, most recently from 495e9f5 to fdb5b72 Compare November 21, 2016 08:08

arlolra reviewed Nov 21, 2016

View reviewed changes

keyserver/server_test.go

// test Unix network

addr = &Address{

Address: "unix:" + testutil.LocalConnection,

Copy link

Member

arlolra Nov 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unix:// ?

arlolra reviewed Nov 21, 2016

View reviewed changes

arlolra approved these changes Nov 21, 2016

View reviewed changes

arlolra mentioned this pull request Nov 22, 2016

Add documentation for keyserver package #125

Closed

Make the usage of the registration bot configurable

5c92812

- Close #91

vqhuy force-pushed the bot-switch branch from 0f78e49 to 5c92812 Compare November 22, 2016 07:02

vqhuy merged commit 5c92812 into master Nov 22, 2016

vqhuy deleted the bot-switch branch November 22, 2016 07:02

vqhuy mentioned this pull request Nov 24, 2016

go testclient #93

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the usage of the registration bot configurable #130

Make the usage of the registration bot configurable #130

vqhuy commented Nov 18, 2016 •

edited

Loading

masomel commented Nov 18, 2016

arlolra commented Nov 18, 2016

masomel commented Nov 19, 2016

arlolra commented Nov 19, 2016

vqhuy commented Nov 19, 2016

arlolra commented Nov 19, 2016

masomel commented Nov 20, 2016

vqhuy commented Nov 21, 2016

arlolra commented Nov 21, 2016

arlolra Nov 21, 2016

arlolra Nov 21, 2016 •

edited

Loading

vqhuy Nov 21, 2016 •

edited

Loading

arlolra Nov 21, 2016

arlolra Nov 21, 2016

vqhuy Nov 21, 2016

arlolra left a comment

arlolra commented Nov 22, 2016

vqhuy commented Nov 22, 2016

masomel commented Nov 30, 2016

arlolra commented Nov 30, 2016

masomel commented Nov 30, 2016 •

edited

Loading

vqhuy commented Nov 30, 2016

Make the usage of the registration bot configurable #130

Make the usage of the registration bot configurable #130

Conversation

vqhuy commented Nov 18, 2016 • edited Loading

masomel commented Nov 18, 2016

arlolra commented Nov 18, 2016

masomel commented Nov 19, 2016

arlolra commented Nov 19, 2016

vqhuy commented Nov 19, 2016

arlolra commented Nov 19, 2016

masomel commented Nov 20, 2016

vqhuy commented Nov 21, 2016

arlolra commented Nov 21, 2016

arlolra Nov 21, 2016

Choose a reason for hiding this comment

arlolra Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

vqhuy Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

arlolra Nov 21, 2016

Choose a reason for hiding this comment

arlolra Nov 21, 2016

Choose a reason for hiding this comment

vqhuy Nov 21, 2016

Choose a reason for hiding this comment

arlolra left a comment

Choose a reason for hiding this comment

arlolra commented Nov 22, 2016

vqhuy commented Nov 22, 2016

masomel commented Nov 30, 2016

arlolra commented Nov 30, 2016

masomel commented Nov 30, 2016 • edited Loading

vqhuy commented Nov 30, 2016

vqhuy commented Nov 18, 2016 •

edited

Loading

arlolra Nov 21, 2016 •

edited

Loading

vqhuy Nov 21, 2016 •

edited

Loading

masomel commented Nov 30, 2016 •

edited

Loading