Xpra: Ticket #744: Allow specifying password/key via environment

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.



Sat, 22 Nov 2014 19:11:47 GMT - Antoine Martin: owner changed

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..


Sat, 22 Nov 2014 19:44:12 GMT - 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).


Sat, 22 Nov 2014 19:49:26 GMT - 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.


Sat, 22 Nov 2014 20:08:16 GMT - reimar: attachment set

Patch adding XPRA_PASSWORD and XPRA_ENCRYPTION_KEY environment variables


Sat, 22 Nov 2014 20:09:19 GMT - 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).


Sat, 22 Nov 2014 20:14:43 GMT - reimar: owner changed

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


Sat, 22 Nov 2014 20:27:00 GMT - Antoine Martin: owner changed

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:

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?


Sat, 22 Nov 2014 21:14:17 GMT - reimar: attachment set

Patch adding XPRA_PASSWORD and XPRA_ENCRYPTION_KEY environment variables


Sat, 22 Nov 2014 21:18:41 GMT - 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?


Sat, 22 Nov 2014 21:22:18 GMT - reimar: attachment set

Patch with assert updated instead of removed


Sat, 22 Nov 2014 21:27:51 GMT - 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).


Sat, 22 Nov 2014 21:30:49 GMT - 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.


Sat, 22 Nov 2014 21:36:52 GMT - 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,


Sun, 23 Nov 2014 02:32:39 GMT - 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.


Sun, 23 Nov 2014 06:02:40 GMT - reimar: status changed; resolution set

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.


Fri, 08 Apr 2016 08:04:20 GMT - Antoine Martin:

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


Sat, 23 Jan 2021 05:04:42 GMT - migration script:

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/744