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

Support allowed signers #7

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Support allowed signers #7

wants to merge 1 commit into from

Conversation

hiddeco
Copy link
Owner

@hiddeco hiddeco commented Mar 24, 2023

This just adds the lowest level API to match the golang.org/x/crypto/ssh package.

The library itself should offer a nicer API addition to also provide proper verification while utilizing the principals and options from the allowed_signers file.

To-do

  • Tests
  • Further API abstraction around verification, and working with principals in general

@hiddeco hiddeco added the enhancement New feature or request label Mar 24, 2023
This just adds the lowest level API to match the
golang.org/x/crypto/ssh package.

The library itself should offer a nicer API addition to also provide
proper verification while utilizing the principals and options from the
`allowed_signers` file.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@tionis
Copy link

tionis commented Sep 24, 2023

Is there a reason why this was never merged?

@hiddeco
Copy link
Owner Author

hiddeco commented Sep 25, 2023

It hasn't been finalized, and I need to find time to do this.

What I recall from the top of my head: while this allows parsing of the file, by itself it would be insufficient to support principals in full, which would require additional logic. For example to support wild cards, and some other features which an allowed_signers file facilities.

@tionis
Copy link

tionis commented Sep 25, 2023

I see, I'll take a look at it next week and maybe shoot a PR your way then.
I'm currently building a small signing infrastructure on top of ssh sigs (+ certificates).

@tionis
Copy link

tionis commented Oct 7, 2023

Ok, I implemented it over at my own tool:
https://github.com/tionis/ssh-tools/blob/a835cb13df0c310e236cd3fb3bb1003e526a4432/allowed_signers/main.go
This still needs some cleanup before I would merge it here. Also, my implementation allows comments even though they are not allowed by the spec (OpenSSH does still parse the file with them though)
This should probably be removed for this library.
Also, some integration with cert revocation files could later be added.

@hiddeco
Copy link
Owner Author

hiddeco commented Oct 12, 2023

Also, my implementation allows comments even though they are not allowed by the spec (OpenSSH does still parse the file with them though)

The implementation in this PR should also successfully parse the file, but does not provide them as output. Which I think is the correct thing to do, as we strive to follow the specification as close as possible within this library except if it breaks integration with more loose implementations (which shouldn't be the case for something that is practically metadata).

I will try to find some time soon (probably next week) to get this PR landed without the further abstractions, as I think it should be placed somewhere else e.g. ../crypto/ssh (or util, but that's an awful package name) to make room for the abstractions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants