Fix Windows compatibility issues and AGE support #436

SlugFiller · 2023-09-07T16:02:33Z

Builds on top of #362. Fixed parts that weren't working correctly, ensured it builds on Python 3.11 without errors, fixed issues with AGE, and added Windows-specific documentation.

Only thing missing is GPG support. However, I couldn't find a user-friendly non-CygWin GPG client for Windows. And I've never used GPG in the past. This makes it difficult to test, debug, or even just document GPG usage.

All other tools are confirmed to work, and appropriate Windows-specific tooling for them has been documented.

SlugFiller · 2023-09-07T21:10:57Z

Should I add a Windows build GitHub Workflow to this? I'm not too familiar with Workflows, but it shouldn't be too difficult. The question is whether CI for Windows is a priority, and if it should be part of this PR or separate.

romanz · 2023-09-08T09:36:56Z

Many thanks for the contribution!
Will review this weekend :)

romanz

Initial review - please take a look below:

doc/README-Windows.md

libagent/win_server.py

setup.py

libagent/ssh/protocol.py

romanz · 2023-09-08T13:26:01Z

libagent/ssh/__init__.py

@@ -154,12 +197,15 @@ def run_server(conn, command, sock_path, debug, timeout):
    ret = 0
    try:
        handler = protocol.Handler(conn=conn, debug=debug)
-        with serve(handler=handler, sock_path=sock_path,
-                   timeout=timeout) as env:
+        serve_platform = serve_win if sys.platform == 'win32' else serve_unix


Suggested change

serve_platform = serve_win if sys.platform == 'win32' else serve_unix

serve_func = serve_win if sys.platform == 'win32' else serve_unix

romanz · 2023-09-08T13:34:31Z

Also the CI seems to fail - please take a look:
https://github.com/romanz/trezor-agent/actions/runs/6114735009/job/16605844896?pr=436#step:5:74

SlugFiller · 2023-09-08T16:47:40Z

I believe I fixed the CI. But without a Dockerfile to run Python Tox, I can't test it locally.

I'm also poking a bit into GPG support. I've already found a few issues that only appear on Windows, e.g. needing to URL-decode paths returned by gpgconf --list-dirs, or mkdir being unavailable on Windows. If I don't manage to get it working (and documented) soon, however, I can leave it to a separate PR.

There's also a mild issue with it that I'm unsure how to handle, and could influence all the tools. And that's the location of pinentry. Gpg4Win contains a version of it, but it's not provided on the path. Rather, its path can be determined using gpgconf --list-components. I could attempt to use that to modify the default if unspecified. The question is whether to only do this on Windows, or allow this check on Linux as well.

Also, it's a slight issue for me to test, since I don't have a hardware device that requires an on-computer PIN entry.

romanz · 2023-09-08T18:34:07Z

Please fix the code style issues:

libagent/win_server.py:20:1: E302 expected 2 blank lines, found 1
libagent/win_server.py:42:18: E261 at least two spaces before inline comment
libagent/win_server.py:47:1: W293 blank line contains whitespace
libagent/win_server.py:55:101: E501 line too long (107 > 100 characters)
libagent/win_server.py:60:9: E722 do not use bare 'except'
libagent/win_server.py:119:101: E501 line too long (103 > 100 characters)
libagent/win_server.py:121:101: E501 line too long (106 > 100 characters)
libagent/win_server.py:194:9: E722 do not use bare 'except'
libagent/ssh/__init__.py:36:1: E302 expected 2 blank lines, found 1
libagent/ssh/__init__.py:206:35: E261 at least two spaces before inline comment
libagent/ssh/__init__.py:208:37: E261 at least two spaces before inline comment
libagent/ssh/__init__.py:315:22: W605 invalid escape sequence '\p'

https://github.com/romanz/trezor-agent/actions/runs/6123995379/job/16624649832?pr=436

romanz · 2023-09-08T18:43:30Z

@Pandapip1 could you please help testing this PR?

Pandapip1 · 2023-09-08T18:52:56Z

Sure! Are there any build instructions I can follow?

For now, I'm making a program using USB/IP that mostly implements the smart card USB interface for use with the Trezor. But if you could get this working, that'd be better!

