Registration bots #33

vqhuy · 2016-07-24T04:10:56Z

Should resolve Use Unix sockets for communication between the bot and the server #73 as well.

liamsi · 2016-08-08T09:59:03Z

bots/bot.go

+
+func SendRequestToCONIKS(address string, msg []byte) ([]byte, error) {
+	conf := &tls.Config{
+		InsecureSkipVerify: true,


I would prefer to either use no TLS connection between the bot and the coniks server (as long as they run on the same machine) or to not use the InsecureSkipVerify mode (see https://golang.org/src/crypto/tls/common.go?s=#L302 for details)

Thanks! InsecureSkipVerify is used for testing only.

The reason I would like to use TLS connection here is to prevent (say) malicious sysadmins from intercepting the connection between the bots and the server, and thus to preserve the privacy of the protocol.

The reason I would like to use TLS connection here is to prevent (say) malicious sysadmins from intercepting the connection between the bots and the server, and thus to preserve the privacy of the protocol.

But if the sysadmin has access to both TLS certs, the connection can still be intercepted, no?

Yes, you're right. But that chance for they to have access to both TLS certs is so small, I think. Hopefully, this would be an extra security layer for our implementation.

But if the sysadmin set up both the server and the registration bot, I think that the chance of them having access to both TLS certs isn't low, is it?

Yes, this's also true. Hmm, so basically shouldn't we use TLS here?

@arlolra suggested that we could use unix socket here instead (see #73 )

arlolra · 2016-08-27T03:55:35Z

bots/twitterbot.go

+		return string(res)
+	}
+
+	// create new CONIKS registration request


Should you just use keyserver.UnmarshalRequest and then validate the request type and strings.EqualFold(strings.ToLower(username) + "@twitter", req.Username)?

I'm not sure if we want to have username field in the request. Why do you prefer this way?
Edit: I see your point, maybe because of this comment #33 (comment)?

I just prefer reusing UnmarshalRequest. The username isn't strictly needed, true, we can construct it, but it does establish the convention that's going to be used later in lookups, so maybe validating it ensures the client understands.

arlolra · 2016-08-27T15:24:23Z

bots/twitterbot.go

+		request, ok := req.Request.(*p.RegistrationRequest)
+		if req.Type != p.RegistrationType || !ok ||
+			// issue: https://github.com/coniks-sys/coniks-go/issues/30
+			(ok && strings.EqualFold(strings.ToLower(username)+"@twitter", request.Username)) {


ok is already assured by || !ok

Shouldn't this be !strings.EqualFold?

* Sleep 5s before replying * Refactor server_test.go: remove redundant tests (which are already tested in directory_test.go)

arlolra · 2016-08-27T16:39:31Z

bots/twitterbot.go

+		request, ok := req.Request.(*p.RegistrationRequest)
+		if req.Type != p.RegistrationType || !ok ||
+			// issue: https://github.com/coniks-sys/coniks-go/issues/30
+			!strings.EqualFold(strings.ToLower(username)+"@twitter", request.Username) {


We need to patch Tor Messenger to remove https://github.com/mozilla/releases-comm-central/blob/master/chat/protocols/twitter/twitter.js#L471-L474 otherwise handles with underscores will never match.

Valid chars are [a-z0-9_]{1,15} https://support.twitter.com/articles/101299#error

Probably need to audit the other normalization functions as well to see what nastiness they're doing.

Also, "handle@twitter" seems ok but, for irc, we're going to end up with "handle@irc.net.work@irc". Should we pick another char?

We need to patch Tor Messenger to remove

Here's a quick guide in case you're excited about doing that,

clone https://github.com/mozilla/releases-comm-central

commit your changes and git format-patch

you can also do that with mercurial since you've been building your own Instantbird hg export > something.patch

clone https://gitweb.torproject.org/tor-messenger-build.git (I like this as origin)

optionally, git remote add github https://github.com/TheTorProject/tor-messenger-build

add the patch in https://github.com/TheTorProject/tor-messenger-build/tree/master/projects/instantbird

add the filename to the input_files https://github.com/TheTorProject/tor-messenger-build/blob/master/projects/instantbird/config#L54

commit that change

open a pull on github

or, git format-patch that one, and create a new ticket https://trac.torproject.org/projects/tor/newticket with the patch attached

Here's a quick guide in case you're excited about doing that,

Actually, nevermind, some of this is bad advice. The patch needs to apply to THUNDERBIRD_45_2_0_RELEASE in https://hg.mozilla.org/releases/comm-esr45/

For that you'll need a dev environment, https://gitweb.torproject.org/tor-messenger-build.git/tree/README

I should just set you up with an account on our build server.

The patch needs to apply to THUNDERBIRD_45_2_0_RELEASE in https://hg.mozilla.org/releases/comm-esr45/

In bug 955420 thread (https://bugzilla.mozilla.org/show_bug.cgi?id=955420), they said that the current twitter's normalize is for backwards compatibility. As I understand, the backwards compatibility is related to the logs. So I think this patch needs to apply to Tor Messenger?

Btw, normalize: aString => aString.toLowerCase(), seems to be correct and normalize: aString => aString.replace(/[^a-z0-9_]/gi, "").toLowerCase(), is also correct, right?

Also, "handle@twitter" seems ok but, for irc, we're going to end up with "handle@irc.net.work@irc". Should we pick another char?

Do you have any proposal for this convention? I think "handle@irc.net.work@irc" is fine for now.

As I understand, the backwards compatibility is related to the logs. So I think this patch needs to apply to Tor Messenger?

Yup

Btw, normalize: aString => aString.toLowerCase(), seems to be correct and normalize: aString => aString.replace(/[^a-z0-9_]/gi, "").toLowerCase(), is also correct, right?

Yup. It's best to just remove the override and let it fallback to the default in jsProtoHelper.

Do you have any proposal for this convention?

Not really

I think "handle@irc.net.work@irc" is fine for now.

Ok

I did this in https://gitweb.torproject.org/tor-messenger-build.git/commit/?id=877c03082dadf7e503da5acedb2c88f5e7899eff because we're about to make a release tomorrow.

Do you have any proposal for this convention? I think "handle@irc.net.work@irc" is fine for now.

I was thinking we could use a format "handle@service", which would look something like [email protected] and [email protected]. Would this work? Is there a reason CONIKS needs to record the protocol?

I think the protocol is useful during registration and after that could be useful if you want to use the same handle for different protocols (and with different public-keys).

Is there a reason CONIKS needs to record the protocol?

I'm not sure, but see https://trac.torproject.org/projects/tor/ticket/17461

Some networks let you use "handle@service", and SRV records disambiguate the protocol.

We could maybe handle this with #17 though. Let's continue in #30

Some networks let you use "handle@service", and SRV records disambiguate the protocol.

I see. Since CONIKS should be protocol agnostic, I figured there was no functional benefit to disambiguating the protocol.

I think the protocol is useful during registration and after that could be useful if you want to use the same handle for different protocols (and with different public-keys).

In this case, I think "handle@protocol1" and "handle@protocol2" should be treated as separate users, though, because CONIKS uses per-service namespaces. But maybe this isn't the best design for standalone key servers that manage keys for multiple services. Let's definitely continue this in #30.

arlolra · 2016-08-30T15:58:34Z

bots/coniksbot/cmd/run.go

+	},
+}
+
+func init() {


Can you make this like https://github.com/coniks-sys/coniks-go/blob/master/keyserver/coniksserver/cmd/run.go#L35

with a pid and dir removed.

arlolra · 2016-08-30T19:10:44Z

bots/twitterbot.go

+	}
+
+	// Notify if the CONIKS key server is down
+	if !testCONIKSConnection(conf.CONIKSAddress) {


I should probably just test, but what happens on the keyserver side here? Just ignores the close?

The keyserver has some error since there is no buffer for it to read. Yup, I think it can ignore the close.
I'm finding a nicer way for this.

Can we just do it?

if _, err := os.Stat(conf.CONIKSAddress); os.IsNotExist(err) { return nil, fmt.Errorf("CONIKS Key Server is down") }

Ya, stat seems less intrusive. Let's do that.

Ultimately, we may want to reconsider what I said in #75. Maybe opening a long-lived connection at the start with the keyserver and doing some "first byte sent indicates how many more bytes to read" protocol is the way to go.

Opening a issue to discuss that may be good.

We could use the same protocol for the public connections but close them after responding, rather than a loop.

Anyways, to discuss ...

I opened #77 for dicussions.
/cc @liamsi @masomel

arlolra · 2016-08-31T14:13:28Z

Can we merge this pull?

LGTM

liamsi · 2016-08-31T15:17:24Z

Can we merge this pull?

LGTM

Besides some nitpicking (which you may ignore): LGTM, too.

masomel · 2016-08-31T15:56:36Z

LGTM, too!

vqhuy · 2016-08-31T16:01:14Z

Merged in 9007547.

vqhuy force-pushed the registration-bots branch 4 times, most recently from a866afd to 1e100eb Compare July 29, 2016 11:34

arlolra changed the title ~~Registration bots~~ [WIP] Registration bots Jul 29, 2016

masomel force-pushed the master branch 3 times, most recently from 25a6925 to 2f704cf Compare July 29, 2016 20:03

vqhuy force-pushed the registration-bots branch 2 times, most recently from 40e5d1b to db61ada Compare July 30, 2016 14:31

vqhuy force-pushed the master branch 2 times, most recently from 4ccce23 to 6f72c64 Compare August 2, 2016 21:27

vqhuy force-pushed the registration-bots branch 4 times, most recently from 6658b60 to 1040d32 Compare August 5, 2016 08:43

vqhuy changed the title ~~[WIP] Registration bots~~ Registration bots Aug 5, 2016

vqhuy self-assigned this Aug 5, 2016

vqhuy force-pushed the registration-bots branch 6 times, most recently from fa7f976 to 7e68e9d Compare August 7, 2016 14:36

liamsi reviewed Aug 8, 2016
View reviewed changes

vqhuy force-pushed the registration-bots branch 3 times, most recently from a20412d to be4e068 Compare August 23, 2016 04:56

arlolra reviewed Aug 27, 2016
View reviewed changes

arlolra force-pushed the registration-bots branch from c2e9d08 to 3a20ee8 Compare August 27, 2016 14:58

arlolra reviewed Aug 27, 2016
View reviewed changes

Bot as a service:

c89c5d5

* Sleep 5s before replying * Refactor server_test.go: remove redundant tests (which are already tested in directory_test.go)

vqhuy force-pushed the registration-bots branch from 3a20ee8 to c89c5d5 Compare August 27, 2016 15:50

arlolra reviewed Aug 27, 2016
View reviewed changes

vqhuy mentioned this pull request Aug 28, 2016

Logging #45

Closed

arlolra reviewed Aug 30, 2016
View reviewed changes

vqhuy force-pushed the registration-bots branch from 15965bf to 46c4472 Compare August 30, 2016 17:18

arlolra reviewed Aug 30, 2016
View reviewed changes

vqhuy mentioned this pull request Aug 31, 2016

Protocol for connection layer #77

Open

Notify if the key server is down or the access token is bogus

5aec569

vqhuy force-pushed the registration-bots branch from 46c4472 to 5aec569 Compare August 31, 2016 07:48

vqhuy force-pushed the registration-bots branch from c943ff0 to 5aec569 Compare August 31, 2016 15:32

make ?CONIKS? a (not exported) constant

7bf9ca7

vqhuy closed this Aug 31, 2016

vqhuy deleted the registration-bots branch August 31, 2016 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Registration bots #33

Registration bots #33

vqhuy commented Jul 24, 2016 •

edited

Loading

liamsi Aug 8, 2016

vqhuy Aug 8, 2016

masomel Aug 8, 2016

vqhuy Aug 9, 2016

masomel Aug 9, 2016

vqhuy Aug 9, 2016 •

edited

Loading

vqhuy Aug 18, 2016

arlolra Aug 27, 2016 •

edited

Loading

vqhuy Aug 27, 2016 •

edited

Loading

arlolra Aug 27, 2016

arlolra Aug 27, 2016 •

edited

Loading

arlolra Aug 27, 2016

arlolra Aug 27, 2016

arlolra Aug 27, 2016 •

edited

Loading

vqhuy Aug 27, 2016 •

edited

Loading

arlolra Aug 27, 2016

arlolra Aug 29, 2016

masomel Aug 29, 2016

liamsi Aug 29, 2016

arlolra Aug 29, 2016

masomel Aug 29, 2016 •

edited

Loading

arlolra Aug 30, 2016

arlolra Aug 30, 2016

vqhuy Aug 31, 2016 •

edited

Loading

vqhuy Aug 31, 2016

arlolra Aug 31, 2016

vqhuy Aug 31, 2016

arlolra commented Aug 31, 2016

liamsi commented Aug 31, 2016

masomel commented Aug 31, 2016

vqhuy commented Aug 31, 2016

Registration bots #33

Registration bots #33

Conversation

vqhuy commented Jul 24, 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 Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra Aug 27, 2016 • edited Loading

Choose a reason for hiding this comment

vqhuy Aug 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra Aug 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

arlolra Aug 27, 2016 • edited Loading

Choose a reason for hiding this comment

vqhuy Aug 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

masomel Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Aug 31, 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 commented Aug 31, 2016

liamsi commented Aug 31, 2016

masomel commented Aug 31, 2016

vqhuy commented Aug 31, 2016

vqhuy commented Jul 24, 2016 •

edited

Loading

vqhuy Aug 9, 2016 •

edited

Loading

arlolra Aug 27, 2016 •

edited

Loading

vqhuy Aug 27, 2016 •

edited

Loading

arlolra Aug 27, 2016 •

edited

Loading

arlolra Aug 27, 2016 •

edited

Loading

vqhuy Aug 27, 2016 •

edited

Loading

masomel Aug 29, 2016 •

edited

Loading

vqhuy Aug 31, 2016 •

edited

Loading