Skip to content

TLS with optional client auth#584

Open
qnic11 wants to merge 5 commits into
sipcapture:masterfrom
qnic11:master
Open

TLS with optional client auth#584
qnic11 wants to merge 5 commits into
sipcapture:masterfrom
qnic11:master

Conversation

@qnic11
Copy link
Copy Markdown

@qnic11 qnic11 commented Jan 13, 2026

Hi Team, we’ve created and tested the attached patch to add TLS termination directly into heplify-server.

We believe this could be a solid starting point to expand TLS-related capabilities over time and give operators more flexibility around protocol and deployment options.

Any feedback or guidance on whether this aligns with your roadmap would be greatly appreciated.

Removed unused dependency on github.com/negbie/cert.
Removed unused dependencies from go.sum.
Added section on HEP input listeners and configuration options.
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jan 13, 2026

CLA assistant check
All committers have signed the CLA.

@kYroL01 kYroL01 requested a review from adubovikov January 13, 2026 10:57
@kYroL01
Copy link
Copy Markdown

kYroL01 commented Jan 13, 2026

Thanks a lot @qnic11 . Amazing contribution to this project!
We'll review it ASAP.
Thank you in advance

@qnic11
Copy link
Copy Markdown
Author

qnic11 commented Jan 13, 2026

Thanks a lot @qnic11 . Amazing contribution to this project! We'll review it ASAP. Thank you in advance

Grazie equipo.

We’d be thrilled to see this feature implemented; please let us know if there’s anything else required from our side.

BR - Q.

@kYroL01 kYroL01 requested review from Copilot and removed request for adubovikov March 10, 2026 10:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds first-class TLS termination to the HEP TLS listener with support for optional/required client certificate authentication, moving away from the previous auto-generated certificate approach.

Changes:

  • Load TLS server certificate/key from configured files and build a reusable tls.Config.
  • Add optional mutual-TLS behavior via TLSClientCAFile and TLSRequireClientCert.
  • Update configuration schema and README to reflect the new TLS configuration fields and remove the old cert-generation dependency.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
server/tls.go Replaces cert auto-generation with file-based TLS config loading; adds client-auth options.
config/config.go Replaces TLSCertFolder with TLSCertFile/TLSKeyFile plus client-auth settings.
README.md Documents listener addresses and the new TLS configuration knobs.
go.mod Removes github.com/negbie/cert dependency.
go.sum Removes github.com/negbie/cert checksums.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread README.md
Comment thread server/tls.go
Comment thread server/tls.go
Comment thread server/tls.go
Comment thread config/config.go
Comment on lines +83 to +87
TLSCertFile string `default:""`
TLSKeyFile string `default:""`
TLSClientCAFile string `default:""`
TLSRequireClientCert bool `default:"false"`
TLSMinVersion string `default:"1.2"`
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

This change removes TLSCertFolder (and the previous auto-generated certificate behavior) in favor of explicit TLSCertFile/TLSKeyFile. That’s a breaking configuration change for existing deployments; consider keeping TLSCertFolder as a deprecated fallback (or adding a clear migration note in docs/release notes) so existing configs don’t silently stop working when HEPTLSAddr is enabled.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

@kYroL01
Copy link
Copy Markdown

kYroL01 commented Mar 10, 2026

@qnic11 Please check the Copilot report for your PR - I think there are some interesting and valid points to check.
Thank you

@adubovikov
Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request

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.

5 participants