New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

listen +port prevents bot connections #754

Closed
vanosg opened this Issue Nov 3, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@vanosg
Member

vanosg commented Nov 3, 2018

Placeholder to troubleshoot this issue from discussion in #552 .

A bot listening with +port isn't sending a STARTTLS.
Instead, the link fails and the initiating bot sends/receives:

[06:42:15] [m->testbot] goodbot
[06:42:15] [m<-testbot] ��
[06:42:15] [m<-testbot] 
[06:42:15] [m<-testbot] Nickname.
[06:42:15] Refused telnet@scoobalooba (invalid handle: goodbot)

and the connection fails.

@michaelortmann

This comment has been minimized.

Member

michaelortmann commented Nov 4, 2018

Are you sure, the listener is the problem or the right place to fix? In case #552 the caller was not doing STARTTLS, if it did, it would probably succeed. There is already theory of a patch in #552, and a small example demostrating that fix could work. That fix opened a new problem that must be fixed, a fallback from STARTTLS to PLAIN if the connection failed. There is also a theory of a patch in #552 for this addional problem.

You can find the fix and the rest of the story in #755.

@michaelortmann

This comment has been minimized.

Member

michaelortmann commented Nov 5, 2018

Its interesting that in some cases the calling bot does start the handshake and in other cases the called bot sends starttls. Maybe there are cases that both is happening. Something doesnt look right here. I realy wasnt expecting a called bot to send starttls if i didnt initialize it. This could lead to bigger problems like in #552 where we want to force into non ssl link. We dont want and dont need the called bot to send unwanted starttls!

Part of the problem here is also that we speak of 2 thinks and think in different ways, when we take into account the called bot could send starttls. i really didn't think of this possibility until i was forced to when i had to look deeper into #552.

@vanosg

This comment has been minimized.

Member

vanosg commented Nov 5, 2018

in tcldcc.c, when establishing a listen port:

#ifdef TLS
  if (*argv[1] == '+')
    dcc[idx].ssl = 1;

then, when receiving an incoming connection:
dcc.c:

    if (!dcc[idx].ssl) {
      dprintf(idx, "starttls\n");

by setting ssl to 1, starttls is not set.

@michaelortmann

This comment has been minimized.

Member

michaelortmann commented Nov 5, 2018

If we patch the line you just found, it would probably fix a problem for patched bots, but it wont fix it for unpatched 1.8.3 that have listen +PORT set. My patch also covers this problem, it doesnt need the starttls of the called bot to ssl link.

@Cizzle Cizzle added this to the v1.8.4 milestone Nov 26, 2018

@vanosg vanosg closed this in #755 Dec 7, 2018

vanosg added a commit that referenced this issue Dec 7, 2018

Improve plain-to-TLS botlinks. Closes #754
Found by: michaelortmann
Patch by: Cizzle, michaelortmann
Fixes: #754


(First entry is port in botaddr on leaf; second is port appearance in listen on hub)

port -> port = plain leaf connection to hub; hub sent starttls, TLS SUCCESS
port -> +port = plain leaf connection to hub; hub attempted TLS read, failed; leaf re-tried with TLS, TLS SUCCESS
+port -> +port = TLS connection to hub, hub attempted TLS read; TLS SUCCESS
+port -> port = TLS connection to hub, failed; LINK FAIL (not a regression though, this was previous behavior)

We can't seem to find a reliable, compatible method where +port->port works. After MUCH discussion, this is being merged as "best we can do for now" and intend (as of now) to switch to plain-only and TLS-only sockets in 1.9. Yes, we all agree it's not ideal, but the best we can get to at this point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment