-
-
Notifications
You must be signed in to change notification settings - Fork 769
Fix: Rebuild PEM format for RSA private keys before passing to Paramiko #6339
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
base: master
Are you sure you want to change the base?
Conversation
b033762
to
0c4f081
Compare
0c4f081
to
f2711a0
Compare
f2711a0
to
7161aa0
Compare
- Introduced _rebuild_pem() to ensure RSA keys are properly wrapped and contain correct headers. - Applied this only to RSA keys in _get_pkey_object(). - Ensures malformed single-line private keys (e.g., from env vars or API) no longer fail to load with Paramiko. - Added changelog entry under Bug Fixes.
7161aa0
to
94cd4d4
Compare
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.
I'm inclined to close this as "won't fix".
StackStorm should preserve user input exactly as it received it. If it is not, then we should fix that bug.
Changing user input like this brings a lot of risk to the StackStorm project.
First, it adds a lot of security risks. The security implications of mangling user input like this are staggering. If the key the user entered is not the key used, then I would consider that to be a significant security vulnerability that requires a security patch, possibly even requiring a CVE. I don't want to trigger security scanners that look for any changes to security sensitive files such as private keys. If the key cannot be shown to be exactly what the user provided then the reliability and provenance of that secret has been broken.
Second, mangling user input causes a maintenance nightmare for a project that, like so many other community-driven open source projects, is short handed. If users encounter errors logging in, and in debugging we find that StackStorm has changed the input somehow, then it is easy to stop there and just blame StackStorm instead of looking farther to understand why the key is not working. Anyone upset about StackStorm mangling input would be justified in their anger. We are likely to get regression reports of private keys that worked with paramiko before but break after making a change like this. Any time you mangle user input, you're asking for hidden, difficult to debug, bugs.
Third, the implementation in this PR introduces serious bugs. For example, the new method assumes that all private keys are RSA format, but, the callsite is in a loop that explicitly works with DSA and ECDSA, not just with RSA.
Fourth, security formats change over time. So there will probably be a new version of paramiko that supports a newer private key format. Any key-mangling logic would have to be updated to support whatever that new format is. Not only that, but whoever works on that would have to read RFCs and other specs to ensure they support all variations of that new format, or at least all that paramiko supports. We should leave this maintenance burden on paramiko keeping the glue code in st2 as minimal as possible.
Fifth, if you are running StackStorm in an environment where you need to support mangling user input, you have 3 options, that I can see, that are supported today: (1) create a wrapper workflow around the action that uses the key and add action(s) that clean up your typical user input before passing it on to the underlying action that needs that input. (2) Create a fork of the python action that uses the private key, subclassing the original action to add user input cleanup before passing the key to the original scroll. (3) Have the users use the st2 cli to load their private key, encrypted, in the datastore, and then have them paste {{ st2kv.user.my_private_key }}
.
Summary
This PR fixes an issue where private keys without proper PEM formatting (e.g., from environment variables or single-line inputs) would cause
paramiko.SSHException
due to incorrect formatting.Problem
When using
_get_pkey_object
, the code assumed that the provided private key string was already correctly PEM-formatted. However, in many automation scenarios, users may provide the key in a single line or malformed format.Changes Made
_rebuild_pem()
that:_rebuild_pem()
only when trying to parse an RSA key (i.e.,paramiko.RSAKey
)DSSKey
andECDSAKey
untouched to avoid incorrect formatting_rebuild_pem
(private) for clarityCHANGELOG.rst
under Bug FixesIssue
Fixes #6338