Repository: open-telemetry/opamp-go
Pull request #149 (2022-11-23 09:22:52)

Add TLSConfig into httpClient
In order to support HTTPs connections with certificate between Agents and Server, TLSConfig shall be passed to http.Client on HTTPSender structure.

A new function AddTLSConfig was created on httpsender.go to handle that, which is called during Start process of OPAMP httpClient.


tigrannajaryan: @lucas-baldo thank you for the PR. Will you be able to also add a full test in clientimpl_test.go that verifies the TLS connection using the mock server?

We already have TLSConfig setting supported in WebSocket client, so with the addition of your code for HTTP client we should be able to test TLSConfig setting for both transports.
codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/149?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
Base: **76.11**% // Head: **75.96**% // Decreases project coverage by **`-0.15%`** :warning:
> Coverage data is based on head [(`653ab0c`)](https://codecov.io/gh/open-telemetry/opamp-go/pull/149?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) compared to base [(`efddaa2`)](https://codecov.io/gh/open-telemetry/opamp-go/commit/efddaa260895e7ebb2431deeefef3a3380d8d902?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   76.11%   75.96%   -0.16%     
==========================================
  Files          24       24              
  Lines        1834     1843       +9     
==========================================
+ Hits         1396     1400       +4     
- Misses        326      331       +5     
  Partials      112      112              
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opamp-go/pull/149?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [client/httpclient.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/149/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2h0dHBjbGllbnQuZ28=) | `95.00% <100.00%> (+0.26%)` | :arrow_up: |
| [client/internal/httpsender.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/149/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2h0dHBzZW5kZXIuZ28=) | `72.04% <100.00%> (+1.08%)` | :arrow_up: |
| [internal/testhelpers/tcpproxy.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/149/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-aW50ZXJuYWwvdGVzdGhlbHBlcnMvdGNwcHJveHkuZ28=) | `73.01% <0.00%> (-7.94%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/149?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).


Patch:
diff --git a/client/httpclient.go b/client/httpclient.go
index fea31a9..0fc34aa 100644
--- a/client/httpclient.go
+++ b/client/httpclient.go
@@ -45,6 +45,9 @@ func (c *httpClient) Start(ctx context.Context, settings types.StartSettings) er
 	// Prepare Server connection settings.
 	c.sender.SetRequestHeader(settings.Header)
 
+	// Add TLS configuration into httpClient
+	c.sender.AddTLSConfig(settings.TLSConfig)
+
 	if settings.EnableCompression {
 		c.sender.EnableCompression()
 	}
diff --git a/client/internal/httpsender.go b/client/internal/httpsender.go
index b2a9938..42244b4 100644
--- a/client/internal/httpsender.go
+++ b/client/internal/httpsender.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"compress/gzip"
 	"context"
+	"crypto/tls"
 	"errors"
 	"fmt"
 	"io"
@@ -263,3 +264,11 @@ func (h *HTTPSender) EnableCompression() {
 	h.compressionEnabled = true
 	h.requestHeader.Set(headerContentEncoding, encodingTypeGZip)
 }
+
+func (h *HTTPSender) AddTLSConfig(config *tls.Config) {
+	if config != nil {
+		h.client.Transport = &http.Transport{
+			TLSClientConfig: config,
+		}
+	}
+}
diff --git a/client/internal/mockserver.go b/client/internal/mockserver.go
index 11a7013..6a85ec6 100644
--- a/client/internal/mockserver.go
+++ b/client/internal/mockserver.go
@@ -37,7 +37,7 @@ type MockServer struct {
 const headerContentType = "Content-Type"
 const contentTypeProtobuf = "application/x-protobuf"
 
-func StartMockServer(t *testing.T) *MockServer {
+func newMockServer(t *testing.T) (*MockServer, *http.ServeMux) {
 	srv := &MockServer{
 		t:                t,
 		expectedHandlers: make(chan receivedMessageHandler, 0),
@@ -65,6 +65,12 @@ func StartMockServer(t *testing.T) *MockServer {
 		},
 	)
 
+	return srv, m
+}
+
+func StartMockServer(t *testing.T) *MockServer {
+	srv, m := newMockServer(t)
+
 	srv.srv = httptest.NewServer(m)
 
 	u, err := url.Parse(srv.srv.URL)
@@ -78,6 +84,26 @@ func StartMockServer(t *testing.T) *MockServer {
 	return srv
 }
 
+func StartTLSMockServer(t *testing.T) *MockServer {
+	srv, m := newMockServer(t)
+
+	srv.srv = httptest.NewTLSServer(m)
+
+	u, err := url.Parse(srv.srv.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+	srv.Endpoint = u.Host
+
+	testhelpers.WaitForEndpoint(srv.Endpoint)
+
+	return srv
+}
+
+func (m *MockServer) GetHTTPTestServer() *httptest.Server {
+	return m.srv
+}
+
 // EnableExpectMode enables the expect mode that allows using Expect() method
 // to describe what message is expected to be received.
 func (m *MockServer) EnableExpectMode() {


Test patch:
diff --git a/client/clientimpl_test.go b/client/clientimpl_test.go
index 2b23751..576244f 100644
--- a/client/clientimpl_test.go
+++ b/client/clientimpl_test.go
@@ -2,6 +2,8 @@ package client
 
 import (
 	"context"
+	"crypto/tls"
+	"crypto/x509"
 	"math/rand"
 	"net/http"
 	"net/http/httptest"
@@ -83,9 +85,17 @@ func prepareSettings(t *testing.T, settings *types.StartSettings, c OpAMPClient)
 	require.NoError(t, err)
 	switch c.(type) {
 	case *httpClient:
-		u.Scheme = "http"
+		if settings.TLSConfig != nil {
+			u.Scheme = "https"
+		} else {
+			u.Scheme = "http"
+		}
 	case *wsClient:
-		u.Scheme = "ws"
+		if settings.TLSConfig != nil {
+			u.Scheme = "wss"
+		} else {
+			u.Scheme = "ws"
+		}
 	}
 	settings.OpAMPServerURL = u.String()
 }
@@ -305,6 +315,50 @@ func TestConnectWithHeader(t *testing.T) {
 	})
 }
 
+func TestConnectWithTLS(t *testing.T) {
+	testClients(t, func(t *testing.T, client OpAMPClient) {
+		// Start a server.
+		srv := internal.StartTLSMockServer(t)
+		var conn atomic.Value
+		srv.OnConnect = func(r *http.Request) {
+			conn.Store(true)
+		}
+
+		certs := rootCAs(t, srv.GetHTTPTestServer())
+
+		// Start a client.
+		settings := types.StartSettings{
+			OpAMPServerURL: "wss://" + srv.Endpoint,
+			TLSConfig: &tls.Config{
+				RootCAs: certs,
+			},
+		}
+
+		startClient(t, settings, client)
+
+		// Wait for connection to be established.
+		eventually(t, func() bool { return conn.Load() != nil })
+
+		// Shutdown the Server and the client.
+		srv.Close()
+		_ = client.Stop(context.Background())
+	})
+}
+
+func rootCAs(t *testing.T, s *httptest.Server) *x509.CertPool {
+	certs := x509.NewCertPool()
+	for _, c := range s.TLS.Certificates {
+		roots, err := x509.ParseCertificates(c.Certificate[len(c.Certificate)-1])
+		if err != nil {
+			t.Fatalf("error parsing server's root cert: %v", err)
+		}
+		for _, root := range roots {
+			certs.AddCert(root)
+		}
+	}
+	return certs
+}
+
 func createRemoteConfig() *protobufs.AgentRemoteConfig {
 	return &protobufs.AgentRemoteConfig{
 		Config: &protobufs.AgentConfigMap{
diff --git a/client/internal/httpsender_test.go b/client/internal/httpsender_test.go
index 31f1aab..58a5268 100644
--- a/client/internal/httpsender_test.go
+++ b/client/internal/httpsender_test.go
@@ -2,6 +2,7 @@ package internal
 
 import (
 	"context"
+	"crypto/tls"
 	"net/http"
 	"sync/atomic"
 	"testing"
@@ -55,3 +56,45 @@ func TestHTTPSenderRetryForStatusTooManyRequests(t *testing.T) {
 	cancel()
 	srv.Close()
 }
+
+func TestAddTLSConfig(t *testing.T) {
+	sender := NewHTTPSender(&sharedinternal.NopLogger{})
+
+	certificate, err := GenerateCertificate()
+	assert.NoError(t, err)
+
+	tlsConfig := &tls.Config{
+		Certificates: []tls.Certificate{certificate},
+	}
+
+	sender.AddTLSConfig(tlsConfig)
+	assert.Equal(t, sender.client.Transport, &http.Transport{TLSClientConfig: tlsConfig})
+}
+
+func GenerateCertificate() (tls.Certificate, error) {
+
+	certPem := []byte(`-----BEGIN CERTIFICATE-----
+MIIBhTCCASugAwIBAgIQIRi6zePL6mKjOipn+dNuaTAKBggqhkjOPQQDAjASMRAw
+DgYDVQQKEwdBY21lIENvMB4XDTE3MTAyMDE5NDMwNloXDTE4MTAyMDE5NDMwNlow
+EjEQMA4GA1UEChMHQWNtZSBDbzBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABD0d
+7VNhbWvZLWPuj/RtHFjvtJBEwOkhbN/BnnE8rnZR8+sbwnc/KhCk3FhnpHZnQz7B
+5aETbbIgmuvewdjvSBSjYzBhMA4GA1UdDwEB/wQEAwICpDATBgNVHSUEDDAKBggr
+BgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MCkGA1UdEQQiMCCCDmxvY2FsaG9zdDo1
+NDUzgg4xMjcuMC4wLjE6NTQ1MzAKBggqhkjOPQQDAgNIADBFAiEA2zpJEPQyz6/l
+Wf86aX6PepsntZv2GYlA5UpabfT2EZICICpJ5h/iI+i341gBmLiAFQOyTDT+/wQc
+6MF9+Yw1Yy0t
+-----END CERTIFICATE-----`)
+
+	keyPem := []byte(`-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIIrYSSNQFaA2Hwf1duRSxKtLYX5CB04fSeQ6tF1aY/PuoAoGCCqGSM49
+AwEHoUQDQgAEPR3tU2Fta9ktY+6P9G0cWO+0kETA6SFs38GecTyudlHz6xvCdz8q
+EKTcWGekdmdDPsHloRNtsiCa697B2O9IFA==
+-----END EC PRIVATE KEY-----`)
+
+	cert, err := tls.X509KeyPair(certPem, keyPem)
+	if err != nil {
+		return tls.Certificate{}, err
+	}
+
+	return cert, nil
+}