Xpra: Ticket #2125: let a server register with a proxy

If a server has the ability to register on a proxy, the proxy can become aware of servers where discovery via bonjour is not possible.

Furthermore, this allows hole punching in the servers firewall if the server is keeping the connection to the proxy alive. This allows to have client and server behind a NAT and only needs a proxy exposed.



Sun, 27 Jan 2019 15:17:01 GMT - Antoine Martin: owner, milestone changed

How would a client identify which server it wants to connect to?

The standard way of doing this at the moment is using sqlite auth (#1488), which you can already use today as long as the server is not behind NAT: the servers can add themselves to the sqlite auth database used by the proxy to locate sessions for its users.

To workaround the NAT problem, the servers would need to connect to the proxy instead of the other way around. The connection would have to be authenticated and then kept alive until a client takes over. This is quite similar to #1022.


Fri, 08 Feb 2019 05:57:05 GMT - Antoine Martin: owner, status, component, summary changed

Add a new command line option so that servers can advertise themselves to remote proxy servers. This would complement wiki/MulticastDNS and make it easier to deploy proxy servers. Can also be used for easy NAT traversal: both server and client can be behind NAT.

Related tickets:

ie:

xpra start --start=xterm \
    --register=tcp://proxyusername:proxypassword@proxyip:proxyport/SESSIONID?\
    username=optionalclientloginusername&password=optionalclientloginpassword

Issues:


Mon, 04 Mar 2019 23:03:55 GMT - brief:

I tried to do changes regarding this ticket, but I got stuck after implementing the command line options.

I suggest using one command line option for the server to register himself at a proxy and one command line option for the proxy to accept registrations.

The option for the server could be like yours above. Multiple --register options could be accepted, each with a list of servers. Connections are made to a random not failing entry in each list. Thus multiple proxies with failover configurations can be configured.

The option for the proxy should include the protocol to listen on.

Should there be a new table containing optionalclientloginusernames and passwords with groups matched to users group?


Tue, 05 Mar 2019 02:02:14 GMT - Antoine Martin:

I tried to do changes regarding this ticket, but I got stuck after implementing the command line options.

Where is your code?

The option for the proxy should include the protocol to listen on.

I think this should be done as per #1160, using chained authentication modules: the last module in the chain would do the registration.

Should there be a new table containing optionalclientloginusernames and passwords with groups matched to users group?

Not sure I understand, I don't think anything should be tied to unix user groups. The solution should be generic.


Tue, 05 Mar 2019 19:13:54 GMT - brief: attachment set


Tue, 05 Mar 2019 19:16:35 GMT - brief: attachment set


Tue, 05 Mar 2019 19:24:39 GMT - brief:

I attached a patch regarding the command line options. The cmd options are not in the format you mentioned but in the old one.

I also attached a patch what I tried with a new paket handler, but I certainly sure this is not the right way to do it.

Regarding optionalclientloginusernames: I understood that a server willing to register at a proxy can provide credentials (optionalclientloginusernames, optionalclientloginpassword). I wanted to ask if a new table in db/... is needed (e.g. server_users) to store these combinations. Furthermore I thought about the idea, to be able to indicate groups at server_users to be able to present users a list of servers matching by group.


Wed, 06 Mar 2019 05:35:38 GMT - Antoine Martin: owner, status changed


Mon, 08 Apr 2019 08:59:48 GMT - Antoine Martin:

See also #2261


Tue, 14 May 2019 17:42:25 GMT - Antoine Martin: owner, status changed

Here's my plan - feel free to comment:

Notes:


Thu, 16 May 2019 05:00:22 GMT - Antoine Martin: cc set

(CC was removed by mistake)


Sun, 08 Sep 2019 17:26:57 GMT - Antoine Martin:

Some useful socket functions have been moved:


Tue, 10 Sep 2019 14:57:06 GMT - Antoine Martin:

More network related updates that can help here:


Fri, 13 Sep 2019 15:45:52 GMT - Antoine Martin: milestone changed

Too late for 3.0, will deal with this after #1160 so we can re-use the same bind-XXX syntax and have different capabilities on different ports.


Wed, 01 Apr 2020 01:28:43 GMT - brief:

Combining seems like a good idea.

The server could send its desired display name, e.g. "Facility 7, Room 5" and forwarded application. Furthermore, the session is tied to a proxy user (from e.g. server_users, see above(and could be shared with a proxy_group)). Clients requesting sessions from the proxy are only getting their accessible sessions, thus the displayname has limited scope.

To balance register/list requests? Or to balance connections between client and server in TURN mode?

A session has to be selected choosen from prior requested list of accessible sessions.

Allow on all specified authenticators?

Take a xpraUri[][] as proxies. Server will connect to one of each inner in all outer.

Can they be stored using the Authenticator?

Are mdns broadcast receivable via client packets?

Is this still a problem?

I would consider to specify a list of proxies where keep-alive packet are send to (including a token). Registering using credentials should be a one time (interactive) action to get a token.


Thu, 02 Apr 2020 05:04:34 GMT - brief: attachment set


Thu, 02 Apr 2020 05:15:51 GMT - brief:

Replying to Antoine Martin:

How much does the patch above hurt your eyes? It seems like an option to divide traffic in the proxy_auth and let the authenticator handle DB/memory specific things. I mocked it up in the proxy_auth function, this could also go in a different class.

Is there any way I can create a branch? Or is uploading patches the way to go?


Fri, 08 May 2020 14:43:40 GMT - Antoine Martin: milestone changed

How much does the patch above hurt your eyes?

The patch is not too bad, but I'm out of time.

I've done some work on generalizing bind command line options in #2406. This needs extending.


Wed, 30 Sep 2020 01:04:48 GMT - brief: attachment set


Wed, 30 Sep 2020 01:15:47 GMT - brief:

I rewrote the patch to use a hello request type for register and update requests.

Start a proxy with register/update support:

proxy --tcp-auth=sqlite:filename=userlist.sdb --proxy-sessions=register --bind-tcp=127.0.0.1:10009

Add a session at the proxy which returns a token to update the session:

xpra register tcp://username:password@127.0.0.1:10009/

Start a server and update the session at the proxy:

xpra start --start=xterm --bind-tcp=127.0.0.1:10010 --proxies=tcp://127.0.0.1:10009/update_token=SESSION_TOKEN

I have only tested it with sqlite, it only works with sqlite atm (scripts/server.py:427). It seems verify_auth in the core has to handle the update request and call auth_verified (server_core.py:1670).

Perhaps you can help me with the following:


Wed, 30 Sep 2020 10:08:51 GMT - Antoine Martin: owner, status changed

Can you explain why you need to register the session first? Doesn't it make sense to just let the server register with the proxy without needing to register first? What's the benefit?

This registration ability should be an attribute of the authentication modules - see #2424. So you could enable this on a specific port (DMZ), and not on another (WAN), ie:

--bind-tcp=127.0.0.1:10009,auth=file:filename=server-auth,auth=sqlite:filename=userlist.sdb:register=1 \
--bind-tcp=0.0.0.0:10000,auth=sqlite:filename=userlist.sdb:register=0

In this example: the registration requires password authentication using file auth before you can add entries to the sqlite db, clients connect to port 10000 and only need to authenticate with sqlite. The registration would only be implemented in the generic sql backend for now - the other modules would just fail when calling proxy_register / proxy_update.


Not essential, but it would be nice of the server maintained the connection to the proxy:


I've merged the 2 whitespace changes. I think that most of the changes to sqlauthbase.py and sqlite_auth.py would be worth merging without the rest as it would reduce the diff when reviewing. (just without the token / session_add stuff for now)

As for the remainder of your questions, the line numbers don't match trunk?


Fri, 02 Oct 2020 17:00:05 GMT - brief:

Replying to Antoine Martin:

Can you explain why you need to register the session first?

For authenticated updates (server -> proxy), an authentication mechanism must be present. Since servers are mostly non-interactive (daemon), any credentials need to be stored in the config file or as parameter. Storing a token which can only update one session at the proxy prevents storing usernames/passwords and limits the abilities of stolen tokens.

Doesn't it make sense to just let the server register with the proxy without needing to register first? What's the benefit?

With the DB schema from the patch, one user can have multiple sessions. Only with username/password, no session to update can be identified. If the identifier can be chosen freely by the client, uniqueness is not guaranteed. Registering is a one-time action per server (upon installing/setting up xpra), updates are sent periodically while the server is running.

This registration ability should be an attribute of the authentication modules - see #2424. So you could enable this on a specific port (DMZ), and not on another (WAN), ie:

--bind-tcp=127.0.0.1:10009,auth=file:filename=server-auth,auth=sqlite:filename=userlist.sdb:register=1 \
--bind-tcp=0.0.0.0:10000,auth=sqlite:filename=userlist.sdb:register=0

In this example: the registration requires password authentication using file auth before you can add entries to the sqlite db, clients connect to port 10000 and only need to authenticate with sqlite. The registration would only be implemented in the generic sql backend for now - the other modules would just fail when calling proxy_register / proxy_update.

So there is a need for another option which enables registering (and updating) selectively by auth backend.

Not essential, but it would be nice of the server maintained the connection to the proxy:

I agree that the server should keep the connection alive. But the proxy should not delete the session because it is possibly updated later. I thought to fill the "updated" field with a timestamp of the last update, but a flag "currently connected" is also possible. I reused the sessions table to also store "registered servers".

As for the remainder of your questions, the line numbers don't match trunk?

The line numbers match revision 27551 with 8d6c8c863e1a97162f1c49598ffeb4cfde96e9c5.patch​ applied.


Fri, 09 Oct 2020 08:53:55 GMT - Antoine Martin:

For authenticated updates (server -> proxy), an authentication mechanism must be present.

Yes, that's why I proposed: --bind-tcp=127.0.0.1:10009,auth=file:filename=server-auth,auth=sqlite:filename=userlist.sdb:register=1 for the socket which is facing the servers. I used file as an example but you could use sqlite for that too. Using tokens makes things more complicated. We have multi-layer authentication support, let's use it.

Storing a token which can only update one session at the proxy prevents storing usernames/passwords and limits the abilities of stolen tokens.

The only gain here is the ability to restrict to one session, not sure how useful that is in practice.

With the DB schema from the patch, one user can have multiple sessions. Only with username/password, no session to update can be identified. If the identifier can be chosen freely by the client, uniqueness is not guaranteed.

OK, or you could just use different username + password combinations for each session.

Registering is a one-time action per server (upon installing/setting up xpra), updates are sent periodically while the server is running.

Hmm. Then if the server drops off, the session will be unreachable. And the proxy has to make a new connection to the server.

The registration would only be implemented in the generic sql backend for now - the other modules would just fail when calling proxy_register / proxy_update.

So there is a need for another option which enables registering (and updating) selectively by auth backend.

Yes. It is much safer that way.

The line numbers match revision 27551 with 8d6c8c863e1a97162f1c49598ffeb4cfde96e9c5.patch​ applied.

Can you please post easy to parse links, maybe just code extracts. And please submit things separately: most of the DatabaseBaseQueries could be merged without the rest. (though I do wonder the point of having QueriesBase - doubles the size of the code without any visible benefits)


Wed, 28 Oct 2020 22:02:21 GMT - brief: attachment set


Wed, 28 Oct 2020 22:02:45 GMT - brief:

Replying to Antoine Martin:

And please submit things separately: most of the DatabaseBaseQueries could be merged without the rest. (though I do wonder the point of having QueriesBase - doubles the size of the code without any visible benefits)

QueriesBase only ties a connection between the sql variants so that I (the developer) do not forget to implement used sql commands in all variants.

Note that I also added a db.commit() since without this, multiple sqlite commands failed on me at the second command.


Wed, 28 Oct 2020 23:42:49 GMT - brief:

Replying to Antoine Martin:

Using tokens makes things more complicated. We have multi-layer authentication support, let's use it. The only gain here is the ability to restrict to one session, not sure how useful that is in practice.

IMHO security is another big gain. If credentials are used to update sessions, they are stored in plain text in the servers config (e.g. to be used while starting automatically on boot). These credentials are designed to be used as a login. By introducing a conceptual difference of credentials and tokens, stored values do not need to be credentials. A token is saved and used to update the session while credentials are not saved and used only while interactively registering a server.

Optionally, if the user decides to store credentials, the server could itself register at the proxy atomatically and de-register itself upon shutdown (dynamically). The benefit is a clean sessions table but it makes e.g. multiple user more complex.

OK, or you could just use different username + password combinations for each session.

I like the idea of splitting users and sessions since it enables that a user can access multiple sessions. An existing user is also required to register a session dynamically. Am I overlooking a disadvantage?


Thu, 29 Oct 2020 02:06:51 GMT - brief: attachment set


Thu, 29 Oct 2020 02:09:23 GMT - brief:

Replying to Antoine Martin:

--bind-tcp=12[...],auth=sqlite:filename=userlist.sdb:register=1

This seems to require at least the patch above. I would also add update=1.


Thu, 29 Oct 2020 06:15:40 GMT - brief:

Replying to brief:

Perhaps you can help me with the following:

How do I get options for a client request (line 11) to not reuse the ones from server start? What should they contain except the token? Is this method of creating a client generally correct?

 1 def do_run_mode(script_file, error_cb, options, args, mode, defaults):
 2     [...]
 3         if not (mode=="proxy" and supports_proxy) and options.proxies:
 4             def update_proxies():
 5
 6                 ips = options.bind_tcp
 7
 8                 for proxy in options.proxies:
 9                     # try to update proxy with token
10
11                     update_options = options
12                     update_display_desc = pick_display(error_cb, update_options, [proxy])
13
14                     from xpra.client.gobject_client_base import UpdateProxyClient
15                     update_app = UpdateProxyClient(update_options, ips)
16
17                     try:
18                         connect_to_server(update_app, update_display_desc, udate_options)
19                     except Exception:
20                         update_app.cleanup()
21                         raise
22
23
24                     do_run_client(update_app)
25             add_when_ready(update_proxies)

How is it possible in verify_auth() in server_core.py to access the display_desc which is added on client requests (main.py:1685: app.display_desc = display_desc)?


Sun, 15 Nov 2020 02:49:49 GMT - brief:

I listed three sql variants with support of tokens and multiple user per sessions.
If the table sessions should only contain available sessions, Variant 3 cannot be used. Furthermore, Variant 2 seems to be simpler and cleaner than Variant 1.

Variant 1: sessions up to date, shared table


                             --------------              -----------
     -----------            | SESSION_USER |            | SESSIONS  |
    | USERS     |            ______________              ___________
     ___________            | session_id   |    <-->    | id        |
|-> | id        |    <-->   | user_id      |            | display   |
|   | user      |            --------------             |           |
|   | password  |                                       |           |
|    -----------                                        |           |
|                                                       |           |
|    ---------------                                    |           |
|   | TOKEN_USER    |            -----------            |           |
|    _______________            | TOKENS    |           |           |
|-> | user_id       |            ___________            | datetime  |
    | token_id      |    <-->   | id        |    <-->   | token_id  |
     ---------------            | value     |            -----------
                                 -----------



Variant 2: sessions up to date, separate tables


                             --------------              -----------
     -----------            | SESSION_USER |            | SESSIONS  |
    | USERS     |            ______________              ___________
     ___________            | session_id   |    <-->    | id        |
|-> | id        |    <-->   | user_id      |            | display   |
|   | user      |            --------------              -----------
|   | password  |
|    -----------                                         -------------------
|                                                       | TOKEN_SESSIONS    |
|    ---------------                                     ___________________
|   | TOKEN_USER    |            -----------            | id                |
|    _______________            | TOKENS    |           |                   |
|-> | user_id       |            ___________            | datetime          |
    | token_id      |    <-->   | id        |    <-->   | token_id          |
     ---------------            | value     |            -------------------
                                 -----------



Variant 3: sessions not up to date


                             --------------              -----------
     -----------            | SESSION_USER |            | SESSIONS  |
    | USERS     |            ______________              ___________
     ___________            | session_id   |    <-->    | id        |
    | id        |    <-->   | user_id      |            | display   |
    | user      |            --------------             | token     |
    | password  |                                       | datetime  |
     -----------                                         -----------




*in normal mode: if a unique "token" (e.g. UUID) already exists at server, it could be used as token. Otherwise, a token can be requested to use persistent user assignments.


Sun, 06 Dec 2020 12:13:23 GMT - brief: attachment set


Sun, 06 Dec 2020 12:13:35 GMT - brief: attachment set


Sun, 06 Dec 2020 12:14:22 GMT - brief: attachment set


Sun, 06 Dec 2020 12:14:32 GMT - brief: attachment set


Sun, 06 Dec 2020 12:26:56 GMT - brief:

I added my current state of work.




This patch should be split up in multiple patches but shows the idea. In an environment with server and client behind different NATs, if the right protocol picks up the packets at the proxy (since there are multiple), a direct connection between server and client is established. In the current code, it stays in this state without activating the 'real' client (screen transmission, ...).


Sun, 06 Dec 2020 12:33:26 GMT - brief: attachment set


Tue, 08 Dec 2020 09:30:18 GMT - Antoine Martin:

It would help to have a description of the changes and the intent.

Review:

All in all, the token stuff adds quite a lot of code and complicates things quite a bit too. Then the proxy still needs to connect back to the server when the client connects, so this doesn't help with NATed servers either..


Fri, 11 Dec 2020 16:06:29 GMT - brief:

Replying to Antoine Martin:

It would help to have a description of the changes and the intent.

The intention is to enable server and client behind NATs.

Its implemented like this (also have a look at the attached ods document):


Review:

get_passwords seems to return multiple passwords so this checks if multiple users are present. perhaps get_passwords should be replaced by get_password.

get_sessions returns an empty array on sqlite in this implementation. this could return none or the datatype in all authenticators could be changed to array.

perhaps the authenticator should decide about sessions and display?

with this change, multiple session are returned on get_info (related to get_sessions)

hole_punching can be enabled/disabled at client, server and authentication module (proxy). hole_punching_authenticator saves the value from the authenticator which is used in proxy_instance:run() to decide if hole punching should start. if hole punching is started, the proxy does not start a connection to the server but reuses a previous "update connection" from the server.

this line can be removed :) I previously used connect_to(disp_desc, opts) instead of preserving the connection. if local_port is set, client requests are send from the local_port. I thought this is useful if no preserved connection from the server is present but the servers firewall still accept packets originating from the proxies ip:port.

