Improve gpg subprocess timeout and stop using custom process module #502
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukpueh
force-pushed
the
gpg-subprocess-timeout
branch
from
5845eba
to
6199f46
Compare
jku
approved these changes
Feb 10, 2023
securesystemslib/gpg/constants.py
Outdated
Comment on lines
37
to
38
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, |
There was a problem hiding this comment.
Suggested change
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| capture_output=True, |
This nit might not get you back to a one liner, but it is shorter... I think this applies to all subprocess.run() calls in the pr
Exactly. I'd have to refactor how Also, I wasn't sure how we want to treat gpg commands specified in an environment variable. Maybe they include some custom flag, which would behave differently if we stopped using shlex. But maybe that's okay. 🤷 I mentioned in the commit message that I'm just trying to replicate the original behavior. I can also add a comment in the code. |
Not strictly needed IMO, I think the option and env var handling issues should be pretty obvious to anyone who does try to improve this |
Prior to this commit, settings.SUBPROCESS_TIMEOUT=3 (seconds) was used as default and also to configure timeouts for any subprocess spawned via the securesystemslib process interface, including running the gpg command to sign, export keys and check version. The small timeout regularly lead to test failures on slow test runners. Furthermore, configuring the timeout via global is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). This patch defines a custom constant, to be used only for gpg subprocesses and increases its value to 10 seconds. To override this default, an optional timeout argument will be added to relevant gpg interfaces in a subsequent commit. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Prior to this change, configuring timeouts for gpg required patching the default-specifying global. This is bad practice and error-prone (see secure-systems-lab#219, secure-systems-lab#345). To allow overriding default timeout in an intuitive way, this commit adds optional arguments to the relevant gpg interface functions. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
force-pushed
the
gpg-subprocess-timeout
branch
from
6199f46
to
f23d800
Compare
This commit replaces calls to the custom process.run function, in
favor of the stdlib subprocess.run.
The former is a very thin wrapper around the latter, which was more
useful when we still needed Py2/3 case-handling. Which is not the
case anymore.
NOTE: This commit replicates the prior behavior, most notably the
following subprocess.run arguments:
- *popenargs: we still use shlex.split to turn the full command
string into a list
- check
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
They are equivalent but the former is shorter: https://docs.python.org/3/library/subprocess.html#subprocess.run Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
force-pushed
the
gpg-subprocess-timeout
branch
from
f23d800
to
ac413ba
Compare
CI passes. Merging... |
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this pull request
Feb 10, 2023
This module was originally developed in in-toto and transferred to securesystemslib in secure-systems-lab#174, primarily as Py2/Py3-agnostic wrapper around stdlib's `subprocess.run`, to to execute `gpg`. In secure-systems-lab#502 we switched to using `subprocess.run` directly. Another wrapper around `subprocess.run`, provided by the module, allows capturing standard streams but still write them to a terminal. It was developed as specific `in-toto-run` feature and does not need to be public API in sslib. in-toto/in-toto#544 moves the function back to in-toto. closes secure-systems-lab#345, transfers secure-systems-lab#337 (to in-toto) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
lukpueh
added a commit
to lukpueh/securesystemslib
that referenced
this pull request
Feb 10, 2023
This module was originally developed in in-toto and transferred to securesystemslib in secure-systems-lab#174, primarily as Py2/Py3-agnostic wrapper around stdlib's `subprocess.run`, to run the `gpg` command. In secure-systems-lab#502 we switched to using `subprocess.run` directly. Another wrapper around `subprocess.run`, provided by the module, allows capturing standard streams AND write them to a terminal. It was developed as specific `in-toto-run` feature and does not need to be public API in sslib. in-toto/in-toto#544 moves the function back to in-toto. closes secure-systems-lab#345, transfers secure-systems-lab#337 (to in-toto) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the changes being introduced by the pull request:
gpg interface function to sign and export public keys now have an optional subprocess timeout. This means, users no longer need to patch a global in order to configure this value. Moreover, the default timeout is now defined inside the gpg subpackage, and increased to 10.
--> The should reduce test failures on slow runners, and makes it more intuitive to override the default.
Internally the gpg subpackage now no longer uses the custom subprocess wrapper, which provides very little benefit over calling directly into stdlib
subprocess.--> This is in preparation for deprecation of the process module
see commit message for details
Please verify and check that the pull request fulfils the following requirements: