Skip to content
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

add support for new OpenSSH private key format #1343

Merged
merged 9 commits into from Dec 3, 2019

Conversation

@jaredhobbs
Copy link
Contributor

jaredhobbs commented Nov 28, 2018

This work is based off the work done in #618

jaredhobbs added 5 commits Nov 28, 2018
This work is based off the work done in #618
@ploxiln

This comment has been minimized.

Copy link
Contributor

ploxiln commented Nov 29, 2018

Is it not possible for the code supporting the new format in ed25519key.py to be removed, and for Ed25519Key to use the shared code now in PKey for that instead?

@jaredhobbs

This comment has been minimized.

Copy link
Contributor Author

jaredhobbs commented Nov 30, 2018

Probably. I'll investigate when I get some free time but I'm pretty swamped at the moment.

@thomasd3

This comment has been minimized.

Copy link

thomasd3 commented Dec 10, 2018

Is there any ETA for when the new format could be supported?

@carumusan

This comment has been minimized.

Copy link

carumusan commented Feb 12, 2019

Please merge this. Looks like since OpenSSH 7.8 (released 6 months ago 2018-08-24) the new format is the default. https://www.openssh.com/txt/release-7.8

@dgjustice

This comment has been minimized.

Copy link

dgjustice commented Mar 6, 2019

Bump! Fedora 29, openssh 7.9p1. Paramiko can't read my private key files without this change.

@bdrydyk

This comment has been minimized.

Copy link

bdrydyk commented Mar 7, 2019

Bumping this as well. This has broken my automation from OS X Mojave

@mitom

This comment has been minimized.

Copy link

mitom commented Mar 13, 2019

Is there anything blocking this that requires help? Lacking this is causing major issues for us.

@kindlehl

This comment has been minimized.

Copy link

kindlehl commented Mar 31, 2019

I'm currently having issues as well. Broken on Debian 9

@aedelbro

This comment has been minimized.

Copy link

aedelbro commented on f9d8c03 Apr 25, 2019

Would it not be better to make it a raw string and drop flake comment?
r"^-{5}BEGIN (RSA|DSA|EC|OPENSSH) PRIVATE KEY-{5}\s*$"
https://lintlyci.github.io/Flake8Rules/rules/W605.html

@jaredhobbs

This comment has been minimized.

Copy link
Contributor Author

jaredhobbs commented Apr 25, 2019

Would it not be better to make it a raw string and drop flake comment?
r"^-{5}BEGIN (RSA|DSA|EC|OPENSSH) PRIVATE KEY-{5}\s*$"
https://lintlyci.github.io/Flake8Rules/rules/W605.html

Yes, fixed.

@pgierz

This comment has been minimized.

Copy link

pgierz commented May 6, 2019

Any idea when this will go in?

elif pkformat == self.PRIVATE_KEY_FORMAT_OPENSSH:
curve, verkey, sigkey = self._uint32_cstruct_unpack(data, "sss")
try:
key = ec.derive_private_key(sigkey, curve, default_backend())

This comment has been minimized.

Copy link
@ploxiln

ploxiln May 22, 2019

Contributor

this doesn't work: sigkey needs to be an integer, and curve needs to be translated into one of the Curve objects in _ECDSA_CURVES

noticed while working on my port and cleanup of this. here's a test to show the errors:
https://github.com/ploxiln/paramiko-ng/pull/13/files#diff-523fff3457d80985c9d2429eb3aecb6e
the fix is mixed into my large refactor, since I figured this out towards the end:
https://github.com/ploxiln/paramiko-ng/pull/13/files#diff-00344f8a080e03112e93210cdfd37302


# Remove padding
padlen = byte_ord(keydata[len(keydata) - 1])
return keydata[: len(keydata) - padlen]

This comment has been minimized.

Copy link
@ploxiln

ploxiln May 22, 2019

Contributor

this unpad suffers from roughly the same bug I fixed for Ed25519Key in #1400

