Fix early return in GetRemoteCerts when TLSInsecure is true

[xenOs76]

Summary by CodeRabbit

• Refactor

  • Optimized TLS configuration initialization order for improved clarity and dependency management.
  • Enhanced certificate state retrieval to occur earlier in the validation flow, reducing redundant operations.

• Tests

  • Expanded test coverage for certificate validation scenarios, including validation with server names, insecure mode, and missing server name configurations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The pull request reorders TLS configuration setup in the certinfo command and handlers, moving SetTLSInsecure/tlsServerName calls relative to certificate loading and endpoint configuration. Additionally, GetRemoteCerts now fetches the TLS connection state earlier in the function flow, and tests are expanded to cover additional certificate validation scenarios.

Changes

Cohort / File(s)	Summary
TLS configuration reordering cmd/certinfo.go	Reordered TLS configuration calls: moved initial SetTLSInsecure/tlsServerName from before CA bundle loading to after SetCertsFromFile, with an additional SetTLSInsecure/SetTLSServerName call before SetTLSEndpoint. Added comment clarifying TLS option dependencies relative to endpoint setup.
Certificate retrieval refactoring internal/certinfo/certinfo_handlers.go	GetRemoteCerts now retrieves TLS connection state and populates TLSEndpointCerts before the insecure check, removing the duplicate connection state retrieval that occurred after the insecure check.
Test expansion and refactoring internal/certinfo/certinfo_handlers_test.go	Refactored TestCertinfo_PrintData test structure with new fields (tlsInsecure, tlsServerName, srvCfg, expectCertsFetchErr, expectCertsFetchMsg) to support additional test scenarios. Added tests for validated certs with server name, TLS insecure mode, missing server name errors, and adjusted SetTLSEndpoint/SetTLSServerName call ordering. Conditional error and certificate verification checks now reflect dependency order.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• TLS dependency order: Requires careful verification that the reordering of SetTLSInsecure/tlsServerName calls relative to SetCertsFromFile and SetTLSEndpoint doesn't introduce unintended side effects or break existing functionality.
• Certificate state retrieval timing: Ensure that fetching TLS connection state earlier doesn't cause issues in any code path or race conditions.
• Test coverage: New test scenarios and conditional assertions need verification to confirm they accurately reflect the new behavior and dependencies.

Possibly related PRs

• (CI) more certinfo tests #13: Modifies the same certinfo TLS setup and certificate-fetching logic with overlapping changes to TLS option application ordering and GetRemoteCerts/SetTLSEndpoint interactions.

Poem

🐰 Hops through the code with careful grace,
Reordering TLS in just the right place,
Certificates early, dependencies clear,
Tests multiplied for confidence near!
Configuration sequences now align,
A cryptographic dance, oh so fine! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing early return behavior in GetRemoteCerts when TLSInsecure is true, which aligns with the core modification of retrieving TLS connection state before the insecure check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/tlsendpointcerts_when_insecure

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 331673c and e4d4274.

📒 Files selected for processing (3)

• cmd/certinfo.go (1 hunks)
• internal/certinfo/certinfo_handlers.go (1 hunks)
• internal/certinfo/certinfo_handlers_test.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.

Learnt from: xenOs76
Repo: xenOs76/https-wrench PR: 3
File: internal/requests/requests.go:164-182
Timestamp: 2025-11-08T16:00:48.187Z
Learning: In the https-wrench tool (xenOs76/https-wrench repository), do not enforce minimum TLS version constraints in the HTTP client configuration. The tool is designed to inspect and test TLS settings on various servers, including legacy ones, and needs the flexibility to connect to servers with older TLS versions.

🧬 Code graph analysis (1)

internal/certinfo/certinfo_handlers_test.go (1)

internal/certinfo/main_test.go (4)

• RSASampleCertKeyFile (95-95)
• RSACaCertFile (91-91)
• RSASampleCertFile (99-99)
• ED25519SamplePlaintextPrivateKey (76-76)

🔇 Additional comments (6)

internal/certinfo/certinfo_handlers.go (1)

162-168: Excellent fix for TLSInsecure early return bug.

The reordering correctly ensures TLSEndpointCerts is populated from the TLS connection state before the early return when TLSInsecure is true. This allows certificate data to be retrieved and displayed even when verification is skipped, which aligns with the tool's purpose of inspecting TLS certificates.

cmd/certinfo.go (1)

82-87: Clear dependency ordering with helpful comment.

The reordering of SetTLSInsecure and SetTLSServerName to occur before SetTLSEndpoint is correct, and the comment clearly documents why this ordering matters. This ensures the TLS connection uses the proper configuration when fetching certificates.

internal/certinfo/certinfo_handlers_test.go (4)

270-281: Well-structured test expansion.

The new test struct fields (tlsInsecure, tlsServerName, srvCfg, expectCertsFetchErr, expectCertsFetcMsg) provide the necessary parameters to drive comprehensive test scenarios covering the TLSInsecure fix.

---

297-369: Comprehensive test case coverage.

The new test cases thoroughly exercise the fix:

• Line 297-309: Validates certificates with proper CA
• Line 311-324: Tests error case without CA (not insecure)
• Line 327-340: Tests insecure mode correctly bypasses verification while still fetching certs
• Line 342-355: Tests missing server name error
• Line 357-369: Tests key mismatch scenario

This coverage ensures the fix works correctly in various TLS configuration combinations.

---

392-411: Test setup properly mirrors production initialization order.

The test correctly calls SetTLSServerName and SetTLSInsecure before SetTLSEndpoint, matching the order established in cmd/certinfo.go. The comment on lines 395-403 clearly documents this dependency for future maintainers.

---

431-461: Conditional assertions handle error scenarios correctly.

The test properly guards certificate-related assertions with !tt.expectCertsFetchErr, ensuring expectations are only validated when certificate fetch succeeds. The TLS endpoint details assertions (lines 457-461) are appropriately conditional on both having a TLS endpoint and no fetch errors.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}