Closed Javier-varez closed 4 months ago
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 #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 #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.
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.
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.
Handles the issue described in https://github.com/skeema/knownhosts/issues/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.