@greenscar

This comment has been minimized.

Copy link

greenscar commented Jun 17, 2019

any movement? We all know Apple is the new Microsoft, doing everything their own way. However, It kinda sucks when I can't run Ansible / Testinfra from my mac box due to
paramiko.ssh_exception.SSHException: not a valid RSA private key file"

@newearthmartin

This comment has been minimized.

Copy link

newearthmartin commented Aug 23, 2019

Can someone please merge this? It has been open for months. Fabric/paramiko is failing on mac.

@joaquinclearmetal

This comment has been minimized.

Copy link

joaquinclearmetal commented Aug 25, 2019

It would be nice if Paramiko supported SSH. I have to recreate keys if they were created on macOS (and apparently this affects Fedora).

@ChrisEdgington

This comment has been minimized.

Copy link

ChrisEdgington commented Oct 7, 2019

I was able to fix this problem by converting my macos id_rsa to pem format via:

ssh-keygen -p -m PEM -f ~/.ssh/id_rsa

@rmelotte rmelotte mentioned this pull request Oct 9, 2019
@cbonitz

This comment has been minimized.

Copy link

cbonitz commented Oct 16, 2019

OpenSSH formatted private keys seem to be the default on Ubuntu 19.04, just like on MacOS and Fedora as mentioned by previous commenters. This is not an edge case anymore, this will concern almost all users of unix-like operating systems very soon.

@XVilka

This comment has been minimized.

Copy link

XVilka commented Nov 7, 2019

Would be nice to have this finally merged.

@jamesharris-garmin

This comment has been minimized.

Copy link

jamesharris-garmin commented Nov 25, 2019

Could we please just run black on pkey and resolve the tests on tests/test_pkey.py?

@bitprophet bitprophet added this to the p0 milestone Nov 25, 2019
@paramiko paramiko deleted a comment from XVilka Dec 3, 2019
@paramiko paramiko deleted a comment from jamesharris-garmin Dec 3, 2019
@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Dec 3, 2019

Diving into reviewing/tweaking/merging this now. Thanks for everyone who's been keeping the patchset rolling, and for everyone's patience 🙏

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Dec 3, 2019

Merged to latest master & blackened, taking a look:

  • No changelog? 😢 Added an entry and linked to most of the major related tickets (sob, there are so many) including crediting a pile of folks ❤️
  • This code only seems to support BEGIN OPENSSH PRIVATE KEY style heads and not "real" RFC 4716 style headers which are BEGIN SSH2 PRIVATE KEY.
    • However, since ssh-keygen only appears to spit out the former header style, and given I'm not experienced with these formats yet myself, I'm going to leave this be.
      • Suspect OpenSSH departed from the RFC afterwards for whatever reason, and this problem domain (OpenSSH compatibility) is definitely one where practice trumps theory 😂
    • Should be pretty easy to extend it later for the variant if folks need it.
  • Renamed the new keys a bit, it wasn't clear to me offhand why they were named <size>_o.key, guessing o for OpenSSH, but I like obvious naming...
  • Not a huge fan of the (as far as I can tell) internal-only format signifier constants (eg PKey.PRIVATE_KEY_FORMAT_ORIGINAL) but it's not a great use of my time to clean things up to that level, so I settled for:
    • making them private so they're not part of the public API and can be tweaked later
    • updating the exceptions raised when they go off the rails (they were not consistent or terrifically readable).
      • also I'm honestly not even sure how we can end up in that particular code branch (the code that spits out the pkformat value would raise its own exception first!) but again, going to avoid rocking the boat too much for now, in case I'm misreading this.
  • Renamed references to "old/new format" as "pem/openssh" to be clearer (to me, anyway)
  • Added tests for the exception behavior around invalid key types (this is, AFAICT, an original sin, but this is a great time to add new tests for it) Never mind, this stuff is still too hairy to mess with outside of a #387 style overhaul
  • It's not clear to me why PKey._uint32_cstruct_unpack does a blanket "any exception is transmuted into an equivalent SSHException". I don't recall this being any sort of contract of Paramiko's behavior, and if anything, we are trying to dial down the abuse of "everything is this one exception class" from the old days. All this does it make it harder to tell quite what broke?
    • After more digging: this is likely because a bunch of mid-level code such as SSHClient attempts to ask the question "did code that reads SSH keys have a problem with a given key? Ignore that for now and move on to the next auth method".
    • I really don't like this (that code should arguably just be doing a bare except, as that is one of the few acceptable use cases for such) but it is an established pattern, just one I'd not been super aware of. Something to axe in #387 too probably. Settled for a comment for now.
  • Cleaned up some "foo" + str(val) + "bar" style string interpolation. Folks, it's 2019, we have .format() and (once we drop Python <3.5...in, probably, 2030...) f-strings now. (This is admittedly something around the codebase from its 2004 days, as well...got some of those too...)
  • Did some final real world manual tests with new keys generated on my (macOS Mojave) system's ssh-keygen.