SlugFiller · 2023-09-09T04:44:52Z

Made my best effort to lint
Fixed GPG. Tested by signing this commit (May upload my public key to GitHub later)
Refactored win_server so it now has the same interface as a regular UNIX socket, and doesn't require a separate server loop
Device UI will now attempt to get pinentry location from gpg-conf --list-components instead of assuming it is in the path. It will fall back to just pinentry if it fails.

Currently not working (On Windows):

Creating identity as subkey. Since it requires making an outgoing communication to gpg-agent, which is a slightly different beast from receiving an incoming one.

SlugFiller · 2023-09-09T06:38:54Z

Improved Ctrl+C behavior. Before, running the GPG agent directly with no client would cause it to freeze, and be uninterruptible. Also improved behavior of named pipe so it remains open while waiting for a connection, and closes instantly on Ctrl+C.

SlugFiller · 2023-09-09T10:12:18Z

Fixed, but didn't test, GPG subkey registration.

SlugFiller · 2023-09-09T16:21:47Z

More linting

SlugFiller · 2023-09-09T17:48:51Z

isorted

libagent/gpg/keyring.py

libagent/util.py

libagent/win_server.py

SlugFiller · 2023-09-09T19:52:58Z

More lints, and minor fixes. Hopefully CI finally passes this time.

SlugFiller · 2023-09-10T09:01:10Z

Okay, after playing around with GPG for a day, I made some minor changes, and updated the docs a lot. And also, GPG is awesome, and I don't know why people are trying to find replacements for it.

I also have a pretty good idea of how to implement an add command that will allow creating an additional hardware-backed identity on the same GPG profile. I feel like such a feature is necessary, since you don't want to use an identity with an @users.noreply.github.com email outside of GitHub, yet GitHub won't allow you to verify with an identity that doesn't have a matching email.

But adding it to this PR would be too many features in one PR. So, as soon as all the fixes are done and this PR is merged, I'll work on that feature.

romanz · 2023-09-10T17:12:17Z

Since rebasing the previous PR killed its signature, should I try rebasing so my commit looks like a merge?

It's OK to rebase without signing the commit, if you prefer.

romanz · 2023-09-10T17:12:53Z

Why does keyring.get_agent_sock_path call gpgconf --list-dirs, parses all the directories, and then extracts only agent-socket instead of just calling gpgconf --list-dirs agent-socket?

Good catch - if gpgconf --list-dirs agent-socket works, let's use it :)

romanz · 2023-09-10T17:16:35Z

libagent/util.py

