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

Add support for SSH certs #33

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Add support for SSH certs #33

merged 5 commits into from
Apr 15, 2024

Conversation

wiktor-k
Copy link
Owner

@wiktor-k wiktor-k commented Apr 4, 2024

This doesn't work but illustrates the way parsing KeyData from binary AddIdentity request with cert fails.

@wiktor-k wiktor-k mentioned this pull request Apr 4, 2024
@jcspencer
Copy link
Collaborator

jcspencer commented Apr 4, 2024

It appears these keys are treated differently based on the algorithm in OpenSSH, see here.

ssh-key has two different types here - I don't think you can use KeyData to store certs; instead there is a Certificate type.

Perhaps the algorithm should be read first, and used to determine if the key delivered is a certificate or a raw key - and wrap them both in an enum?

e.g.:

pub enum KeyType {
  Key(KeyData),
  Certificate(Certificate)
}

@wiktor-k
Copy link
Owner Author

wiktor-k commented Apr 4, 2024

Yep, exactly. I tried to use KeyData which seem to have support for unknown types: #3 (comment)

Your suggestion to use an enum may be exactly what's needed 👌

As I mentioned in the other comment SSH binary format is not regular and one can't parse it without knowing the schema and the specs are sometimes broken (just yesterday I found reserved values I'm restrict destination constraints that were in the source but not in the spec).

@baloo baloo force-pushed the wiktor/add-certs branch from 601fe5a to f7f159a Compare April 5, 2024 05:45
@baloo
Copy link
Collaborator

baloo commented Apr 5, 2024

This last commit should fix parsing, but this requires a fix upstream to expose a function:
https://github.com/RustCrypto/SSH/compare/cb2706f118c8a26b185c951ba5818abfb1352cb6...baloo:SSH:baloo/ssh-key/fixes?expand=1

I wouldn't mind if we could put that on the back-burner, get those fixes upstream, and work slowly.

@wiktor-k wiktor-k force-pushed the wiktor/migrate-to-rustcrypto branch from e2b879a to a1c0beb Compare April 5, 2024 06:45
Base automatically changed from wiktor/migrate-to-rustcrypto to main April 5, 2024 06:49
@wiktor-k
Copy link
Owner Author

wiktor-k commented Apr 5, 2024

Just FYI after merging a ton of PRs (some of them rebased) I've cleaned up this PR by basically rebasing on top of main and leaving just two commits: my, adding test vectors, and @baloo's which implements.

I wouldn't mind if we could put that on the back-burner, get those fixes upstream, and work slowly.

I think this is a good idea and thanks a lot for working on it! 🙇

src/proto/message.rs Outdated Show resolved Hide resolved
@baloo baloo force-pushed the wiktor/add-certs branch from d0d4de9 to 20725a2 Compare April 12, 2024 21:50
@baloo baloo marked this pull request as ready for review April 12, 2024 21:50
@baloo baloo force-pushed the wiktor/add-certs branch from 20725a2 to b2d807b Compare April 12, 2024 22:55
Copy link
Owner Author

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

This looks just great 👌

I'll want to take a look at it on my computer but I think it's good to be merged. Great job 👏

src/proto/privatekey.rs Outdated Show resolved Hide resolved
tests/sign-and-verify.sh Outdated Show resolved Hide resolved
tests/sign-and-verify.sh Show resolved Hide resolved
wiktor-k and others added 2 commits April 13, 2024 13:50
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
Signed-off-by: Arthur Gautier <[email protected]>
@baloo baloo force-pushed the wiktor/add-certs branch from ac91625 to 56dc974 Compare April 13, 2024 20:51
@baloo baloo changed the title Broken: Add support for SSH certs Add support for SSH certs Apr 13, 2024
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k
Copy link
Owner Author

wiktor-k commented Apr 15, 2024

Okay, I've fixed a couple of small issues I've found and added test case for AddIdentityConstrained with multiple extensions (using my FIDO key and destination constraints). It seems to be working well.

I'll approve this PR and if @baloo doesn't have anything against I propose we merge it :)

Edit: I forgot formally I'm the "author" and cannot approve it... lol :)

@baloo baloo merged commit 0bbe3c7 into main Apr 15, 2024
18 checks passed
@baloo
Copy link
Collaborator

baloo commented Apr 15, 2024

ship it

@baloo baloo deleted the wiktor/add-certs branch April 15, 2024 16:03
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