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

Added clientname to session policy and environment #2099

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

hg-sepag
Copy link

Upon reconnect, we needed to determine the correct running session by clientname rather than by IP number because clients may connect via service with dynamic IPs or from different clients over the same IP (NAT). Also i made available the clientname as an environment variable CLIENTNAME inside the session.
The changes consist mainly of looping through of clientname from rdp into the session.

@matt335672
Copy link
Member

Thanks for this, @hg-sepag

I've had a quick look through this, and it certainly like a worthwhile addition to me.

We are, however, just about to pull v0.9.18 together, and either as part of this (or shortly after) we will be merging #2011 which is going to cause some quite important changes to the SCP code. As I'm sure you're aware, this is an area of xrdp which needs quite a bit of work. #2011 is the first of a series of changes that will make SCP a lot easier to work with.

#2011 will break your code here, but it will be a simple matter to fix. At that point we can have a serious go at merging this.

A couple of other observations for you in the meantime:-

  1. You'll need to change the value of CLIENT_INFO_CURRENT_VERSION in common/xrdp_client_info.h. There's a comment in that file here which explains that.
  2. The comment about the Policy variable in sesman/sesman.ini.in will need updating.
  3. The manpage template docs/man/sesman.ini.5.in will need updating.
  4. I'm not so sure adding the CLIENTNAME to the session is a good idea, as for use-cases other than yours, the client can change between connections, or even there there at all. In the longer-term, it will be possible to query sesman to get the information directly, but we aren't there yet for the reasons I've outlined above.

Does that make sense? Happy to chat about any of the above.

@hg-sepag
Copy link
Author

Hello @matt335672 ,

thank you for your comments. Sorry to answer so late, but immediatly after this pull request i went on holiday :-)

1. You'll need to change the value of CLIENT_INFO_CURRENT_VERSION in common/xrdp_client_info.h. There's a comment in that file here which explains that.

2. The comment about the Policy variable in sesman/sesman.ini.in will need updating.

3. The manpage template docs/man/sesman.ini.5.in will need updating.

This all is quite clear, i simply did not know and will do it in the next couple of days.

4. I'm not so sure adding the CLIENTNAME to the session is a good idea, as for use-cases other than yours, the client can change between connections, or even there there at all. In the longer-term, it will be possible to query sesman to get the information directly, but we aren't there yet for the reasons I've outlined above.

This behavior is similar to M$ Windows: here %CLIENTNAME% is also set at logon via RDP. It is true that our use case is probably special, but i think the same is true for the other session allocation policies except "per user".

@matt335672
Copy link
Member

Hi @hg-sepag

Thanks for the reply. Definitely worth putting in a PR before having a holiday - nothing much happens for a few days anyway :-)

About my comment 1 - with more thought on my part it's probably better to just more the member declaration further down in the file to somewhere below the 'private to xrdp' comment. That way, the change will work without needing to also recompile xorgxrdp, if you're using that.

I take your point about the %CLIENTNAME% compatibility - I've just reproduced this myself on a Win 10 target. I'm happy with your justification, but we can put the point to the rest of the team when the PR is re-worked.

#2011 is now merged. As a result, all the SCP V0 code is in one place so when you re-work the PR you will need to consider what to do about the sesrun utility, as this will now fail to compile when you modify SCP. You've already got the clientname string as an optional part of the protocol, so you may also need to test for a non-null value before setting the CLIENTNAME environment variable.

Hope that makes sense - it's getting late here so it may not do. Let me know if you have any questions.

@yxzzx
Copy link

yxzzx commented Nov 8, 2022

Might it be worthwhile to consider my idea for the client ip and put the client name into a xserver resource? I think using xrdb to get session information is much easier than using xrdp-sesadmin. Maintenance for xrdp-sesadmin should be easier as well when there is no need to parse (complex) query commands. Using xserver resources should be similar to windows, because with resources you automatically have session and not process scope (as with e.g. environment variables).

@matt335672
Copy link
Member

The xserver resource database is a clever idea. It does tie us in to an X server though, which looking (a fair way) ahead may not be the best way.

xrdp-sesadmin can be made as complex or easy to use for this purpose as we like - it's within our gift. We could use something like xrdp-sesadmin --query-session client_name for example. In fact we could even use xrdp-sesadmin as a wrapper around xrdb so that the user doesn't need to be concerned with the implementation detail.

@hg-sepag
Copy link
Author

hg-sepag commented Nov 9, 2022 via email

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 this pull request may close these issues.

3 participants