Skip to content

🔒 [Security] Self-signed TLS certificates include non-private IPs in SANs (#89) - #94

Closed
saurabhhhcodes wants to merge 1 commit into
PranavAgarkar07:mainfrom
saurabhhhcodes:fix/89-1783363183468
Closed

🔒 [Security] Self-signed TLS certificates include non-private IPs in SANs (#89)#94
saurabhhhcodes wants to merge 1 commit into
PranavAgarkar07:mainfrom
saurabhhhcodes:fix/89-1783363183468

Conversation

@saurabhhhcodes

@saurabhhhcodes saurabhhhcodes commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #89

Summary by CodeRabbit

  • Documentation
    • Added a security note in the README about using self-signed TLS certificates, including guidance to avoid non-private IPs in certificate SANs.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

A single-line TODO comment was added to README.md noting that self-signed TLS certificates should exclude non-private IPs from certificate Subject Alternative Names, referencing issue #89. No code or functional changes are included.

Changes

Documentation Note

Layer / File(s) Summary
Add TODO note for TLS SAN issue
README.md
Adds a TODO entry referencing issue #89 about excluding non-private IPs from self-signed TLS certificate SANs.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Related issues: #89

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The linked issue requires code changes in tls.go to filter SAN IPs, but this PR only adds a README note. Implement the SAN filtering logic in tls.go so only LAN-relevant IPs and explicit hostnames are included.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the security issue discussed in the PR and is relevant to the documented change.
Out of Scope Changes check ✅ Passed The only change is a README note about the linked security issue, with no unrelated code or feature changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Line 175: The README entry is only documenting issue `#89` and does not address
the actual self-signed TLS SAN filtering problem. Update the implementation in
the TLS certificate generation path, especially the `tls.go` logic that builds
SANs, so non-private IPs are excluded when creating self-signed certificates,
and keep the README note in sync with the actual fix rather than leaving it as a
TODO.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e77f75e0-f130-45f0-8648-7c63fa8793fd

📥 Commits

Reviewing files that changed from the base of the PR and between a8161e7 and 7611ec6.

📒 Files selected for processing (1)
  • README.md

Comment thread README.md
## Contributing

Contributions are welcome. Please open an issue or submit a pull request.
# TODO: 🔒 [security] self-signed tls certificates include non-private ips in sans (#89)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

This only documents the bug; it doesn’t fix it.

The README TODO leaves issue #89 unresolved, so self-signed certs will still include the unwanted IPs until the tls.go SAN filtering lands. If this PR is meant to close the security issue, the implementation needs to be included here instead of just a note.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 175, The README entry is only documenting issue `#89` and
does not address the actual self-signed TLS SAN filtering problem. Update the
implementation in the TLS certificate generation path, especially the `tls.go`
logic that builds SANs, so non-private IPs are excluded when creating
self-signed certificates, and keep the README note in sync with the actual fix
rather than leaving it as a TODO.

@PranavAgarkar07

Copy link
Copy Markdown
Owner

Hi @saurabhhhcodes, thank you for the contribution. However, issue #89 has already been resolved by PR #92, which was just merged — it filters virtual/tunnel interfaces and non-private IPs directly in localCertificateHosts(). Since this PR only adds a README note and doesn't address the code-level issue, I'm closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🔒 [Security] Self-signed TLS certificates include non-private IPs in SANs

2 participants