Net fixes #253

hean01-cendio · 2018-03-26T12:55:42Z

Commit e94b384:

Change that DNS resolve only occurs once for server name provided by command line and for server name provided by server upon redirection. This fixes problem with reconnect triggered by dynamic session resize or network error, and DNS resolves different addresses due to round robin configuration.
This changes does also fix that regarding of what you enter on command line, CredSSP will use resolved FQDN for service principle name.

Commit 7d6466f:

Fixes the code that handles what should be done after rdp main loop exists. There are many variables that have impact on the decision. The code was refactored for clarity. This commit also adds a workaround for a difference between Windows 2008 Server and earlier and newer which fixes a problem with a reconnect triggered if one hit ESC on login screen.

CendioOssman · 2018-03-27T07:33:03Z

cssp.c


 	output.value = malloc(size);
-	snprintf(output.value, size, "%s@%s", service_name, server);
+	snprintf(output.value, size, "%s@%s", service_name, fqdn);


Won't this break systems where you don't have forward and reverse lookups in sync? AI_CANONNAME is probably a better idea, possibly with getnameinfo() as a fallback for ip addresses (if `AI_CANONNAME' doesn't do reverse lookup for you).

As I understand and have verified, AI_CANONNAME would be the resolved name for the passed hostname to getaddrinfo(), this name can be a RDS farm name with round robin and this is not allowed to be used with a SPN which needs to be the actual hostname FQDN. So we can't use that.

Another problem exists, it resolves the first item in list and assume that this is the host connected to. Should be the entry int the list that a successful connection was made to. In most cases this will work, except when the first entry from a round robin is down and second one is used for connection.

Shouldn't you add the farm SPN to each server in that case? I can find some people referring to using setspn, but this Microsoft blog seem to indicate that the connection broken can keep track of a common Kerberos identity:

https://cloudblogs.microsoft.com/enterprisemobility/2009/05/20/creating-kerberos-identity-for-rd-session-host-farms-part-i-using-the-remote-desktop-services-provider-for-windows-powershell/

That was a feature for only Windows 2008 that is deprecated. See first response to that blog post. When deploying a RDS session host a SPN is automatically created for the host which makes our approach to work out of the box against both 2008 and later.

Alright. And no issues with the security? Doing a reverse lookup for normal Kerberos makes it vulnerable to spoofing, so the same might be true here?

AI_CANONNAME seems to have issues as well, though: :/
http://k5wiki.kerberos.org/wiki/Projects/Trust_KDC-local_name_resolution
http://mailman.mit.edu/pipermail/kerberos/2011-July/017313.html

CendioOssman · 2018-03-27T07:34:27Z

proto.h

@@ -19,6 +19,8 @@
 #ifndef RDESKTOP_PROTO_H
 #define RDESKTOP_PROTO_H

+#include <netdb.h>


A small optimisation is to just declare struct addrinfo; here. You don't need the full header here.

Will make it a forward declaration

CendioOssman · 2018-03-27T07:36:56Z

tcp.c

 		{
-			if (connect(g_sock, res->ai_addr, res->ai_addrlen) == 0)
+			/* try connect to server */
+			res = connect(g_sock, pai->ai_addr, pai->ai_addrlen);


The logging from the previous version was lost here. That would be nice to have when debugging.

I'll move the logging for a successful connection into the loop

CendioOssman · 2018-03-27T07:38:16Z

tcp.c

 		return False;
 	}

-#endif /* IPv6 */
+	getnameinfo(pai->ai_addr,  pai->ai_addrlen, hostname, sizeof(hostname), NULL, 0, NI_NUMERICHOST);
+	logger(Core, Debug, "Connected to %s", hostname);


I don't see it storing the result anywhere. It needs to remember which address it actually used or we'll get problems with reconnects after redirects.

I will fix that, this is also needed for CredSSP to resolve the correct name for the connection in case of other then first entry is used.

CendioOssman · 2018-03-27T07:39:33Z

tcp.c

+	hints.ai_flags = AI_NUMERICSERV | AI_CANONNAME;
+	hints.ai_socktype = SOCK_STREAM;
+
+	res = getaddrinfo(address, service, &hints, result);


Where do we free this? Or do we let it leak on each resolve?

I left it out because there will theoretically only be two allocations for a instance lifetime, one for commandline servername and one for redirection, both done in main()

Only a problem if you want to run valgrind then. :)

Major change in this commit is to resolve address given via command line as early as possible and only once during an instance lifetime of rdesktop. This change fixes the problems with network error or pending resize reconnect (Windows 2008) which could resolve to another IP if DNS is configured for round robin and with that, network connect cookie is invalidated. I also took the liberty to remove IPv6 from being a compile time options and make it always used. This change also fixed problems with CredSSP where a FQDN was forced to be entered as servername via command line to work as expected. Now it would use the getnameinfo() to get FQDN for building a complete service name.

We have seen different behaviour between versions of Windows RDP servers how a connection should be closed and rdesktop should exit. Windows 2008 server and earlier versions sets an error info value of 0 and then sends deactivate PDU. Later versions sends a error info of vlaue 12 (User initiated logoff) but does not send deactivate PDU. A work around was added to translate this case for Windows 2008 and earlier to newer aporach to get proper handling. This prevents reconnect loop introduced when hitting ESC or wait for timeout at logon screen against 2008 server or earlier. This commit also fixes a problem where a reconnect loop was triggerd even if no required 'auto-reconnect cookie' is received from the server.

CendioOssman · 2018-03-27T13:19:49Z

tcp.c

+			{
+				pai = NULL;
+				break;
+			}


Don't we want to keep this if we're trying to reconnect after a network error? We might get a few failed connect() before we get back in touch with the server.

pai is local variable and only used to instrument a connect failure further down, whats important here is the connection_pai which is the pointer to the connection to be made which only updates on successful connection.

moobyfr · 2018-03-28T14:05:58Z

Fixes #197

trentasis · 2018-04-26T20:24:20Z

Interesting pr, when will be merged?
Thanks

derfian · 2018-04-30T07:45:22Z

@trentasis I believe there were still somes issues with this PR, so #255 was created to deal with the immediate problems. I don't fully recall the differences any longer, maybe @hean01-cendio or @CendioOssman could clarify if there's anything left of value that wasn't addressed by #255.

We should probably rename or close this PR depending on what's left.

CendioOssman · 2018-05-02T09:20:59Z

The part that is left here is some work on improving hostname handling in CredSSP. But it might be best to break out those parts to a new PR.

hean01-cendio requested review from derfian, CendioOssman and uglym8 March 26, 2018 12:55

hean01-cendio mentioned this pull request Mar 26, 2018

CredSSP not fully working #197

Open

CendioOssman requested changes Mar 27, 2018

View reviewed changes

hean01-cendio added 2 commits March 27, 2018 14:15

hean01-cendio force-pushed the net_fixes branch from 7d6466f to 6b0ce30 Compare March 27, 2018 12:19

CendioOssman reviewed Mar 27, 2018

View reviewed changes

CendioOssman mentioned this pull request Mar 28, 2018

Fix reconnect issues #255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Net fixes #253

Net fixes #253

hean01-cendio commented Mar 26, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

hean01-cendio Mar 27, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

CendioOssman Mar 27, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

CendioOssman Mar 27, 2018

CendioOssman Mar 27, 2018

hean01-cendio Mar 27, 2018

moobyfr commented Mar 28, 2018

trentasis commented Apr 26, 2018

derfian commented Apr 30, 2018

CendioOssman commented May 2, 2018

Net fixes #253

Are you sure you want to change the base?

Net fixes #253

Conversation

hean01-cendio commented Mar 26, 2018

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

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

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

moobyfr commented Mar 28, 2018

trentasis commented Apr 26, 2018

derfian commented Apr 30, 2018

CendioOssman commented May 2, 2018