I think this is needed on non-windows to reuse sockets with the same port properly.

If reuse is set, socket.SO_REUSEADDR is set. if a port is given, the socket binds to it so that multiple sockets exist on the port. I found no problems using the sockets if only one type of protocol (server/client) is used.

In an environment with server and client behind different NATs, if the right protocol picks up the packets at the proxy (since there are multiple), a direct connection between server and client is established. In the current code, it stays in this state without activating the 'real' client (screen transmission, ...).

With this implementation, sometime an error occurs: unknown or invalid packet type at the proxy server.

While server and client are hole punching, they send status updates to the proxy.

I tested with this network:

     server            router             proxy            router      client
172.20.42.1       172.20.42.3
               192.168.178.42    192.168.178.25    192.168.178.49
                                                         10.0.0.3    10.0.0.1

and this router config based on iptables:

# Delete all
sudo iptables -P INPUT ACCEPT
sudo iptables -P FORWARD ACCEPT
sudo iptables -F
sudo iptables -F -t nat
# Allow traffic from internal network
sudo iptables -A INPUT -i enp0s8 -j ACCEPT
# Allow local
sudo iptables -A INPUT -i lo -j ACCEPT
# Allow ssh traffic from external
sudo iptables -A INPUT -i enp0s3 -p tcp -m tcp --dport 22 -j ACCEPT
# allow local related packets
sudo iptables -A INPUT -i enp0s3 -m state --state RELATED,ESTABLISHED -j ACCEPT
# Drop everything by default
sudo iptables -P INPUT DROP
# NAT
sudo iptables -A FORWARD -i enp0s3 -o enp0s8 -m state --state RELATED,ESTABLISHED -j ACCEPT
sudo iptables -A FORWARD -i enp0s8 -o enp0s3 -s [10.0.0.0/24 or 172.20.42.0/24] -j ACCEPT
sudo iptables -t nat -A POSTROUTING -o enp0s3 -s [10.0.0.0/24 or 172.20.42.0/24] -j MASQUERADE
sudo iptables -P FORWARD DROP

Wed, 13 Jan 2021 03:15:03 GMT - Antoine Martin: milestone changed


Sat, 23 Jan 2021 05:42:52 GMT - migration script:

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/2125