@bitprophet bitprophet merged commit 25de1a7 into paramiko:master Dec 3, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Dec 3, 2019

This will be out in Paramiko 2.7 (only one more ticket after this one before that's cut!)

@grant-humphries

This comment has been minimized.

Copy link

grant-humphries commented Dec 3, 2019

@bitprophet thank you for your work on this!!

@jaredhobbs

This comment has been minimized.

Copy link
Contributor Author

jaredhobbs commented Dec 3, 2019

thanks so much!

@johnnybubonic

This comment has been minimized.

Copy link

johnnybubonic commented Dec 3, 2019

yay

please don't forget about #1136 and #1261 too! they should be able to be resolved now that this format's supported.

@bitprophet

This comment has been minimized.

Copy link
Member

bitprophet commented Dec 3, 2019

@johnnybubonic I put 'em in a milestone, I can't say when they'll be gotten to but at least they're on my radar now, thank you!

intgr added a commit to intgr/paramiko that referenced this pull request Dec 11, 2019
The Ed25519Key class contained key parsing and decryption logic for
OPENSSH-format keys before that code became generic. Now that paramiko#1343 is
merged, this logic becomes redundant and can be removed.

This cleanup was extracted from
ploxiln#13 and the original author
@ploxiln is credited even though I (@intgr) am submitting this pull
request.

Signed-off-by: Marti Raudsepp <marti@juffo.org>
intgr added a commit to intgr/paramiko that referenced this pull request Dec 11, 2019
The Ed25519Key class contained key parsing and decryption logic for
OPENSSH-format keys before that code became generic. Now that paramiko#1343 is
merged, this logic becomes redundant and can be removed.

This cleanup was extracted from
ploxiln#13 and the original author
@ploxiln is credited as commit author even though I (@intgr) am
submitting this pull request.

Signed-off-by: Marti Raudsepp <marti@juffo.org>
intgr added a commit to intgr/paramiko that referenced this pull request Dec 11, 2019
The Ed25519Key class contained key parsing and decryption logic for
OPENSSH-format keys before that code became generic. Now that paramiko#1343 is
merged, this logic becomes redundant and can be removed.

This cleanup was extracted from
ploxiln#13 and the original author
@ploxiln is credited as commit author even though I (@intgr) am
submitting this pull request.

Signed-off-by: Marti Raudsepp <marti@juffo.org>
intgr added a commit to intgr/paramiko that referenced this pull request Dec 11, 2019
The Ed25519Key class contained key parsing and decryption logic for
OPENSSH-format keys before that code became generic. Now that paramiko#1343 is
merged, this logic becomes redundant and can be removed.

This cleanup was extracted from
ploxiln#13 and the original author
@ploxiln is credited as commit author even though I (@intgr) am
submitting this pull request.

Signed-off-by: Marti Raudsepp <marti@juffo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.