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

Session allocation policy - Connection setting not useful? #2239

Closed
matt335672 opened this issue Apr 24, 2022 · 8 comments · Fixed by #2251
Closed

Session allocation policy - Connection setting not useful? #2239

matt335672 opened this issue Apr 24, 2022 · 8 comments · Fixed by #2251

Comments

@matt335672
Copy link
Member

sesman.ini contains the following section for the session allocation policy:-

;; Policy - session allocation policy
; Type: enum [ "Default" | "UBD" | "UBI" | "UBC" | "UBDI" | "UBDC" ]
; "Default" session per <User,BitPerPixel>
; "UBD" session per <User,BitPerPixel,DisplaySize>
; "UBI" session per <User,BitPerPixel,IPAddr>
; "UBC" session per <User,BitPerPixel,Connection>
; "UBDI" session per <User,BitPerPixel,DisplaySize,IPAddr>
; "UBDC" session per <User,BitPerPixel,DisplaySize,Connection>
Policy=Default

The above is the latest, but little has changed since xrdp v0.9.1 when this section was introduced as part of commit 1934c9e

The problem I've got is the 'connection' setting. A 'connection' in this context is a string created by common/os_calls.c:g_write_ip_address(). The string is of the form:-

<ip_addr>:<port> - socket: <sck>

where <ip_addr> and <port> are from the peer connected to xrdp, and sck is the socket number allocated to the connection by xrdp.

If 'C' is in the session allocation policy, the connection string for an existing session has to match the connection string for a reconnection for a reconnect to succeed (code is here). I can't see how this can succeed reliably. Both <port> and <sck> are very likely to be different on a reconnect.

So as far as I can tell, the 'Connection' argument to the Policy setting is effectively useless. Consequently, I'd like to remove this option for the next major release as it seems to have no use cases, and so no-one can be using it. It can usefully be replaced with the client name - see #2099

Am I misunderstanding something here?

@fpaquet - am I correct in thinking you had something to do with 1934c9e? If so, I'd appreciate you telling me what I'm misunderstanding, or what your actual intentions were for this setting.

Thanks for any enlightenment.

@fpaquet
Copy link

fpaquet commented Apr 24, 2022

Thank you for the question.

The meaning of was basically to open a new session for each user connection.
Meant to also have a policy option, ensuring never to connect to an existing session (always start a new session).

If you are not lucky with this, you can use .
Converting IP address into names would have given additional overhead, and may be forged externally in name service.

@matt335672
Copy link
Member Author

Thank you for your answer.

What would you then do with the existing session(s) in that model? It seems you could never connect to them at all. Is that right?

@fpaquet
Copy link

fpaquet commented Apr 24, 2022

Correct, they're kind of lost but still can run some app's or commands.

You might want to set the KillDisconnected, DisconnectedTimeLimit or IdleTimeLimit in sesman.ini
to get it cleaned up.

@matt335672
Copy link
Member Author

OK - thanks for the explanation.

It seems like an odd kind of use-case to me, but I'm sure a lot of things I do seem odd to other people!

It's very easy to support. In fact, it doesn't even need to be combined with the other options, as if it's in any combination. session_get_bydata() will always fail.

My main reason for asking this question, incidentally, is to tidy up the sesman code in this area. I'd also like to make it easier for other contributors to provide PRs like #2099 which cater for other use-cases the team haven't thought of.

How does this look to you as a modified interface for the next major version?

; Policy - session allocation policy
;
;  Type: enum [ "Default" | "Separate" | Combination from {UBDIN} ]
;  "Default"    Same as UB
;  "Separate"   All sessions are separate. Sessions can never be rejoined,
;               and will need to be cleaned up manually, or automatically
;               by setting other sesman options.
;
;   Combination options:-
;      U        Sessions are separated per user
;      B        Sessions are separated by bits-per-pixel
;      D        Sessions are separated by initial display size
;      I        Sessions are separated by IP address
;      N        Sessions are separated by client name sent by the client
;
;   The options U and B are always active, and cannot be de-selected.
Policy=Default

@fpaquet
Copy link

fpaquet commented Apr 26, 2022

People using 'C' will have to reconfigure Xrdp.
All others can continue without configuration change.

Hard to say how many are using option 'C'.

Needs at least a release note, pointing to the configuration change.

@matt335672
Copy link
Member Author

Yes - agreed.

We've got other config changes in the next major version, including a move to Unix Domain Sockets. The old config files will work, but will generate warnings. I can add a warning that 'C' is replaced with 'Separate'. Would that help do you think?

In all the time I've been triaging faults, few people have been changing this option from the default, and no-one has mentioned the 'C' option at all.

@fpaquet
Copy link

fpaquet commented Apr 28, 2022

Because it's a background service, it would be very helpful to have a logged warning. Warning will also help you, to reduce the number of support issues.

@matt335672
Copy link
Member Author

I'll add a warning.

matt335672 added a commit to matt335672/xrdp that referenced this issue Apr 30, 2022
The connected client is currently described in two places in
the xrdp_client_info structure:-

1) In the connection_description field. This was introduced as
   field client_ip by commit d797b2c
   for xrdp v0.6.0

2) In the client_addr and client_port fields introduced by commit
   2536946 for xrdp v0.8.0

This commit unifies these two sets of fields into a single
set of fields describing the connection IP and port (for
AF_INET/AF_INET6 connections only) and a connection description
for all connection types.

The code in os_calls to provide client logging has been simplified
somewhat which should make it easier to add new connection types (e.g.
AF_VSOCK).

The old connection_description field used to be passed to sesman to
inform sesman of the IP address of the client, and also to provide
a string for 'C' field session policy matching. 'C' field session policy
matching does not actually need this string (see neutrinolabs#2239), and so now only
the IP field is passed to sesman.
matt335672 added a commit to matt335672/xrdp that referenced this issue May 4, 2022
The connected client is currently described in two places in
the xrdp_client_info structure:-

1) In the connection_description field. This was introduced as
   field client_ip by commit d797b2c
   for xrdp v0.6.0

2) In the client_addr and client_port fields introduced by commit
   2536946 for xrdp v0.8.0

This commit unifies these two sets of fields into a single
set of fields describing the connection IP and port (for
AF_INET/AF_INET6 connections only) and a connection description
for all connection types.

The code in os_calls to provide client logging has been simplified
somewhat which should make it easier to add new connection types (e.g.
AF_VSOCK).

The old connection_description field used to be passed to sesman to
inform sesman of the IP address of the client, and also to provide
a string for 'C' field session policy matching. 'C' field session policy
matching does not actually need this string (see neutrinolabs#2239), and so now only
the IP field is passed to sesman.
matt335672 added a commit to matt335672/xrdp that referenced this issue May 4, 2022
The connected client is currently described in two places in
the xrdp_client_info structure:-

1) In the connection_description field. This was introduced as
   field client_ip by commit d797b2c
   for xrdp v0.6.0

2) In the client_addr and client_port fields introduced by commit
   2536946 for xrdp v0.8.0

This commit unifies these two sets of fields into a single
set of fields describing the connection IP and port (for
AF_INET/AF_INET6 connections only) and a connection description
for all connection types.

The code in os_calls to provide client logging has been simplified
somewhat which should make it easier to add new connection types (e.g.
AF_VSOCK).

The old connection_description field used to be passed to sesman to
inform sesman of the IP address of the client, and also to provide
a string for 'C' field session policy matching. 'C' field session policy
matching does not actually need this string (see neutrinolabs#2239), and so now only
the IP field is passed to sesman.
matt335672 added a commit to matt335672/xrdp that referenced this issue May 5, 2022
The connected client is currently described in two places in
the xrdp_client_info structure:-

1) In the connection_description field. This was introduced as
   field client_ip by commit d797b2c
   for xrdp v0.6.0

2) In the client_addr and client_port fields introduced by commit
   2536946 for xrdp v0.8.0

This commit unifies these two sets of fields into a single
set of fields describing the connection IP and port (for
AF_INET/AF_INET6 connections only) and a connection description
for all connection types.

The code in os_calls to provide client logging has been simplified
somewhat which should make it easier to add new connection types (e.g.
AF_VSOCK).

The old connection_description field used to be passed to sesman to
inform sesman of the IP address of the client, and also to provide
a string for 'C' field session policy matching. 'C' field session policy
matching does not actually need this string (see neutrinolabs#2239), and so now only
the IP field is passed to sesman.
@matt335672 matt335672 linked a pull request May 5, 2022 that will close this issue
matt335672 added a commit to matt335672/xrdp that referenced this issue May 18, 2022
The connected client is currently described in two places in
the xrdp_client_info structure:-

1) In the connection_description field. This was introduced as
   field client_ip by commit d797b2c
   for xrdp v0.6.0

2) In the client_addr and client_port fields introduced by commit
   2536946 for xrdp v0.8.0

This commit unifies these two sets of fields into a single
set of fields describing the connection IP and port (for
AF_INET/AF_INET6 connections only) and a connection description
for all connection types.

The code in os_calls to provide client logging has been simplified
somewhat which should make it easier to add new connection types (e.g.
AF_VSOCK).

The old connection_description field used to be passed to sesman to
inform sesman of the IP address of the client, and also to provide
a string for 'C' field session policy matching. 'C' field session policy
matching does not actually need this string (see neutrinolabs#2239), and so now only
the IP field is passed to sesman.
derekschrock pushed a commit to derekschrock/xrdp that referenced this issue Dec 15, 2022
The connected client is currently described in two places in
the xrdp_client_info structure:-

1) In the connection_description field. This was introduced as
   field client_ip by commit d797b2c
   for xrdp v0.6.0

2) In the client_addr and client_port fields introduced by commit
   2536946 for xrdp v0.8.0

This commit unifies these two sets of fields into a single
set of fields describing the connection IP and port (for
AF_INET/AF_INET6 connections only) and a connection description
for all connection types.

The code in os_calls to provide client logging has been simplified
somewhat which should make it easier to add new connection types (e.g.
AF_VSOCK).

The old connection_description field used to be passed to sesman to
inform sesman of the IP address of the client, and also to provide
a string for 'C' field session policy matching. 'C' field session policy
matching does not actually need this string (see neutrinolabs#2239), and so now only
the IP field is passed to sesman.
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