let a server register with a proxy

[totaam]

Issue migrated from trac ticket # 2125

component: network | priority: major

2019-01-27 14:36:47: brief created the issue

---

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.

[totaam]

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.

[totaam]

Add a new command line option so that servers can advertise themselves to remote proxy servers.
This would complement 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:

• network layer improvements #1590 network tracker ticket
• Extend Xpra proxy for terminal server deployments #675 extend xpra proxy for terminal server deployments

ie:

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

Issues:

• only allow registration on some tcp / ws / wss / ssh sockets?
• support multiple proxies: register with all of them, or the first one, or random, or...
• re-connect to the proxy if the connection drops, maybe add an option to give up and exit if none of the proxies are available for too long
• keep connection to proxy alive - also similar to Xpra Client in Listen Mode #1022: "client listen mode"
• the proxy needs to keep track of those sessions in memory, return them before or after the ones from the client auth module?
• the proxy servers could also listen for mdns broadcasts and expose those servers

[totaam]

2019-03-04 23:03:55: brief commented

---

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?

[totaam]

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.

[totaam]

2019-03-05 19:13:55: brief uploaded file command line options.patch (6.3 KiB)

[totaam]

2019-03-05 19:16:35: brief uploaded file paket handler.patch (3.7 KiB)

[totaam]

2019-03-05 19:24:40: brief commented

---

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.

[totaam]

• there is no authentication on the "register" packets? You would be better off using a hello request option. (see how detach or info requests are handled)
• if using a dedicated packet type, the "register" packet handler should live in the proxy code, not in server core module.
• I don't understand why you're using mod_python here
• the "answer" object is constructed weird and I don't see the code for add_user anywhere
• the register string should be a regular xpra URI (ie for TCP: tcp://username:password@host:port/?extra_args), that way authentication will be handled automatically

[totaam]

See also #2261

[totaam]

Here's my plan - feel free to comment:

• --proxy-sessions=auth,sys,mdns,register so the proxy can be told where to get the list of sessions from (and maybe those options can be combined?):

• auth (default) would still use the auth module so we don't break any existing setups
• sys same as what most auth modules currently do: similar to running xpra list using the same uid supplied by the auth module (usually the user that authenticated)
• mdns without knowing if the server already has clients or not since we can't seem to get TXT record update notifications (see update mdns properties on the fly #2187)
• register get the list of sessions from servers that register with the proxy

• I would like to re-use all the socket code and authentication configuration options for the server registration (so servers can use ssl / ssh / whatever), but ideally without letting the servers have the same privileges as regular authenticated users (ie: no ability to connect to sessions or start new sessions, just registering) - not sure how to do that yet (comment:4 suggested using a new authentication module, but this would be too difficult to tie with other features: keepalive, etc), should this be configured at the socket level (as part of the --bind-XXXX option), or as part of the authentication module? (ie: access=full,register,info,... to specify what the authentication gives access to)
• the sessions can be identified using their uuid, and maybe other attributes? the display string is not a good match as multiple servers running on different systems can use the same display number. uids are not usually portable either (not without things like ldap - which we can't require)
• the server code will need a new option for telling it to register with the proxy, something like: --register=tcp://host:port/?options, it will need options like keepalive and re-connect, we should detect authentication failures and not retry too often in that case
• the proxy can keep these connections open, proxy_auth can be taught about the special "registered" sessions and bypass connect_to

Notes:

• do we want to think about load-balancing?
• how do we select a session easily?
• if more than one is available?

[totaam]

Some useful socket functions have been moved:

• r23736 refactoring: move code to socket_util
• r23737 move socket_util to xpra.net
• r23738 make add_listen_socket more re-usable
• r23740 remove socket_types mapping, use callback argument instead
• r23741 move accept_connection code to socket_util

[totaam]

More network related updates that can help here:

• r23742 + r23756: cosmetic
• r23747 assume socket if path is given as display
• r23748 control channel fix for python3 proxy instances
• r23750 : proxy ping #2408
• r23754 warn if a unix domain socket goes MIA
• r23759 cleanup now stops listening for socket events, fix r23740 (must return True to continue receiving socket events)
• r23760 BUG introduced by r23759, cleanup IO event source only once
• r23761 simplify named-pipe extra "pipe_handle" argument (make it a default arg)
• r23762 : socket-dirs defaults are ignored #2407
• r23763 : proxy instance process message queue cleanup
• r23768 refactor peek_connection to make it reusable
• Xpra Client in Listen Mode #1022 : client listen mode
• r23769 constify

[totaam]

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.

[totaam]

2020-04-01 02:28:43: brief commented

---

• --proxy-sessions=auth,sys,mdns,register so the proxy can be told where to get the list of sessions from (and maybe those options can be combined?):
  Combining seems like a good idea.
  [[BR]]
• the sessions can be identified using their uuid, and maybe other attributes? the display string is not a good match as multiple servers running on different systems can use the same display number. uids are not usually portable either (not without things like ldap - which we can't require)
  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.
  [[BR]]
• do we want to think about load-balancing?
  To balance register/list requests? Or to balance connections between client and server in TURN mode?
  [[BR]]
• how do we select a session easily?
• if more than one is available?
  A session has to be selected choosen from prior requested list of accessible sessions.
  [[BR]]
• only allow registration on some tcp / ws / wss / ssh sockets?
  Allow on all specified authenticators?
  [[BR]]
• support multiple proxies: register with all of them, or the first one, or random, or...
  Take a xpraUri[][] as proxies. Server will connect to one of each inner in all outer.
  [[BR]]
• the proxy needs to keep track of those sessions in memory, return them before or after the ones from the client auth module?
  Can they be stored using the Authenticator?
  [[BR]]
• the proxy servers could also listen for mdns broadcasts and expose those servers
  Are mdns broadcast receivable via client packets?
  [[BR]]
• I would like to re-use all the socket code and authentication configuration options for the server registration (so servers can use ssl / ssh / whatever), but ideally without letting the servers have the same privileges as regular authenticated users (ie: no ability to connect to sessions or start new sessions, just registering) - not sure how to do that yet (comment:4 suggested using a new authentication module, but this would be too difficult to tie with other features: keepalive, etc), should this be configured at the socket level (as part of the --bind-XXXX option), or as part of the authentication module? (ie: access=full,register,info,... to specify what the authentication gives access to)
  Is this still a problem?
  [[BR]]
• the server code will need a new option for telling it to register with the proxy, something like: --register=tcp://host:port/?options, it will need options like keepalive and re-connect, we should detect authentication failures and not retry too often in that case
  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.

[totaam]

2020-04-02 06:04:35: brief uploaded file evolve.patch (15.4 KiB)

[totaam]

2020-04-02 06:15:52: brief commented

---

Replying to [comment:9 Antoine Martin]:

• I would like to re-use all the socket code and authentication configuration options for the server registration (so servers can use ssl / ssh / whatever), but ideally without letting the servers have the same privileges as regular authenticated users (ie: no ability to connect to sessions or start new sessions, just registering) - not sure how to do that yet (comment:4 suggested using a new authentication module, but this would be too difficult to tie with other features: keepalive, etc), should this be configured at the socket level (as part of the --bind-XXXX option), or as part of the authentication module? (ie: access=full,register,info,... to specify what the authentication gives access to)
  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?

[totaam]

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.

[totaam]

2020-09-30 02:04:49: brief uploaded file 8d6c8c863e1a97162f1c49598ffeb4cfde96e9c5.patch (40.8 KiB)

[totaam]

2020-09-30 02:15:47: brief commented

---

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:[email protected]: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:

• How do I get options for a client request in main.py:555? What should they contain except the token?
• Is this method of creating a client generally correct? (main.py:586-603)
• Is it possible to access the request arguments (token) in proxy_server:680?
• Is there a better way of checking if an sql auth is given than in scripts/server.py:427?

[totaam]

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:

• the proxy could automatically remove the session when the server disconnects
• the proxy would not need to connect back to the server when a client claims the session, it could just re-use the connection

---

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?

[totaam]

2020-10-02 18:00:05: brief commented

---

Replying to [comment:18 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.
[[BR]]
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.
[[BR]]

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.
[[BR]]

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

• the proxy could automatically remove the session when the server disconnects
• the proxy would not need to connect back to the server when a client claims the session, it could just re-use the connection

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".
[[BR]]

As for the remainder of your questions, the line numbers don't match trunk?
The line numbers match revision 27551 with 8d6c8c863e1a97162f1c49598ffeb4cfde96e9c5.patch​ applied.

[totaam]

2020-10-09 09:53:56: antoine commented

---

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)

[totaam]

2020-10-28 22:02:22: brief uploaded file db.patch (7.4 KiB)

[totaam]

2020-10-28 22:02:46: brief commented

---

Replying to [comment:20 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.

[totaam]

2020-10-28 23:42:50: brief commented

---

Replying to [comment:20 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.
[[BR]]

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?

[totaam]

2020-10-29 02:06:52: brief uploaded file server_core.py.patch (0.9 KiB)

[totaam]

2020-10-29 02:09:24: brief commented

---

Replying to [comment:20 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.

[totaam]

2020-10-29 06:15:40: brief commented

---

Replying to [comment:17 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)?

[totaam]

2020-11-15 02:49:49: brief commented

---

I listed three sql variants with support of tokens and multiple user per sessions.
[[BR]]
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.
[[BR]]

Variant 1: sessions up to date, shared table

[[BR]]

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

• Client can access sessions connected via SESSION_USER->SESSIONS and TOKEN_USER->TOKENS->SESSIONS
• Sessions are accessible since proxy adds/removes sessions upon server request (with datetime)
• Server request can contain
  • token (session is created/removed)
  • user/password (session is created/removed, only this user can access session*)
• Expiring sessions:
  • static (without token): no
  • dynamic (with token): based on datetime
    [[BR]][[BR]]

Variant 2: sessions up to date, separate tables

[[BR]]

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

• Client can access sessions connected via SESSION_USER->SESSIONS and TOKEN_USER->TOKENS->TOKEN_SESSIONS
• Sessions are as before, token_sessions are accessible since proxy adds/removes sessions upon server request (with datetime)
• Server request can contain
  • token (token_session is created/removed)
  • user/password
    • token mode: token is created
    • normal mode: token_session is created/removed*
• Expiring sessions:
  • static (SESSIONS): no
  • dynamic (TOKEN_SESSIONS): based on datetime
    [[BR]][[BR]]

Variant 3: sessions not up to date

[[BR]]

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

• Client can access sessions connected via SESSION_USER->SESSIONS
• Sessions are not necessarily accessible, sessions without token and datetime represent sessions as before
• Proxy adds/removes sessions upon server request (with datetime)
• Server request can contain
  • token (session is updated)
  • user/password
    • token mode: session is created
    • normal mode: session is created/removed*
• Expiring sessions:
  • static (without datetime and token): never
  • dynamic(token mode) (with token): never
  • dynamic(normal mode) (without token): based on datetime
    [[BR]]

---

[[BR]]
*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.

[totaam]

2020-12-06 12:13:24: brief uploaded file sessions.patch (7.6 KiB)

[totaam]

2020-12-06 12:13:36: brief uploaded file sessions.2.patch (7.6 KiB)

[totaam]

2020-12-06 12:14:22: brief uploaded file socket.patch (15.2 KiB)

[totaam]

2020-12-06 12:14:32: brief uploaded file hp.patch (79.5 KiB)

[totaam]

2020-12-06 12:26:56: brief commented

---

I added my current state of work.
[[BR]]

• sessions.patch has the intention to let authenticators return multiple sessions so a session identifier can be used (sessions.2 is a duplicate).
  [[BR]]
• socket.patch refactors the socket creation.
  [[BR]]
• hp.patch introduces new functionality:
  • ability to register at a proxy
  • update a proxy with current server status
  • initiates tcp hole punching, if enabled on server, client and proxy
    [[BR]]
    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, ...).

[totaam]

2020-12-06 12:33:27: brief uploaded file xpra.ods (14.8 KiB)

[totaam]

2020-12-08 09:30:19: antoine commented

---

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

Review:

• if all authentication modules now return a list of sessions (which makes sense) then I think that self.sessions = [(uid, gid, displays, env_options, session_options)] should become [(uid, gid, display, env_options, session_options) for each display] - this should be a separate change from get_session_by_token and parse_session_datas. AFAICT, this could be merged easily enough for 4.1, including the unit test changes, etc
• assert len(passwords) in 1 - why?
• print("success, found password") - never call print from server code
• why replace if auth_sessions: with if len(auth_sessions) > 0: ?
• # TODO selection session - this is crucial and should be moved to a method that could eventually do more than just fine the matching display
• sessions_info changes - not sure what that does, not looked at it carefully - should be split up as this is unrelated?
• I like the abstract listenSocket class, though I'm not sure I want to deal with the extra abc dependency for packaging (minor detail) - this is probably too late to merge into 4.1, or maybe not..
• the changes to SQLAuthenticator should be split up: first the move to QueriesBase (preferably without abc) without the token bits
• the hole_punch_authenticator needs an explanation
• opts.local_port = client_proto._conn.local[1] - what does this do?
• why do we want / need this?

if hasattr(socket, 'SO_REUSEPORT'):
   listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1)

• adding proxy_sessions and hole_punch in server_core.py is unlikely to fly
• socket_connect(dtype, host, port, reuse=False,local_port=0) - can you explain why we need to re-use the local_port and how it's done?
• again, the hole_punch, hp_local_server_address changes to the proxy instance classes is unlikely to fly
• hp-client-status / hp-server-status - what does this do?

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..

[totaam]

2020-12-11 16:06:30: brief commented

---

Replying to [comment:27 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):
[[BR]]

• Upon running, the server updates the proxy with its local address details. The proxy saves the address the packet is comming from and the local address reported by the server.
• Upon running a client, the local address details are send with the connect request to the proxy.
  • The proxy does not relay the connection to the server but sends back the servers local/remote addresses to the client.
  • The proxy also sends the client addresses to the server using an existing connection originating from the servers update request.
  • The client tries to reach the servers addresses and reports its status to the proxy.
  • The server tries to reach the clients addresses and reports its status to the proxy.
    [[BR]]

Review:

• if all authentication modules now return a list of sessions (which makes sense) then I think that self.sessions = [(uid, gid, displays, env_options, session_options)] should become [(uid, gid, display, env_options, session_options) for each display] - this should be a separate change from get_session_by_token and parse_session_datas. AFAICT, this could be merged easily enough for 4.1, including the unit test changes, etc

• assert len(passwords) in 1 - why?
  get_passwords seems to return multiple passwords so this checks if multiple users are present. perhaps get_passwords should be replaced by get_password.
  [[BR]]

• why replace if auth_sessions: with if len(auth_sessions) > 0: ?
  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.
  [[BR]]

• # TODO selection session - this is crucial and should be moved to a method that could eventually do more than just fine the matching display
  perhaps the authenticator should decide about sessions and display?
  [[BR]]

• sessions_info changes - not sure what that does, not looked at it carefully - should be split up as this is unrelated?
  with this change, multiple session are returned on get_info (related to get_sessions)
  [[BR]]

• the hole_punch_authenticator needs an explanation
  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.
  [[BR]]

• opts.local_port = client_proto._conn.local[1] - what does this do?
  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.
  [[BR]]

• why do we want / need this?

if hasattr(socket, 'SO_REUSEPORT'):
   listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1)

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

• socket_connect(dtype, host, port, reuse=False,local_port=0) - can you explain why we need to re-use the local_port and how it's done?

• While holepunching, the server needs to connect to the client using the port the client is trying to reach (server port).
• Also the client is trying to establish a connecting to the server using the same local port as used to connect to the proxy (since the proxy discovers the clients remote address and relays it to the server).
  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.
[[BR]]

• hp-client-status / hp-server-status - what does this do?
  While server and client are hole punching, they send status updates to the proxy.
  [[BR]]

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

[totaam]

• the token stuff should be separate - as I'm not sure I can merge that
• if the authentication modules return a list of sessions, then "displays" should not be a list
• I'll try to merge the socket patch early for 4.2
• the hp patch is way too big and includes some commented out code, needs splitting up - and as I said before, we can't pollute the interface with hope-punching concerns
• there is no documentation..

[totaam]

This approach is way to complicated.