Conversation
…ding the key's password
…ecure and ServerName methods
WalkthroughCLI now forwards a private-key password env var into certinfo; certinfo APIs were refactored to accept writers and return errors, TLS endpoint parsing and remote cert retrieval were reworked, style output was writer-ized, and tests were converted to table-driven form with a TLS-capable test server helper. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as cmd/certinfo
participant Env as Environment
participant FS as FileReader
participant Cert as internal/certinfo
participant Remote as TLS Endpoint
participant Out as Writer (stdout)
CLI->>Env: read CERTINFO_PKEY_PW
CLI->>Cert: call SetPrivateKeyFromFile(keyFile, keyPwEnvVar, FS)
Cert->>FS: read key file
FS-->>Cert: key bytes / read error
Cert->>Cert: decrypt/parse key (uses env var via keyPwEnvVar)
CLI->>Cert: call SetTLSEndpoint(host:port)
Cert->>Remote: connect TLS (GetRemoteCerts)
Remote-->>Cert: certs / TLS error
CLI->>Cert: call PrintData(Out)
Cert->>Out: write formatted output (errors propagated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmd/certinfo.go (1)
19-19: Consider exporting the constant frominternal/certinfoto avoid duplication.The same value
"CERTINFO_PKEY_PW"is defined here and asprivateKeyPwEnvVarininternal/certinfo/certinfo.go. If the env var name changes, both locations need updating.Option 1: Export the constant from
internal/certinfo:// In internal/certinfo/certinfo.go - privateKeyPwEnvVar = "CERTINFO_PKEY_PW" + PrivateKeyPwEnvVar = "CERTINFO_PKEY_PW"Then use it in cmd:
keyPwEnvVar = certinfo.PrivateKeyPwEnvVarOption 2: Keep the duplication but add a comment referencing the other location to prevent drift.
internal/certinfo/certinfo_test.go (2)
11-18: ThereadErrorfield is unused; simplify the mock.The
readErrorfield is assigned insideReadFilebut never read from outside. The mock can be simplified.-type mockReader struct { - readError error -} +type mockReader struct{} func (mr mockReader) ReadFile(name string) ([]byte, error) { - mr.readError = fmt.Errorf("unable to read file %s", name) - return nil, mr.readError + return nil, fmt.Errorf("unable to read file %s", name) }
153-160: Redundant checks:wantPool.Equal()andcmp.Difftest the same thing.If
require.True(wantPool.Equal(cc.CACertsPool))passes,cmp.Diffwill always be empty. Consider keeping only one check.require.NoError(t, errWantPool) - require.True(t, wantPool.Equal(cc.CACertsPool)) - - if diff := cmp.Diff(wantPool, cc.CACertsPool); diff != "" { - t.Errorf( - "SetCaPoolFromFile: pool mismatch (-want +got):\n%s", - diff, - ) - } + require.True(t, wantPool.Equal(cc.CACertsPool), "CACertsPool mismatch")Note:
cmp.Diffon*x509.CertPoolmay not produce meaningful output since it's an opaque type. TheEqual()method is the correct way to compare cert pools.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/certinfo.go(2 hunks)internal/certinfo/certinfo.go(1 hunks)internal/certinfo/certinfo_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
🧬 Code graph analysis (2)
internal/certinfo/certinfo.go (1)
internal/certinfo/common_handlers.go (1)
GetKeyFromFile(259-287)
internal/certinfo/certinfo_test.go (2)
internal/certinfo/certinfo.go (2)
Reader(40-43)NewCertinfoConfig(67-78)internal/certinfo/common_handlers.go (3)
GetRootCertsFromFile(69-89)GetCertsFromBundle(104-148)GetKeyFromFile(259-287)
🔇 Additional comments (6)
internal/certinfo/certinfo.go (1)
111-127: LGTM! Good refactoring to decouple env var configuration.The updated signature allows callers to specify the environment variable name, improving flexibility and testability. The implementation correctly forwards
keyPwEnvVartoGetKeyFromFile.cmd/certinfo.go (1)
87-93: LGTM!The call correctly passes the environment variable name to the updated
SetPrivateKeyFromFilesignature.internal/certinfo/certinfo_test.go (4)
31-103: Good table-driven test structure with comprehensive error scenarios.The test table effectively covers multiple error conditions (empty path, unreadable file, non-existent file, wrong format, wrong PEM type) and the
expectMsgmap cleverly allows reuse across different test functions.
164-219: LGTM!The test correctly validates both error scenarios and success path. Using
cmp.Difffor certificate slice comparison is appropriate.
284-308: LGTM!Simple and effective test for boolean flag toggling.
11-18: ThemockReaderstruct is dead code that's never instantiated. ThemockErrReaderandinputReadervariables used in the test struct at line 53 and elsewhere are not instances of this struct—they are properly implemented Reader types (MockErrReaderandInputReader) defined inmain_test.gothat include bothReadFileandReadPasswordmethods.Remove the unused
mockReaderstruct from lines 11-18.Likely an incorrect or invalid review comment.
…of TLSEndpoint to receiver
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/certinfo/main_test.go (2)
419-430: Consider addingMinVersionto the TLS configuration.The TLS config sets
MaxVersionbut notMinVersion. While this is test code and defaults to TLS 1.0 for servers, addingMinVersion: tls.VersionTLS12(ortls.VersionTLS13) would make the test server configuration more explicit and secure.Based on learnings, you wanted to be reminded about potentially adding MinVersion enforcement to TLS configurations in test code once legacy server compatibility becomes less critical.
ts.TLS = &tls.Config{ Certificates: []tls.Certificate{cert}, CipherSuites: tlsCipherSuites, MaxVersion: tlsMaxVersion, + MinVersion: tls.VersionTLS12, }
376-397: Consider consolidating duplicate listener creation logic.The listener creation blocks for proxy-enabled and non-proxy cases share similar error handling patterns. A minor refactor could reduce duplication.
- if cfg.serverAddr != emptyString && !cfg.proxyprotoEnabled { - listener, err := net.Listen("tcp", cfg.serverAddr) - if err != nil { - return nil, fmt.Errorf("error creating listener: %w", err) - } - - ts.Listener = listener - } - - if cfg.serverAddr != emptyString && cfg.proxyprotoEnabled { + if cfg.serverAddr != emptyString { ln, err := net.Listen("tcp", cfg.serverAddr) if err != nil { - return nil, fmt.Errorf("error creating proxyproto enabled listener: %w", err) + return nil, fmt.Errorf("error creating listener: %w", err) } - proxyListener := &proxyproto.Listener{ - Listener: ln, - ReadHeaderTimeout: 10 * time.Second, + if cfg.proxyprotoEnabled { + ts.Listener = &proxyproto.Listener{ + Listener: ln, + ReadHeaderTimeout: 10 * time.Second, + } + } else { + ts.Listener = ln } - - ts.Listener = proxyListener }internal/certinfo/certinfo_handlers.go (1)
132-132: Minor: Add blank line above return.The linter flags excess lines above the return statement. Consider adding a blank line between line 131 and the return for consistency.
Apply this diff:
CertsToTables(w, rootCerts) } + return nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/certinfo.go(3 hunks)internal/certinfo/certinfo.go(3 hunks)internal/certinfo/certinfo_handlers.go(6 hunks)internal/certinfo/certinfo_handlers_test.go(1 hunks)internal/certinfo/certinfo_test.go(2 hunks)internal/certinfo/main_test.go(4 hunks)internal/style/style_handlers.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
🧬 Code graph analysis (5)
internal/certinfo/certinfo_handlers.go (2)
internal/certinfo/certinfo.go (1)
CertinfoConfig(21-36)internal/style/style_handlers.go (2)
LgSprintf(22-27)PrintKeyInfoStyle(58-89)
internal/certinfo/main_test.go (1)
internal/requests/requests.go (1)
Host(75-78)
internal/certinfo/certinfo.go (1)
internal/certinfo/common_handlers.go (2)
GetCertsFromBundle(104-148)GetKeyFromFile(259-287)
internal/certinfo/certinfo_handlers_test.go (3)
internal/certinfo/certinfo.go (1)
NewCertinfoConfig(66-77)internal/certinfo/common_handlers.go (1)
GetCertsFromBundle(104-148)internal/certinfo/certinfo_handlers.go (1)
CertsToTables(187-242)
internal/certinfo/certinfo_test.go (2)
internal/certinfo/certinfo.go (2)
Reader(39-42)NewCertinfoConfig(66-77)internal/certinfo/common_handlers.go (3)
GetRootCertsFromFile(69-89)GetCertsFromBundle(104-148)GetKeyFromFile(259-287)
🪛 ast-grep (0.40.0)
internal/certinfo/certinfo_handlers.go
[warning] 135-138: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: c.CACertsPool,
InsecureSkipVerify: c.TLSInsecure,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
internal/certinfo/main_test.go
[warning] 425-429: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
Certificates: []tls.Certificate{cert},
CipherSuites: tlsCipherSuites,
MaxVersion: tlsMaxVersion,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🪛 golangci-lint (2.5.0)
internal/certinfo/certinfo_handlers.go
[error] 132-132: missing whitespace above this line (too many lines above return)
(wsl_v5)
internal/style/style_handlers.go
[warning] 87-87: unhandled-error: Unhandled error in call to function fmt.Fprintln
(revive)
🔇 Additional comments (19)
internal/style/style_handlers.go (1)
58-89: LGTM! Writer-based output improves testability.The refactor to accept
io.Writerenables flexible output targets and better test coverage. The ED25519 capitalization aligns with the other key type labels (RSA, ECDSA).Regarding the static analysis warning about the unhandled error from
fmt.Fprintlnon line 87: this is low-risk since write failures to the provided writer are rare in practice and this is display-only code. However, if you want to be consistent with the error-returning pattern used elsewhere in the PR (e.g.,PrintDatareturning errors), you could consider returning an error from this function as well.internal/certinfo/main_test.go (1)
365-375: LGTM! Well-structured test server helper.The
NewHTTPSTestServerfunction provides a reusable TLS-enabled test server facility with good error handling and flexible configuration options.internal/certinfo/certinfo_handlers_test.go (3)
11-159: Comprehensive TLS testing with good coverage.The
TestCertinfo_GetRemoteCertstests cover a wide range of TLS scenarios including RSA certs, insecure mode, CA validation, IPv6, and server name mismatches. The table-driven approach and proper parallel execution are well done.
161-267: LGTM! Good test coverage for certificate table generation.The test verifies that
CertsToTablescorrectly renders certificate information for RSA, ECDSA, and Ed25519 certificates.
269-379: LGTM! Thorough PrintData validation.The test validates that
PrintDatacorrectly outputs certificate information for various configurations including local certs, CA certs, and remote TLS endpoints.cmd/certinfo.go (2)
76-99: LGTM! Error handling allows graceful partial configuration.The pattern of printing errors but continuing execution allows users to see results from successfully loaded components even when some files fail to load. The new
PrintDatawith explicit writer and error return improves testability.
20-20: LGTM! Environment variable for private key password.Using an environment variable for the private key password is a secure pattern that avoids exposing sensitive data in command-line arguments.
internal/certinfo/certinfo_test.go (4)
22-94: LGTM! Well-structured shared test data table.The
certinfoConfigFileReadErrorTeststable provides a reusable set of error scenarios that are shared across multiple test functions, reducing duplication and ensuring consistent coverage.
96-153: LGTM! Good refactor to table-driven tests.The refactored
TestCertinfo_SetCaPoolFromFileproperly iterates over shared error cases and includes a dedicated success test withcmp.Diffvalidation.
212-273: LGTM! Comprehensive private key file loading tests.The tests cover error scenarios and validate successful key loading with proper comparison.
275-326: LGTM! Simple and effective setter tests.The
TestCertinfo_SetTLSInsecureandTestCertinfo_SetTLSServerNametests are straightforward and cover the expected value propagation.internal/certinfo/certinfo.go (4)
98-101: LGTM: Formatting improvement.The multiline formatting of the GetCertsFromBundle call improves readability.
113-117: Good API improvement for password handling.Making the key password environment variable name configurable (via
keyPwEnvVarparameter) provides better flexibility for callers compared to hardcoding a single environment variable name.
155-158: LGTM: Clear parameter naming.Using
skipVerifyas the parameter name makes the intent explicit.
160-165: LGTM: Clear parameter naming.Using
serverNameas the parameter name makes the intent explicit.internal/certinfo/certinfo_handlers.go (4)
24-68: LGTM: PrintData properly refactored for testability.The function now accepts an
io.Writerparameter and returns errors, making it testable and allowing proper error propagation. All output is correctly redirected to the writer.
135-165: Consider adding MinVersion to TLS configuration.Based on learnings, you've expressed interest in adding MinVersion enforcement to TLS configurations. While this is production code (not test code), the static analysis correctly flags that no MinVersion is set, defaulting to TLS 1.2 for clients.
For a tool focused on certificate inspection and TLS diagnostics, you may want to retain compatibility with older TLS versions to diagnose legacy servers. However, if legacy support isn't a requirement, consider adding
MinVersion: tls.VersionTLS13to the TLS config on line 136.Based on learnings, you've previously noted interest in MinVersion enforcement once legacy compatibility becomes less critical. Would you like to enforce TLS 1.3 as the minimum version here, or maintain backward compatibility for now?
163-165: Verify intended behavior: early return in insecure mode prevents certificate storage.When
TLSInsecureis true, the function returns early on line 164 without storingcs.PeerCertificatesinc.TLSEndpointCerts(which happens on line 168). This means that when running in insecure mode, the endpoint certificates won't be available for display inPrintData.Confirm this is the intended behavior. If certificates should still be displayed even in insecure mode, move the certificate storage (line 168) before the early return.
187-242: LGTM: CertsToTables properly refactored.The function now accepts an
io.Writerparameter, making it testable. All output is correctly directed to the writer.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/certinfo/certinfo_test.go (1)
203-208: Minor: error message wording could be more precise.The error message "pool mismatch" (line 205) is slightly misleading since we're comparing certificate slices, not a pool. Consider "certs mismatch" for clarity, though the intent is clear from context.
Apply this diff:
if diff := cmp.Diff(wantCerts, cc.CertsBundle); diff != "" { t.Errorf( - "SetCertsFromFile: pool mismatch (-want +got):\n%s", + "SetCertsFromFile: certs mismatch (-want +got):\n%s", diff, ) }internal/certinfo/certinfo.go (1)
135-153: Verify: SetTLSEndpoint now performs network I/O.The function now calls
GetRemoteCerts(line 146), which performs TLS handshake and retrieval over the network. This meansSetTLSEndpointcan fail with network timeouts, connection refused, or TLS errors—not just endpoint parsing errors.While this coupling simplifies the API, it may surprise callers who expect a "Set" method to only store values. Consider whether:
- The function name should hint at the network behavior (e.g.,
SetTLSEndpointAndFetch), or- Parsing and retrieval should remain separate (as they were before), requiring explicit calls to both SetTLSEndpoint and GetRemoteCerts.
The tests acknowledge this design (certinfo_test.go:396-399), so this appears intentional. If so, adding a doc comment explaining the network behavior would help callers understand failure modes.
Alternatively, confirm this design aligns with your intended API surface. If the tight coupling is desired, adding a godoc comment would suffice:
+// SetTLSEndpoint parses the hostport, stores the endpoint details, and +// immediately retrieves certificates from the endpoint via TLS handshake. +// Returns an error if the endpoint format is invalid or if the TLS +// connection fails (e.g., network timeout, certificate validation error). func (c *CertinfoConfig) SetTLSEndpoint(hostport string) error {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
devenv.nix(1 hunks)internal/certinfo/certinfo.go(3 hunks)internal/certinfo/certinfo_handlers_test.go(1 hunks)internal/certinfo/certinfo_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/certinfo/certinfo_handlers_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
📚 Learning: 2025-11-29T14:05:35.000Z
Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 10
File: internal/requests/requests_test.go:918-997
Timestamp: 2025-11-29T14:05:35.000Z
Learning: In the https-wrench tool test suite (xenOs76/https-wrench repository), the user wants to be reminded in the future about potentially adding MinVersion enforcement to TLS configurations in test code, once legacy server compatibility becomes less critical.
Applied to files:
devenv.nix
🧬 Code graph analysis (1)
internal/certinfo/certinfo_test.go (2)
internal/certinfo/certinfo.go (2)
Reader(39-42)NewCertinfoConfig(66-77)internal/certinfo/common_handlers.go (3)
GetRootCertsFromFile(69-89)GetCertsFromBundle(104-148)GetKeyFromFile(259-287)
🔇 Additional comments (10)
devenv.nix (1)
455-515: LGTM! Typo fixes improve test descriptions.The consistent correction of "tlsEnpoint" to "tlsEndpoint" across all test scripts improves readability and maintains naming consistency with the codebase.
internal/certinfo/certinfo_test.go (7)
7-9: Good choice of assertion library for detailed comparisons.Switching to
go-cmpfor structural comparisons while retainingtestify/requirefor basic assertions provides clear, actionable diff output when comparing complex certificate and key structures.
22-94: Excellent table-driven test design.The shared
certinfoConfigFileReadErrorTeststable elegantly reduces duplication across SetCaPoolFromFile, SetCertsFromFile, and SetPrivateKeyFromFile tests. The comprehensive error scenarios and the helpful comment about AppendCertsFromPEM behavior (lines 81-82) demonstrate thoughtful test design.
96-153: Comprehensive test coverage with clear error paths.The refactored test properly validates both error cases (via the shared table) and the success path with dual assertions (CertPool.Equal + cmp.Diff). The explicit handling of empty-string inputs (lines 109-112) documents expected no-op behavior clearly.
212-273: Well-structured private key validation tests.The test correctly validates private key loading with proper error handling and uses cmp.Diff to compare the loaded key against the expected key. The parameterization of the password environment variable (line 223, 261) aligns with the API changes in certinfo.go.
275-299: LGTM! Simple and effective test.The test comprehensively covers both boolean states for TLSInsecure with clear assertions.
301-326: LGTM! Good coverage of server name scenarios.The test covers representative cases including empty string, simple name, and fully-qualified domain name.
328-410: Well-designed endpoint parsing tests with clear scope.The comprehensive test table covers IPv4, IPv6, and various malformed endpoints. The comment at lines 396-399 clearly explains why error checking is skipped for success cases: SetTLSEndpoint now invokes GetRemoteCerts internally, which requires actual network access. This test focuses on endpoint parsing logic, leaving the certificate retrieval behavior to dedicated GetRemoteCerts tests (as mentioned in the comment).
internal/certinfo/certinfo.go (2)
113-133: Good API improvement: parameterizes password environment variable.The addition of the
keyPwEnvVarparameter makes SetPrivateKeyFromFile more flexible and testable by allowing callers to specify which environment variable contains the private key password, rather than hardcodingprivateKeyPwEnvVar.
160-166: Good defensive check for server name assignment.The addition of the empty-string guard (line 161) prevents unintentionally clearing a previously set TLSServerName when an empty value is passed. This makes the setter more robust.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.