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

parsing of ssh-keygen allowed signers file format #1473

Closed
wants to merge 1 commit into from

Conversation

castedo
Copy link
Contributor

@castedo castedo commented Jan 4, 2025

Implements low-level parsing for enhancement #1431

@castedo castedo requested a review from jelmer as a code owner January 4, 2025 21:43
@castedo castedo force-pushed the pr/allowed_signers branch from c2c1532 to 9d9e747 Compare January 4, 2025 21:54
@jelmer
Copy link
Owner

jelmer commented Jan 6, 2025

This is all ssh-specific (rather than Git) and the referenced source file is from openssh, any reason this shouldn't live in sshsig?

@castedo
Copy link
Contributor Author

castedo commented Jan 6, 2025

This is all ssh-specific (rather than Git) and the referenced source file is from openssh, any reason this shouldn't live in sshsig?

In #1394 (comment) you wrote:

The support for parsing allowed signers is git-specific and should be a part of dulwich, not sshsig. Applying the rules in such a file is also non-trivial and you'd want support from dulwich to apply them.

But I'm fine with this code living in sshsig if you prefer it not be in dulwich.

I agree it is ssh specific. The utilityssh-keygen is defining the format.

@jelmer
Copy link
Owner

jelmer commented Jan 7, 2025

This is all ssh-specific (rather than Git) and the referenced source file is from openssh, any reason this shouldn't live in sshsig?

In #1394 (comment) you wrote:

The support for parsing allowed signers is git-specific and should be a part of dulwich, not sshsig. Applying the rules in such a file is also non-trivial and you'd want support from dulwich to apply them.

But I'm fine with this code living in sshsig if you prefer it not be in dulwich.

I agree it is ssh specific. The utilityssh-keygen is defining the format.

Ah, sorry - to maybe make it a bit more nuanced: I think the parsing should be in sshsig and the application (which would be importing the parser from sshsig) in dulwich since that needs dulwich-specific context, if that makes sense? Maybe I am being overly cautious - we can always move the code later anyway.

@castedo
Copy link
Contributor Author

castedo commented Jan 7, 2025

Makes sense to me that dulwich, not sshsig, would have (yet-to-exist) code that replicates the logic of the git CLI as to where to find the allowed signers file based on git config files (and probably environment variables too).

Seems reasonable to me that sshsig contain the basic allowed signers parsing code that I've already coded (and am using in 'hidos' in a hidos specific way).

I've go ahead and close this PR and create a new one in the sshsig repo to have the sshsig package include allowed signer parsing code.

@castedo castedo closed this Jan 7, 2025
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