search

francoismichel / ssh3

SSH3: faster and rich secure shell using HTTP/3, checkout our article here: https://arxiv.org/abs/2312.08396 and our Internet-Draft: https://datatracker.ietf.org/doc/draft-michel-ssh3/
https://arxiv.org/abs/2312.08396
Apache License 2.0
3.18k stars 81 forks source link

draft: fix(util): match non-pointer public key types #48

Closed TheoTechnicguy closed 6 months ago

TheoTechnicguy commented 6 months ago

This PR aims to fix #38. Some pubkeys passed to the function are not pointers, but are still valid keys. This commit fixes this bug by adding the non-pointer types to the switch. Furthermore, more debugging log prints are added.

I am submitting this PR as a draft, as integration tests do not pass (see this comment).

francoismichel commented 6 months ago

Thank you for the PR!

It should now work in integration tests if you rebase your branch on main.

What would be nice as well is to add an integration test case with ed25519 similar to the It("Should connect using privkey" test case. I added an ed25519 in the CI scripts accessible in the integration tests through the TESTUSER_ED25519_PRIVKEY environment variable. Would you feel like adding a test case using that key as well ? I can work on that otherwise.

TheoTechnicguy commented 6 months ago

I agree that creating a test case for this is a good idea. I'd have to look up how to do that with Ginko, which might take a while. Furthermore, I am working on adding a bit more debug logging (draft PR coming up [edit: #58]), which will help to get better bug reports (also, I already know zerolog, so this is easier for me :yum:). Not wanting to discredit the value of testing, the latter one seems more important to me, as it will help with bug reports.

Depending on your judgment, I can hand over none, one or both PRs and/or reprioritize these.

francoismichel commented 6 months ago

Do it as you like the most, I can add the testcase on this PR myself as well, it would not take me much time so it is as you prefer. :-)

TheoTechnicguy commented 6 months ago

In that case, I'll extend the debug logging ^^.

francoismichel commented 6 months ago

Merged as 6127e06, thank you !