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

fix: insecure host key #254

Closed
wants to merge 1 commit into from
Closed

fix: insecure host key #254

wants to merge 1 commit into from

Conversation

tenthirtyam
Copy link
Collaborator

Summary

Enhancements to the EsxiDriver in the builder/vmware/common/driver_esxi.go file to improve security by using known host key verification. The most important changes involve importing the knownhosts package and updating the SSH connection configuration to use a known hosts file.

  • Imported the knownhosts package to enable host key verification.
  • Updated the connect function to use a known hosts file for host key verification instead of ignoring host keys.

Reference

https://github.com/hashicorp/packer-plugin-vmware/security/code-scanning/7

Uses the knownhosts package to validate the host key.

Signed-off-by: Ryan Johnson <[email protected]>
@tenthirtyam tenthirtyam added bug security Security issues/fixes. labels Oct 15, 2024
@tenthirtyam tenthirtyam added this to the v1.1.1 milestone Oct 15, 2024
@tenthirtyam tenthirtyam self-assigned this Oct 15, 2024
@tenthirtyam tenthirtyam requested a review from a team as a code owner October 15, 2024 02:53
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hey @tenthirtyam,

I understand the rationale for not vetting any host here, but I wonder if this won't cause more harm than good, I might be missing something though so please feel free to pushback on my comments, I'd love to hear if I'm mistaken!

@@ -747,12 +748,18 @@ func (d *EsxiDriver) connect() error {
auth = append(auth, gossh.PublicKeys(signer))
}

knownHostsFile := filepath.Join(os.Getenv("HOME"), ".ssh", "known_hosts")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think this is not portable? AFAIK HOME is only set in UNIX environments, so if you're building on a Windows host that'll probably not work, will it?

knownHostsFile := filepath.Join(os.Getenv("HOME"), ".ssh", "known_hosts")
hostKeyCallback, err := knownhosts.New(knownHostsFile)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this, this means that if I don't have a local hosts file the builder will fail?
Since Packer implements its own SSH code instead of relying on OpenSSH or alternatives, it is conceivable that host machines won't have one, so we might break their builds by merging this I'm afraid

sshConfig := &ssh.Config{
Connection: ssh.ConnectFunc("tcp", address),
SSHConfig: &gossh.ClientConfig{
User: d.Username,
Auth: auth,
HostKeyCallback: gossh.InsecureIgnoreHostKey(),
HostKeyCallback: hostKeyCallback,
Copy link
Contributor

Choose a reason for hiding this comment

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

That I wonder, since the machine we're connecting to is a temporary VM, we probably don't have an entry for it in the local known_hosts, won't that prevent us from connecting later?

@tenthirtyam tenthirtyam marked this pull request as draft November 8, 2024 18:13
@tenthirtyam tenthirtyam deleted the fix/insecure-host-key branch November 13, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug security Security issues/fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants