Skip to content
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

Better (any) error message for SSL... #458

Closed
vanosg opened this issue Sep 4, 2017 · 1 comment · Fixed by #645
Closed

Better (any) error message for SSL... #458

vanosg opened this issue Sep 4, 2017 · 1 comment · Fixed by #645
Milestone

Comments

@vanosg
Copy link
Member

vanosg commented Sep 4, 2017

... when a linking bot is not compiled with SSL and fails to connect to an SSL-only port.
Currently, the leaf bot (with no SSL) only sees:

[02:02:13] Linking to bot1 at 172.17.0.3:1123 ...
.[02:02:30] net: attempted socket connection refused: 172.17.0.3
[02:02:30] net: attempted socket connection refused: 172.17.0.3
[02:02:30] net: attempted socket connection refused: 172.17.0.3
[02:02:30] Failed link to bot1.

the hub (with SSL, console +b-d) only sees:

[02:06:18] TLS: failed in: SSLv2/v3 read client hello A.
[02:06:18] TLS: failed in: SSLv2/v3 read client hello A.
<snip>
[02:06:19] TLS: failed in: SSLv2/v3 read client hello A.
[02:06:19] TLS: failed in: SSLv2/v3 read client hello A.
[02:06:20] Lost telnet connection to telnet@172.17.0.2/51613

Maybe at the end of all those failed hellos, we add

Lost telnet connection to telnet@172.17.0.2/51613
Is the bot compiled with SSL?

as the only reason I can think of for that many failed hello's would be no SSL. Or something, but that's a starting point. Probably worth looking at the reverse case (hub no ssl, leaf ssl) too

@michaelortmann
Copy link
Member

michaelortmann commented Sep 24, 2018

1. "only reason I can think of for that many failed hello's would be no SSL"

true, but the connecting client could be a bot OR a user.
telnet to ssl only port, then kill that telnet, will result in same messages.

so we cant display

Is the bot compiled with SSL?

at least the message must be changed.

2. error mesage (ssl bot port)

the code in tls.c:ssl_info() producing the +b message
TLS: failed in: SSLv2/v3 read client hello A.
doesn't seem a good choice for enhancing the error message.

but the code in net.c:sockread() seems:

currently its like:

SSL_read()
if SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE
  no debug / log
else
  debug1("sockread(): SSL error = %s", ERR_error_string(ERR_get_error(), 0));

the case happening here is case SSL_ERROR_SYSCALL and its only handled in the general else branch.

we should intercept SSL_ERROR_SYSCALL and display an error message like i now committed with #645.

Also the general error message above could be enhanced to read "SSL read error" instead of "SSL error".

There are more cases, where we could replace error mesage "SSL error" with a more detailled one, like after SSL_write().

The ssl bot of course doesn't know if its a bot or a user telnet, or whatever, that tries to connect. So it can only display some error message like "Could be..."

3. error mesage (non ssl bot)

the non ssl bot connects to an ssl only port. the ssl bot expects the hello and is sending nothing. so the non ssl bot cant receive anything and is quite blind. it cant find out that the end point is an ssl port.

but we can change this. yes!

of course only, if stealth-telnets isn't activated and/or a new config setting allows for it.

the ssl bot could send a TOKEN over the wire, so that the non ssl bot/user could see its an ssl port and react to it.

bonus: a non-ssl bot could not only inform about the ssl port, but could also instantly drop its attempt to connect. maybe even send another TOKEN to the ssl port to tell the ssl bot, what just happened, so it also could report/act to it.

4. the many "TLS: failed in:"

i would like to see such verbose messages to be moved from a normal output channel to debug() output.

(1) and (2) is fixed with #645
(3) and (4) is for 1.9+

vanosg pushed a commit that referenced this issue Oct 12, 2018
Found by: Geo
Patch by: michaelortmann
Fixes: #458
@vanosg vanosg added this to the v1.8.4 milestone Oct 13, 2018
Cizzle pushed a commit that referenced this issue Oct 18, 2018
Found by: Geo
Patch by: michaelortmann
Fixes: #458
Cizzle pushed a commit that referenced this issue Oct 18, 2018
Found by: Geo
Patch by: michaelortmann
Fixes: #458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants