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

export to ssh #323

Merged
merged 1 commit into from
Oct 4, 2023
Merged

export to ssh #323

merged 1 commit into from
Oct 4, 2023

Conversation

pmazzini
Copy link
Contributor

@pmazzini pmazzini commented Sep 24, 2023

Initial support for exporting keys in the SSH format as requested in #111.
For now it only supports Ed25519 curves but it shouldn't be hard to add additional ones.

@tomato42
Copy link
Member

IIRC, openssh supports only 4 or 5 curves (P-256, P-384, P-521, Ed25519, Ed448), I see no reason not to support all of them...

src/ecdsa/ssh.py Outdated Show resolved Hide resolved
Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test coverage missing

@beldmit
Copy link

beldmit commented Sep 26, 2023

IIRC, openssh supports only 4 or 5 curves (P-256, P-384, P-521, Ed25519, Ed448), I see no reason not to support all of them...

Not sure about Ed448

@pmazzini
Copy link
Contributor Author

test coverage missing

I am happy to add some tests if you think the code is looking ok-ish.

@pmazzini
Copy link
Contributor Author

Not sure about Ed448

Yeah, there is no support for Ed448 yet.

@tomato42
Copy link
Member

test coverage missing

I am happy to add some tests if you think the code is looking ok-ish.

yes, the code looks ok-ish, but python 2.6 compat and test coverage is mandatory, I won't merge the code otherwise.

@pmazzini pmazzini requested a review from tomato42 October 1, 2023 21:09
@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 1, 2023

Added unit tests. CI for Py 2.x was working fine.

@tomato42
Copy link
Member

tomato42 commented Oct 1, 2023

you will need to use methods from https://github.com/tlsfuzzer/python-ecdsa/blob/master/src/ecdsa/_compat.py for py2 compat

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failures are relevant

@pmazzini pmazzini requested a review from tomato42 October 1, 2023 23:22
@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 1, 2023

Switched to compat int_to_bytes.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failures on Python 2 still.

Also, please note the code checks run: https://github.com/tlsfuzzer/python-ecdsa/actions/runs/6374029084/job/17310264458?pr=323

@pmazzini pmazzini requested a review from tomato42 October 2, 2023 13:27
src/ecdsa/ssh.py Fixed Show fixed Hide fixed
@pmazzini pmazzini requested a review from tomato42 October 2, 2023 19:06
@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 2, 2023

Switch to using der.topem() and fix it while on it.
Use a line break every 76 bytes: https://docs.python.org/3/library/base64.html#base64.encodebytes

@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 2, 2023

Use compat26_str in der.topem().

@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 3, 2023

More compat26_str.

src/ecdsa/ssh.py Fixed Show fixed Hide fixed
@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 3, 2023

Fix typo.

@pmazzini
Copy link
Contributor Author

pmazzini commented Oct 4, 2023

Any blocker for merging? :)

@tomato42 tomato42 merged commit eed49e2 into tlsfuzzer:master Oct 4, 2023
46 checks passed
@tomato42
Copy link
Member

tomato42 commented Oct 4, 2023

Any blocker for merging? :)

yes, my time to do a final review :)

thanks for the PR!

@pmazzini pmazzini deleted the ssh branch October 4, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants