Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

export to ssh #323

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

Merged
tomato42 merged 1 commit into tlsfuzzer:master from pmazzini:ssh
Oct 4, 2023
Merged

export to ssh #323

tomato42 merged 1 commit into tlsfuzzer:master from pmazzini:ssh
Oct 4, 2023

Conversation

@pmazzini
Copy link
Contributor

@pmazzini pmazzini commented Sep 24, 2023
edited
Loading

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.

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...

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

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 reacted with thumbs up emoji

Copy link
Contributor Author

test coverage missing

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

Copy link
Contributor Author

Not sure about Ed448

Yeah, there is no support for Ed448 yet.

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 reacted with thumbs up emoji

Copy link
Contributor Author

pmazzini commented Oct 1, 2023

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

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

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 reacted with thumbs up emoji
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

Copy link
Contributor Author

pmazzini commented Oct 2, 2023

Use compat26_str in der.topem().

Copy link
Contributor Author

pmazzini commented Oct 3, 2023

More compat26_str.

Copy link
Contributor Author

pmazzini commented Oct 3, 2023

Fix typo.

Copy link
Contributor Author

pmazzini commented Oct 4, 2023

Any blocker for merging? :)

@tomato42 tomato42 merged commit eed49e2 into tlsfuzzer:master Oct 4, 2023
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 reacted with thumbs up emoji

@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

Reviewers

@tomato42 tomato42 tomato42 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /