Refactor ip address handling #2251
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Supplies the IP address that an authentication event is received from as the PAM parameter PAM_RHOST for PAM-capable systems.
Made session allocation policies more readable and maintainable. The 'C' policy which was confusing before has been replaced with the 'Separate' keyword. This is a public interface change, but is unlikely to affect many users. The logging in session_get_bydata() is substantially improved, making it far easier to spot why sessions are getting matched or not matched.
matt335672
force-pushed
the
refactor_ip_addr_handling
branch
from
67e0337
to
3e48877
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #392 #2239
Replacement for the withdrawn #2223
The main reason for withdrawing #2223 was I discovered there were two separate places in the xrdp_client_info structure which described the currently connected client:-
The
connection_descriptionfield. This was originally (and confusingly) calledclient_ip, and introduced by commit d797b2c for xrdp v0.6.0The
client_addrandclient_portfields introduced by commit 2536946 for xrdp v0.8.0In terms of maintainability and readability, it made sense to unify these two sets of fields into a single set of fields.
Another confusion is the presence of the 'C' argument to the session allocation policy in sesman.ini. The code for this in sesman uses the
connection_descriptionfield. This is explored further in #2239This series of commits does the following:-
connection_description,client_addrandclient_portwith two unified fields in xrdp_client_info. These fields areclient_ipandclient_description. they are used as follows:-client_ipcontains the IP address of the client. For other connection types (e.g. AF_UNIX and AF_VSOCK) this field is empty.client_descriptioncontains a string which describes the connected client.g_bitmask_to_charstr()andg_charstr_to_bitmask()are added to string_calls.c. These are intended for parsing thePolicystring in sesman.ini in a more flexible way.This PR changes the public interface of the session allocation policy in sesman.ini. Effectively, the old 'C' policy is achieved by setting the policy to 'Separate'. This is I believe a little clearer, and I also think it is unlikely there are many users of the 'Separate' policy anyway. A use of the old 'C' flag is not honoured, but a warning is currently generated if it is discovered.