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

intial unit test and impl for ssh-keygen simulator #1432

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

castedo
Copy link
Contributor

@castedo castedo commented Nov 14, 2024

  • unit test that check-novalidate subcmd rejects
  • basic CLI impl for ssh-keygen simulator
  • trivial always fail impl of check-novalidate subcmd

* unit test that check-novalidate subcmd rejects
* basic CLI impl for ssh-keygen simulator
* trivial always fail impl of check-novalidate subcmd
@castedo castedo requested a review from jelmer as a code owner November 14, 2024 12:46
@castedo castedo marked this pull request as draft November 14, 2024 12:47
@castedo
Copy link
Contributor Author

castedo commented Nov 14, 2024

Probably not worth merging yet since it doesn't actually to any check. So I've put it in draft mode. But good preview of what's to come soon so you can point out any improvements and changes that make sense.

@castedo
Copy link
Contributor Author

castedo commented Nov 14, 2024

@jelmer Pick your poison:

Soon I'm going to be adding a line of

from __future__ import annotations

from https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L1
so that a return type hint like:
https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L48
does not have to be quoted.

And I think there are few other benefits.

BUT if you think it's better to not __future__, just let me know and I'll quote some of these type hints and go back from the __future__.

@jelmer
Copy link
Owner

jelmer commented Nov 14, 2024

@jelmer Pick your poison:

Soon I'm going to be adding a line of

from __future__ import annotations

from https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L1
so that a return type hint like:
https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L48
does not have to be quoted.

And I think there are few other benefits.

BUT if you think it's better to not __future__, just let me know and I'll quote some of these type hints and go back from the __future__.

Why do we need that? Dulwich is python >= 3.9, so annotations shouldn't require a "from future import"

@castedo
Copy link
Contributor Author

castedo commented Nov 14, 2024

@jelmer Pick your poison:
Soon I'm going to be adding a line of

from __future__ import annotations

from https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L1
so that a return type hint like:
https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L48
does not have to be quoted.
And I think there are few other benefits.
BUT if you think it's better to not __future__, just let me know and I'll quote some of these type hints and go back from the __future__.

Why do we need that? Dulwich is python >= 3.9, so annotations shouldn't require a "from future import"

It is still needed for recursive/self-referential type hints. For instance, in

https://gitlab.com/perm.pub/hidos/-/blob/923691a51921de59250e80f7fd82fe40fa895637/hidos/sshsiglib/ssh_keygen.py#L48

the unquoted AllowedSigner return type hint is for a function of the class AllowedSigner. So even with 3.9, either the type hint needs to be quoted or the __future__ is needed.

@castedo
Copy link
Contributor Author

castedo commented Nov 14, 2024

I'm usually in a container with Python 3.12 and the choice still is needed in 3.12.

* strict mypy typing for ssh_keygen.py submodule
* enable optional ability to double check against real ssh-keygen exec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelmer You probably have a better idea than I do on the best way to handle this with unittest. This DOUBLE_CHECK_SSH_KEYGEN_SIMULATION flag will enable also running the real ssh-keygenalong with the main unit test, with the same test case input data, and make sure the exit return code is the same.

I'm thinking this double checking against the real ssh-keygen should be off by default. It's not really the point of the usual unittest. But it's also nice to sanity double check from time to time.

Makefile Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelmer I wasn't sure the best way to handle this mypy strictness. Mypy seems to be pretty annoying in terms of selectively enable strict mode (it doesn't really seem to be possible). But I've written all the sshsiglib code passing under strict mode. So seems a pity to not try and keep it type strict.

@castedo
Copy link
Contributor Author

castedo commented Nov 15, 2024

Interesting ... I just discovered another benefit to using from __future__ import annotations:

ruff requests using the X | Y type notation instead of Union and Option, which Python 3.9 barfs on (I just confirmed) ... but without __future__. With the __future__ Python 3.9 will ignore the pipe X | Y type notation. I'll start following ruffs suggestion by using __future__ to keep both ruff and Python 3.9 happy.

@castedo castedo marked this pull request as ready for review November 15, 2024 03:09
@castedo
Copy link
Contributor Author

castedo commented Nov 15, 2024

@jelmer OK, this is an actual implementation of ssh-keygen -Y check-novalidate, the core crypto functionality at the base of SSH key signing. This could be a good check point to merge, even though there's more to come, and some more extra sshsig features layerd on top that git uses (namely checking the allowed_signers file for the key found in the signature from this core stage).

@castedo
Copy link
Contributor Author

castedo commented Nov 15, 2024

I figured I would just throw all the code in to one file, to keep it simple within the bigger picture of dulwich. However, there are a number of natural splits and layers to this implementation. This single file could naturally be split into multiple and placed within a subdirectory/submodule for all this sshsig/ssh_keygen stuff.

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.

2 participants