xpra icon
Bug tracker and wiki

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#744 closed enhancement (fixed)

Allow specifying password/key via environment

Reported by: reimar Owned by: reimar
Priority: minor Milestone:
Component: core Version: trunk
Keywords: Cc:

Description

The password/key files are rather inconvenient when you want to easily start/script a complex setup that has to go through multiple ssh tunnels and uses an auto-generated random password/key.
I suggest a solution inspired by what e.g. MOSH does, and allow specifying them via environment variables.
Attached is an ugly patch that adds XPRA_PASSWORD and XPRA_ENCRYPTION_KEY.
I am really bad at python, so sorry if it's messy, plus some of the code changes should probably be refactored.

Attachments (3)

env_pwd_enc.diff (10.1 KB) - added by reimar 6 years ago.
Patch adding XPRA_PASSWORD and XPRA_ENCRYPTION_KEY environment variables
env_pwd_enc.2.diff (10.2 KB) - added by reimar 6 years ago.
Patch adding XPRA_PASSWORD and XPRA_ENCRYPTION_KEY environment variables
env_pwd_enc.3.diff (10.3 KB) - added by reimar 6 years ago.
Patch with assert updated instead of removed

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by Antoine Martin

Owner: changed from Antoine Martin to reimar

Would a --password= command line option help instead?
The patch isn't too bad as it is, but I would prefer something more obvious.
What is so difficult about using a temp password file?
Before merging, I would need man page updates and "--help" updates, etc..

comment:2 Changed 6 years ago by reimar

I understand that --password= option would seem nicer, but such an option can't be used safely as basically any user on the system can read out the command-line and thus see the password.
There are several things that make me dislike password files:
1) If you just wanted to do a "xpra start" on a remote server with a password you just generated, you'd have to do an scp (or worse hacks using only ssh) to get the password to the server
2) Short version: Using a password file securely is a huge pain.
Longer version: You have to be careful that these password files are not accessible by the wrong persons. So you have to make sure you have the right umask set. You have to check that your commands to create them actually don't fail, otherwise you might be using a file an attacker created "for you". You also have to make sure the directory where you created the file is either only accessible by the user or has the +t bit set, otherwise you have a race condition.

I'll try to find time for man page/help updates if that should be the major blocking part, I just wanted to get an idea if this has a chance of being integrated before refining it (though of course any help is welcome).

comment:3 Changed 6 years ago by Antoine Martin

any user on the system can read out the command-line and thus see the password


That's why we currently have --password-file instead.
We can change the command shown with ps, etc. But it's not foolproof.

Short version: Using a password file securely is a huge pain.
(..)


Understood.

I just wanted to get an idea if this has a chance of being integrated before refining it


I see no reason not to, as long as you can ensure everything is well documented.

Changed 6 years ago by reimar

Attachment: env_pwd_enc.diff added

Patch adding XPRA_PASSWORD and XPRA_ENCRYPTION_KEY environment variables

comment:4 Changed 6 years ago by reimar

I added descriptions to the man page. Is there some other documentation I should update?
I also fixed a bug that I never used the filtered_env (even though that feature is maybe overkill anyway).

comment:5 Changed 6 years ago by reimar

Owner: changed from reimar to Antoine Martin

Just trying to fix the owner. No idea why it changed back to me.

comment:6 Changed 6 years ago by Antoine Martin

Owner: changed from Antoine Martin to reimar

Just trying to fix the owner. No idea why it changed back to me.


I change it to whoever is supposed to respond to the last comment. And I've just done it again. (no big deal if you forget to re-assign to me, I get email notifications)

Reviewing the patch:

  • filtered_env = os.environ should be using filtered_env = os.environ.copy(), or calling xpra info will change the env variable value! (modifying the same dictionary in place)
  • I would prefer checking with the python "not": + if not self.password_file and not os.environ.get('XPRA_PASSWORD'): (rather than checking with "is None" - if the env var is an empty string, we don't want to use it either, which will now be caught) - multiple instances of this pattern
  • removing assert self.password_file is probably safe, but I would prefer replacing it with a better assert instead

Apart from these small nitpicks, it looks ok. I don't think it is worth refactoring the env lookup bits, they are repeated but no big deal.

Can you send us an updated patch?

Changed 6 years ago by reimar

Attachment: env_pwd_enc.2.diff added

Patch adding XPRA_PASSWORD and XPRA_ENCRYPTION_KEY environment variables

comment:7 Changed 6 years ago by reimar

Ok, done.
A few places got more ugly since key is checked against "None", so I needed more checks. Check if that looks like what you had in mind. Also the filtered_env I guess you could discuss which condition that should use.
I also think I had a bug in get_encryption_key where I preferred XPRA_PASSWORD over password_file, might be worth if you check I got it right now.
I forgot about the assert part, will update the patch once more later.
By chance I stumbled over a lot of other environment variables (XPRA_SOCKET_TIMEOUT XPRA_TCP_NODELAY XPRA_SOCKET_DIR XPRA_ALLOW_UNENCRYPTED_PASSWORDS XPRA_DETECT_LEAKS) are those purely internal or should I maybe add those to the man page as well (as a separate patch/ticket) while I'm at it?

Changed 6 years ago by reimar

Attachment: env_pwd_enc.3.diff added

Patch with assert updated instead of removed

comment:8 Changed 6 years ago by Antoine Martin

Final nitpick. Instead of using this pattern in a few places:

if v is None and os.environ.get('XPRA_PASSWORD'):
   v = os.environ.get('XPRA_PASSWORD')
return v

You could just do:

return v or os.environ.get('XPRA_PASSWORD')

No point in checking if the env value exists or not, just use it as fallback, and if it is unset it will be None, which also evaluates to False.


By chance I stumbled over a lot of other environment variables ..


Feel free to add those, there may even be more.
I have not documented them because not all of them were meant to remain (tcp nodelay can probably be removed I think), and they are rarely useful (socket timeout only when running with valgrind, etc).

comment:9 Changed 6 years ago by reimar

Hm, when I run with --encryption=AES "xpra stop" hangs or fails with error instead of terminating the server.
I think this is an existing bug and not something I introduced but it might be worth double-checking that it's not a real bug in my patch and I am just failing at testing.

comment:10 Changed 6 years ago by reimar

The one example you had for this pattern might be possible to avoid, though I still think it risky.
But the bigger problem is e.g. this code:
+ if key is None and os.environ.get('XPRA_ENCRYPTION_KEY'):
+ key = os.environ.get('XPRA_ENCRYPTION_KEY')

if key is None and self.password_file:

If I remove the "and os.environ.get('XPRA_ENCRYPTION_KEY')" part here, then as I understand it setting XPRA_ENCRYPTION_KEY to will result in us not falling back to the password, which seems unexpected behaviour if we in all other aspects ignore and empty environment variable.
If you think this is reasonable behaviour I don't mind changing it though,

comment:11 Changed 6 years ago by Antoine Martin

with --encryption=AES "xpra stop" hangs or fails with error instead of terminating the server


I cannot reproduce this, though r8144 improves the error handling (ie: missing key file, etc).

As for the rest, I've merged it in r8145. If that works for you, please close this ticket.

comment:12 Changed 6 years ago by reimar

Resolution: fixed
Status: newclosed

Thanks, it works for me, closing.
I'll see if I can dig into the "xpra stop" issue.
But for me (note: running locally, both with and without --no-mmap) stop only works when not specifying --encryption=AES, even when the server uses encryption.

comment:13 Changed 4 years ago by Antoine Martin

This was removed briefly during the development of 0.17, then restored again. See r12334.

Note: See TracTickets for help on using tickets.