+    Does not add quotes. This allows appending multiple strings inside the quotes.
+    """
+    if sys.platform == 'win32':
+        return in_str.translate({37: '%%', 34: '\"\"'})


nit: prefer using a single char key for readability:

>>> 'abcba'.translate(str.maketrans({'a': 'AA', 'b': 'BB'})) 'AABBcBBAA'

(here and below)

romanz · 2023-09-10T17:18:58Z

libagent/ssh/__init__.py

@@ -258,8 +295,8 @@ def main(device_type):

    public_keys = None
    filename = None
-    if args.identity.startswith('/'):
-        filename = args.identity
+    if args.identity.startswith('/') or args.identity.startswith('file:'):


nit: please define FILE_PREFIX = 'file:' and use it and len(FILE_PREFIX) here:

romanz · 2023-09-10T17:20:42Z

libagent/ssh/__init__.py

+        log.error('running in foreground mode requires specifying %s', SOCK_TYPE_PATH)
+        sys.exit(1)
+    elif sys.platform == 'win32':
+        suffix = random.choices(string.ascii_letters, k=10)


Not sure - will any ASCII letter suffix work on Windows?
Maybe it's simpler/safer to use something like os.urandom(N).hex() here? or even https://docs.python.org/3/library/tempfile.html#tempfile.mkstemp?

mkstemp is not applicable because it will try to create and open the file, which can't be done for a pipe address. mktemp will also try to check for the file's existence, which may or may not work for a pipe address, and is deprecated.

romanz · 2023-09-10T17:37:23Z

libagent/ssh/__init__.py

+        try:
+            os.unlink(self.file.name)
+        except OSError:
+            pass


Please add a log.warning here (in case file removal fails).

romanz · 2023-09-10T18:01:39Z

libagent/gpg/__init__.py

-    check_call(['mkdir', '-p', homedir])
-    check_call(['chmod', '700', homedir])
+    # Prepare the key before making any changes
+    key_id, public_key = export_public_key(device_type, args)


nit:

Suggested change

key_id, public_key = export_public_key(device_type, args)

key_id, public_key_bytes = export_public_key(device_type, args)

romanz · 2023-09-10T18:05:30Z

Sure! Are there any build instructions I can follow?

@Pandapip1 Please take a look at https://github.com/romanz/trezor-agent/blob/d0d0c83638f29d631a0e56f6e15e87d30fe9ea9a/doc/README-Windows.md

romanz · 2023-09-10T18:17:38Z

LGTM (reviewed most of the code - except win_server.py), many thanks!

SlugFiller · 2023-09-10T19:16:00Z

It's OK to rebase without signing the commit, if you prefer.

Already rebased as a merge. Was fairly simple to do

Good catch - if gpgconf --list-dirs agent-socket works, let's use it :)

It does, used.

Fixed pydocs. According to tox.ini this should be the last step. (Unless coverage has an issue with me too)

SlugFiller · 2023-09-10T19:21:01Z

Okay, now it should lint. Crosses fingers

SlugFiller · 2023-09-10T19:27:56Z

Drat. Seems like the agent_sock_path optimization either only works on Windows, or only works with a specific version of GPG. Reverted that change.

SlugFiller · 2023-09-10T19:28:43Z

No wait. The issue is with the test. It uses a mock subprocess

SlugFiller · 2023-09-10T19:31:54Z

Okay, how do I fix it? If I change the mock process, it's going to be a trivial test. If I use a real call, would GPG be available in the testing environment? Alternately, I can just remove the test.

SlugFiller · 2023-09-10T19:40:09Z

Okay, for now I'm attempting with a real process. If it goes well, great. If not, I'm going to need help with this.

SlugFiller · 2023-09-10T20:19:24Z

Okay, that went better than I was expecting. Corrected the test. Should work now.

SlugFiller · 2023-09-10T23:13:33Z

Optimized the GPG init so it doesn't create files that don't need to stay there.

SlugFiller · 2023-09-11T14:54:45Z

Not meaning to rush anyone, but CI has finally passed, I've applied the suggested fixes (where applicable), and while there's a small mountain of fixes I want to add, none of them are specific to Windows, i.e. they should not be part of this PR. So, what is the next step? Waiting for testers? Reviewing the last few bits of code added?

romanz

No problem, and many thanks for the work!

gtbuchanan · 2023-09-11T18:38:31Z

@SlugFiller Thank you for running with this and actually preserving my contribution. I was obviously out of my comfort zone. GPG support is coming too? Nice work!

SlugFiller · 2023-09-11T22:26:12Z

@gtbuchanan While I ended up changing a lot, your contribution was a huge factor in being able to make this PR, or at the very least, make it this quickly (Less than a week from conception to merge). At a bare minimum, having a working named pipes with overlapped io implementation in pywin32 I could reference, got the hard part out of the way.

Pandapip1 · 2023-09-12T14:47:34Z

Thank you both! I already made a workaround involving WSL, but if it works natively that's even better (and presumably better-supported!)

Add Windows native SSH support

498093f

SlugFiller force-pushed the windows-support branch 2 times, most recently from e4e383a to 7eaee93 Compare September 7, 2023 21:02

romanz reviewed Sep 8, 2023

View reviewed changes

SlugFiller force-pushed the windows-support branch from 7eaee93 to 1227770 Compare September 8, 2023 16:11

SlugFiller force-pushed the windows-support branch from 1227770 to ba4d39e Compare September 9, 2023 04:32

SlugFiller force-pushed the windows-support branch from ba4d39e to 96cf4fd Compare September 9, 2023 06:30

SlugFiller force-pushed the windows-support branch 2 times, most recently from c26e285 to a280f02 Compare September 9, 2023 10:08

SlugFiller force-pushed the windows-support branch from a280f02 to 254da47 Compare September 9, 2023 16:20

SlugFiller force-pushed the windows-support branch 2 times, most recently from 67204d9 to e0cdad8 Compare September 9, 2023 17:48

romanz reviewed Sep 9, 2023

View reviewed changes

libagent/gpg/keyring.py Outdated Show resolved Hide resolved

libagent/util.py Outdated Show resolved Hide resolved

romanz reviewed Sep 9, 2023

View reviewed changes

libagent/win_server.py Show resolved Hide resolved

libagent/win_server.py Outdated Show resolved Hide resolved

libagent/win_server.py Show resolved Hide resolved

SlugFiller force-pushed the windows-support branch 2 times, most recently from 9f898f0 to b0a793d Compare September 9, 2023 19:46

SlugFiller force-pushed the windows-support branch from b0a793d to e16d321 Compare September 9, 2023 21:13

SlugFiller force-pushed the windows-support branch from 73d9484 to d0d0c83 Compare September 10, 2023 17:53

romanz reviewed Sep 10, 2023

View reviewed changes

SlugFiller force-pushed the windows-support branch 2 times, most recently from 39f7b62 to 93d0ce8 Compare September 10, 2023 19:15

SlugFiller force-pushed the windows-support branch from 93d0ce8 to 48c549d Compare September 10, 2023 19:19

SlugFiller force-pushed the windows-support branch from 48c549d to 98c6db9 Compare September 10, 2023 19:27

SlugFiller force-pushed the windows-support branch from 98c6db9 to e471108 Compare September 10, 2023 19:39

SlugFiller force-pushed the windows-support branch from e471108 to 6d5fa84 Compare September 10, 2023 20:18

Fix Windows compatibility issues and AGE and GPG support

fa6d80e

SlugFiller force-pushed the windows-support branch from 6d5fa84 to fa6d80e Compare September 10, 2023 23:05

romanz self-requested a review September 11, 2023 18:04

romanz approved these changes Sep 11, 2023

View reviewed changes

romanz merged commit 9345f28 into romanz:master Sep 11, 2023
5 checks passed

	serve_platform = serve_win if sys.platform == 'win32' else serve_unix
	serve_func = serve_win if sys.platform == 'win32' else serve_unix

	key_id, public_key = export_public_key(device_type, args)
	key_id, public_key_bytes = export_public_key(device_type, args)

Fix Windows compatibility issues and AGE support #436

Fix Windows compatibility issues and AGE support #436

Conversation

SlugFiller commented Sep 7, 2023

SlugFiller commented Sep 7, 2023

romanz commented Sep 8, 2023

romanz left a comment

Choose a reason for hiding this comment

romanz Sep 8, 2023

Choose a reason for hiding this comment

romanz commented Sep 8, 2023

SlugFiller commented Sep 8, 2023

romanz commented Sep 8, 2023

romanz commented Sep 8, 2023

Pandapip1 commented Sep 8, 2023

SlugFiller commented Sep 9, 2023

SlugFiller commented Sep 9, 2023

SlugFiller commented Sep 9, 2023

SlugFiller commented Sep 9, 2023

SlugFiller commented Sep 9, 2023

SlugFiller commented Sep 9, 2023

SlugFiller commented Sep 10, 2023

romanz commented Sep 10, 2023

romanz commented Sep 10, 2023

romanz Sep 10, 2023

Choose a reason for hiding this comment

romanz Sep 10, 2023

Choose a reason for hiding this comment

romanz Sep 10, 2023

Choose a reason for hiding this comment

SlugFiller Sep 10, 2023

Choose a reason for hiding this comment

romanz Sep 10, 2023

Choose a reason for hiding this comment

romanz Sep 10, 2023

Choose a reason for hiding this comment

romanz commented Sep 10, 2023

romanz commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 10, 2023

SlugFiller commented Sep 11, 2023

romanz left a comment

Choose a reason for hiding this comment

gtbuchanan commented Sep 11, 2023

SlugFiller commented Sep 11, 2023

Pandapip1 commented Sep 12, 2023