[-] keep the Hetzner API password out of the process list - #425
Open
pashagolub wants to merge 5 commits into
Open
pashagolub wants to merge 5 commits into
pashagolub wants to merge 5 commits into
Conversation
pashagolub
force-pushed
the
fix/hetzner-credentials-exposure
branch
from
September 11, 2026 15:34
7c08901 to
e9fc979
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain with quote parsing, proxy support, and IPv4 transport coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Replaces curl-based Hetzner API calls with an IPv4-only Go HTTP client and improves credential handling.
Changes:
- Uses HTTP Basic Authentication.
- Supports flexible credential formats.
- Warns about insecure credential-file permissions.
- Expands API and integration tests.
File summaries
| File | Summary |
|---|---|
README.md |
Documents credential-file permission guidance. |
ipmanager/hetznerConfigurer.go |
Implements HTTP access, credential parsing, and permission warnings. |
ipmanager/hetznerConfigurer_test.go |
Tests HTTP behavior, parsing, errors, permissions, and edge cases. |
Review details
Suppressed comments (2)
ipmanager/hetznerConfigurer.go:68
- This custom transport leaves
Proxynil, sonet/httpwill bypassHTTPS_PROXY/ALL_PROXYeven though the previouscurlinvocation honored those environment settings. Deployments that reach the Robot API only through an outbound proxy will now fail; preserve the existing behavior by settingProxy: http.ProxyFromEnvironmentalongside the IPv4 dialer.
Transport: &http.Transport{
DialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
ipmanager/hetznerConfigurer.go:72
- The new transport is the part that replaces
curl --ipv4, but all HTTP stubs bind to an IPv4 loopback address, so these tests pass even if the IPv4 pinning is removed or broken. Add a transport-level assertion that the dialer receives/usestcp4(or an equivalent dual-stack test) so a future change cannot silently make the Robot API resolve over IPv6.
DialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
if network == "tcp" || network == "tcp6" {
network = "tcp4"
}
return dialer.DialContext(ctx, network, address)
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if len(line) > 6 { | ||
| password = line[6 : len(line)-1] | ||
| } | ||
| value = strings.Trim(strings.TrimSpace(value), `"'`) |
The Robot API was queried by shelling out to "curl --ipv4 -u user:password", so the password stood in the command line of that process for as long as it ran. /proc/<pid>/cmdline is readable by every local user, and process accounting and audit logs pick command lines up as well. The API is queried with net/http now and the credentials travel in the Authorization header. The reason given for curl - that selecting IPv4, which is all the Robot API listens on, is not trivial in Go - no longer holds: the transport uses a dialer that turns "tcp" into "tcp4". That also removes curl as a runtime dependency of a component that has to work during a failover, and the answer of the API is no longer read through a shell. While at it: * the credentials are parsed as key=value instead of by offset. The old code needed exactly user="value" and silently dropped the last character of a value that was not quoted, which failed later as an unexplained authentication error. Spaces around the equals sign, single quotes and the long forms username/password are accepted now. * a credentials file that is readable by group or others is reported. It is a warning, not a refusal, so that an existing installation keeps working.
The answer of the Robot API was taken apart with unchecked type assertions,
so anything that did not look exactly as expected took the whole process down
- in the middle of a failover, which is the one moment when it has to keep
running. A real error answer of the API reaches this: only one that carries
status, code and message together survived, and "{"error":{"code":...}}"
without a status was enough to panic on a nil interface conversion.
Every field is read with a checked assertion now and an answer that does not
fit is reported as an error, which the callers already handle by keeping the
cached state at unknown. The test that pinned the panic down as expected
behaviour asserts an error instead.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
pashagolub
force-pushed
the
fix/hetzner-credentials-exposure
branch
from
September 14, 2026 15:25
69fcb95 to
8cbfc71
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Robot API was queried by shelling out to "curl --ipv4 -u user:password",
so the password stood in the command line of that process for as long as it
ran. /proc//cmdline is readable by every local user, and process
accounting and audit logs pick command lines up as well.
The API is queried with net/http now and the credentials travel in the
Authorization header. The reason given for curl - that selecting IPv4, which
is all the Robot API listens on, is not trivial in Go - no longer holds: the
transport uses a dialer that turns "tcp" into "tcp4". That also removes curl
as a runtime dependency of a component that has to work during a failover, and
the answer of the API is no longer read through a shell.
While at it:
the credentials are parsed as key=value instead of by offset. The old code
needed exactly user="value" and silently dropped the last character of a
value that was not quoted, which failed later as an unexplained
authentication error. Spaces around the equals sign, single quotes and the
long forms username/password are accepted now.
a credentials file that is readable by group or others is reported. It is a
warning, not a refusal, so that an existing installation keeps working.