Skip to content

Fix mutual TLS parameter handling and clarify CA certificate usage #2210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 8, 2025

This PR fixes a critical bug in the mutual TLS implementation and improves documentation to clarify the distinction between different uses of CA certificates in node-to-node communication.

Issues Fixed

1. Critical Bug: Ignored Mutual Parameter

The NewTLSMux function was ignoring the mutual parameter, always passing false to the internal implementation:

// Before (buggy)
func NewTLSMux(ln net.Listener, adv net.Addr, cert, key, caCert string, insecure, mutual bool) (*Mux, error) {
    return newTLSMux(ln, adv, cert, key, caCert, false) // Always false!
}

// After (fixed)  
func NewTLSMux(ln net.Listener, adv net.Addr, cert, key, caCert string, insecure, mutual bool) (*Mux, error) {
    return newTLSMux(ln, adv, cert, key, caCert, mutual) // Now respects the parameter
}

This meant the -node-verify-client flag had no effect, as mutual TLS was never actually enabled.

2. Unclear Documentation

The documentation didn't clearly distinguish between two different uses of CA certificates:

  • Server certificate validation: When a client connects to a server, it uses a CA cert to verify the server's certificate
  • Mutual TLS client verification: When a server requires clients to present certificates, it uses a CA cert pool to verify client certificates

Changes Made

Enhanced Documentation

  • Updated all TLS configuration function comments to clearly explain CA certificate usage
  • Improved flag descriptions to clarify the roles of -node-ca-cert and -node-verify-client
  • Added explanatory comments distinguishing server validation vs mutual TLS scenarios

Improved Logging

Enhanced startup logging to clarify CA certificate purpose based on configuration:

if cfg.NodeVerifyClient {
    b.WriteString(fmt.Sprintf(", CA cert %s (for client verification)", cfg.NodeX509CACert))
} else {
    b.WriteString(fmt.Sprintf(", CA cert %s (for server verification)", cfg.NodeX509CACert))
}

Comprehensive Testing

Added focused tests that verify:

  • Mutual TLS is properly enabled when mutual=true
  • Mutual TLS is properly disabled when mutual=false
  • TLS configuration reflects the intended settings

Impact

This fix ensures that:

  • The -node-verify-client flag now works as documented
  • Users can properly configure mutual TLS for enhanced security
  • Documentation clearly explains the distinction between different CA certificate uses
  • Future development has better clarity around TLS configuration

The changes are surgical and focused, fixing the specific bug while improving clarity without breaking existing functionality.

Fixes #2209.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • invalid-address
    • Triggering command: /tmp/go-build2500858737/b617/http.test -test.testlogfile=/tmp/go-build2500858737/b617/testlog.txt -test.paniconexit0 -test.timeout=10m0s (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@otoolep otoolep marked this pull request as ready for review August 8, 2025 16:07
Co-authored-by: otoolep <536312+otoolep@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Clarify and implement clear handling of mutual TLS between nodes Fix mutual TLS parameter handling and clarify CA certificate usage Aug 8, 2025
@Copilot Copilot AI requested a review from otoolep August 8, 2025 16:21
Copilot finished work on behalf of otoolep August 8, 2025 16:21
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.

Clarify and implement clear handling of mutual TLS between nodes
2 participants