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 cert authorities #8

Closed

Conversation

Javier-varez
Copy link
Contributor

Handles the issue described in #7 by actually looking up the known_hosts file to detect @cert-authority markers in the given line for the given host.

This solution feels a bit hacky, but given the API provided by the upstream crypto/ssh package, I could not find a better alternative.

@evanelias
Copy link
Contributor

Thank you for the pull request, but I cannot merge this approach for two reasons:

  • This PR changes the return type of HostKeyCallback.HostKeys(). Sorry but that's a breaking change, which is a non-starter. Please understand this package is imported by over 6500 other repos on GitHub and those will break if you change any exported type or function signature here.

  • This package intentionally does not reimplement any knownhosts file parsing, as stated in the README and in Certificate in known_hosts #7, but your approach here relies on doing that, and (if I'm reading this correctly) re-reading the file N times if it contains N keys. I don't want this package to read the knownhosts file any extra times at all, beyond what is done in x/crypto/ssh/knownhosts. As I mentioned in Certificate in known_hosts #7, I suspect this means that issue cannot possibly be solved today. We need x/crypto/ssh/knownhosts to expose more fields for this to be possible.

@evanelias evanelias closed this Jun 17, 2024
@evanelias
Copy link
Contributor

In retrospect I closed this too hastily. I should have provided a deeper explanation of why re-parsing the file is undesirable, and given space to discuss and explore whether those concerns can be mitigated. Please accept my apologies. I had a rough month, but it's no excuse.

Re-opening this PR now, but still pondering a few topics which will affect the solution to #7

Avoiding breaking changes:
Ideally I'm very hesitant to bump to a v2 unless absolutely necessary. So if we can find a way to not change any exported types/signatures, that would certainly be preferable. But it is certainly tricky with HostKeyCallback just being the same as ssh.HostKeyCallback, instead of being a struct that can store additional fields.

Avoiding re-parsing the knownhosts file:
My thinking is that it's always better to avoid re-implementing any core logic from x/crypto/ssh/knownhosts. One of the selling points of skeema/knownhosts is that it's a thin wrapper, leaving the core security-minded logic to x/crypto/ssh/knownhosts. In theory there are also performance considerations, but the file should generally already be in the filesystem cache if it was just read, so that's not too major.

That said, perhaps we should allow re-parsing the file a single time for implementing features like #7 which x/crypto/ssh/knownhosts doesn't provide, as long as we also provide a mechanism to disable that functionality. That way, this package could still be used in contexts which strictly prefer not re-parsing the file at all.

Test coverage:
Since this is a security-related repo, we should probably be very strict about test coverage. But I still need to add a CONTRIBUTING.md to this repo, which spells out requirements on test coverage for PRs, and provides better direction on PRs here in general. I'll aim to get that written soon.

Anyway, I need to give this more thought before I can suggest anything concrete on next step for this PR. Open to feedback on these topics.

@evanelias
Copy link
Contributor

I created a new PR #9 which builds on this one and adds an additional commit to retain backwards compatibility. Closing this one, but if/when #9 is merged it will be done via merge commit, so that your original commit here is properly tracked and you'll be credited as a contributor properly by GitHub.

@evanelias evanelias closed this Jul 8, 2024
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