Repository navigation
feat(discovery): opt-in ssh_access exposes discovery scope as a dynamic ssh-proxy - #157
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces SSH access to discovered VMs by registering a sibling ssh-proxy datasource alongside the discovery-proxy datasource when the ssh_access flag is enabled. It includes a design specification, configuration parsing, validation logic, and comprehensive unit tests. The review feedback highlights a potential runtime panic in pkg/ws/handler.go if the datasource configuration is nil, and suggests improving IP/CIDR parsing robustness in pkg/proxy/discovery/ssh_access.go by converting bare IP addresses to single-host CIDRs.
dfbdfe9 to
fff2d50
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SSH access to discovered VMs by registering a dynamic-mode ssh-proxy sibling datasource alongside the main discovery-proxy datasource when ssh_access is enabled. It also refactors GCP Secret Manager credential loading to avoid deprecated APIs and updates bare IP parsing in the SSH proxy to treat them as single-host CIDRs. The review feedback correctly identifies a critical bug in the bare IP parsing logic where manually constructing a net.IPNet with a 16-byte IPv4 address and a 4-byte mask can cause IPNet.Contains to fail, and provides a clear code suggestion to resolve it by using ip.To4().
GO-2026-6348 (grpc) and GO-2026-6354/6355 (x/crypto) fail the vuln job on every PR. x/crypto 0.56.0 requires go 1.26, so the go directive moves to 1.26.6, matching the Dockerfile build image; 1.26.0 would pull in stdlib vulns fixed in later patches.
v2.7.2 is built with go1.25 and refuses to load a go 1.26 module. The newer staticcheck flags the deprecated DetectOptions.CredentialsFile (now loaded via NewCredentialsFromJSON with the file's declared type) and an SA6001 false positive in a test helper.
…ic ssh-proxy Discovery reaches hosts over SSH, but only for signed inventory packs, so nothing else could run a command on a discovered host. With ssh_access: true a discovery datasource also registers a sibling ssh-proxy (<id>:ssh) in dynamic mode, reusing its credentials, scope (allowed_cidrs -> allowed_hosts) and known_hosts_file. The sibling is refused unless signing, host key verification and an explicit scope are configured, since it turns inventory credentials into a shell. Also: - instantiate discovery-proxy from cloud config sync (it was skipped as an unknown proxy type) and advertise it in the greeting capabilities; - don't panic on a config sync entry without a config block; - treat bare IPs in ssh-proxy allowed_hosts as single-host networks, as discovery already does.
fff2d50 to
7ddd3ae
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an ssh_access feature for the discovery-proxy datasource, allowing the registration of a sibling ssh-proxy to run ad-hoc commands on in-scope hosts. It also updates GCP Secret Manager credentials loading to avoid deprecated options. The review feedback highlights several potential nil pointer dereference panics, specifically when calling verifier.Enabled() if the verifier is nil, and when accessing the configuration map cfg in SSHAccessEnabled and SSHAccessConfig if it is nil. Additionally, it is recommended to consistently filter out empty strings in the stringSlice helper for both []string and []any inputs.
Description
Discovery reaches hosts over SSH, but only to run signed inventory packs. Nothing else can run a command on a discovered host.
This PR adds an opt-in
ssh_accessflag on discovery datasources (off by default). When it is on, forager also registers a siblingssh-proxydatasource in dynamic mode:<id>:ssh, name<name>-ssh.allowed_cidrsbecome the sibling'sallowed_hosts.known_hosts_filefor host-key checks.params.host, and it must be inside that scope.Guards. The sibling doesn't start, and an error is logged, if any of these is missing:
signing_public_keyknown_hosts_fileallowed_cidrsThe flag turns credentials granted for inventory packs into a shell, so it gets a stricter bar than inventory.
Also in this PR
discovery-proxy. It used to be skipped as an unknown proxy type. The sibling is added or removed as the flag changes.configblock no longer panics.allowed_hostsare treated as single-host networks, as discovery already does.discovery-proxy.vulnpasses here whichever PR merges first.Type of change
How Has This Been Tested?
The new unit tests cover:
[]anyfrom cloud push;go test -race ./...,govulncheckandgolangci-lintall pass.Checklist
make validatepasses (fmt + lint + test)