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

Document lack of host Key validation #2

Closed
DanielleMaywood opened this issue Sep 19, 2024 · 8 comments
Closed

Document lack of host Key validation #2

DanielleMaywood opened this issue Sep 19, 2024 · 8 comments
Labels
planned Feature requests that I plan to implement soon

Comments

@DanielleMaywood
Copy link

I noticed you do not perform host key validation. Maybe this should be made more obvious to users without having to dig through the source?

sidekick/utils/utils.go

Lines 77 to 80 in b99b61b

HostKeyCallback: func(hostname string, remote net.Addr, key ssh.PublicKey) error {
// use OpenSSH's known_hosts file if you care about host validation
return nil
},

@MightyMoud
Copy link
Owner

Yup fair point. I honestly didn't expect anyone to actually use this...
Thanks for pointing that out. I'll address this soon.

@MightyMoud MightyMoud added the planned Feature requests that I plan to implement soon label Sep 20, 2024
@lpil
Copy link

lpil commented Sep 20, 2024

Forgive me, I'm not familiar with this library. What is one expected to do in this callback?

@DanielleMaywood
Copy link
Author

Forgive me, I'm not familiar with this library. What is one expected to do in this callback?

So you're expected to check if the key matches what you expect by checking the .ssh/known_hosts file or some other source of truth.

You can use a library like https://pkg.go.dev/golang.org/x/crypto/ssh/knownhosts or https://pkg.go.dev/github.com/skeema/knownhosts for making that easier.

@MightyMoud
Copy link
Owner

You both are too kind! 🙂

I understand how critical this is now after some research.
I'll open a PR tonight. I would be glad if you guys can review it to make sure I get this right.

Let me know if you're keen, I'll assign you then...

@DanielleMaywood
Copy link
Author

I'm more than happy to check it over if you want another set of eyes.

@MightyMoud
Copy link
Owner

Here we go - #9

@DanielleMaywood
Copy link
Author

Will get round to this later!

@MightyMoud
Copy link
Owner

No rush!
Let's chat in the PR if you have any comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
planned Feature requests that I plan to implement soon
Projects
None yet
Development

No branches or pull requests

3 participants