-
Notifications
You must be signed in to change notification settings - Fork 15k
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
.RDP Password Attribute #3756
.RDP Password Attribute #3756
Conversation
Allow password to be stored in .RDP file and parsed and settings updated, this will allow for dynamic .RDP files to be created with complete login credentials, using this method the username, server and password will no longer be visible within process lists. Also fixed issue of username and domain being read from .RDP files and set to null by command line processor.
Can one of the admins verify this patch? |
@freerdp-bot test |
Refer to this link for build results (access rights to CI server needed): |
@freerdp-bot test please |
Refer to this link for build results (access rights to CI server needed): |
client/common/cmdline.c
Outdated
free(settings->Username); | ||
|
||
if (!settings->Domain && user) | ||
if (!settings->Username && user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is no good idea. If I want to override the username provided via RDP file this check will prevent it. A simple if (user)
should do the trick.
@bigpjo Sorry for the change of mind, just found this issue when retesting your pull. |
As a remark: Although this is a possible solution we haven't added "password" support in .rdp files yet because you then possibly have files lying around with plain passwords. So if you don't wan't to show the user/server/domain/password in the processes list you probably also don't want to have the password in files. I think currently the best solution is to create a wrapper script and use /from-stdin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for @akallabeth 's remarks I looks good to me. Thanks.
I appreciate that storing plain text passwords in the file are not ideal and this is not a suggestion/change I would recommend without a specific goal in mind, there are a number of other open source projects dependent on starting FreeRDP from a command line, it is these projects that would benefit from this change as it would be possible to create .RDP files with a password, start the process and delete the file after. Not ideal but a solution. If it would be of benefit, I could also add another command line parameter to delete the .RDP file once the process is started to ensure that there are not 100's of .RDP files with passwords left around but this would still be user choice. In .RDP files there is a hashed password field password 51, the issue with this is that you need a windows API call for the RDP client to decrypt this and send it to establish the report session, this will not work I believe in a linux/mac/unix env making this type of change non portable. Any comments welcomed, in the grand schema of things, this pull request is not a major contribution to an excellent project |
My 2 euros cents: previous version of mstsc on mac used to support the password in clear. |
@bigpjo Agree with the necessity of storing the password in some cases. That aside, if you fix the check for allowing command line to override RDP username you got my vote ;) |
Change to allow username command line parameter to overwrite the .RDP username
@freerdp-bot test |
@freerdp-bot test, come on |
Refer to this link for build results (access rights to CI server needed): |
Allow password to be stored in .RDP file, parsed and settings
updated, this will allow for dynamic .RDP files to be created with
complete login credentials, using this method the username, server and
password will no longer be visible within process lists.
Also fixed bug with username being
set to null by command line processor if the command line parameter not specified but the value set from the .RDP files.
Usage: create .RDP file, add line
password:s:XXXXXX where XXXXXX is the password