Skip to content

Commit d336d78

Browse files
khushijain21belimawr
khushijain21
and
belimawr
authored
[beatsauthextension] Fix bug in configureTransport method (#690)
* [beatsauthextension] Fix bug how custom verifyConnection is built * add tls server name * put root ca back into TLSconfig * add additonal unit tests * fixed test case * upgrade ea-libs * add comments * update elastic-agent-libs * use logp logger * fix go mod tidy * Update extension/beatsauthextension/authenticator.go Co-authored-by: Tiago Queiroz <[email protected]> * Update extension/beatsauthextension/authenticator_test.go Co-authored-by: Tiago Queiroz <[email protected]> * remove ca cert * fix data race * create a copy instead * update --------- Co-authored-by: Tiago Queiroz <[email protected]>
1 parent 3174b19 commit d336d78

File tree

5 files changed

+271
-12
lines changed

5 files changed

+271
-12
lines changed

‎extension/beatsauthextension/authenticator.go‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"fmt"
2323
"net/http"
2424

25+
"github.com/elastic/elastic-agent-libs/logp"
2526
"github.com/elastic/elastic-agent-libs/transport/tlscommon"
2627
"go.opentelemetry.io/collector/component"
2728
"go.opentelemetry.io/collector/extension"
@@ -37,19 +38,25 @@ type authenticator struct {
3738
cfg *Config
3839
telemetry component.TelemetrySettings
3940
tlsConfig *tlscommon.TLSConfig // set by Start
41+
logger *logp.Logger
4042
}
4143

4244
func newAuthenticator(cfg *Config, telemetry component.TelemetrySettings) (*authenticator, error) {
43-
return &authenticator{cfg: cfg, telemetry: telemetry}, nil
45+
logger, err := logp.NewZapLogger(telemetry.Logger)
46+
if err != nil {
47+
return nil, err
48+
}
49+
return &authenticator{cfg: cfg, telemetry: telemetry, logger: logger}, nil
4450
}
4551

4652
func (a *authenticator) Start(ctx context.Context, host component.Host) error {
4753
if a.cfg.TLS != nil {
54+
4855
tlsConfig, err := tlscommon.LoadTLSConfig(&tlscommon.Config{
4956
VerificationMode: tlsVerificationModes[a.cfg.TLS.VerificationMode],
5057
CATrustedFingerprint: a.cfg.TLS.CATrustedFingerprint,
5158
CASha256: a.cfg.TLS.CASha256,
52-
})
59+
}, a.logger)
5360
if err != nil {
5461
return err
5562
}
@@ -77,13 +84,21 @@ func (a *authenticator) RoundTripper(base http.RoundTripper) (http.RoundTripper,
7784
}
7885

7986
func (a *authenticator) configureTransport(transport *http.Transport) error {
87+
8088
if a.tlsConfig != nil {
81-
// injecting verifyConnection here, keeping all other fields on TLSClientConfig intact
82-
beatTLSConfig := a.tlsConfig.BuildModuleClientConfig(a.cfg.TLS.ServerName)
89+
90+
// copy incoming CertPool into our tls config
91+
// because ca_trusted_fingerprint will be appended to CertPool
92+
tlsConfig := *a.tlsConfig // copy before updating, configureTransport may be called concurrently
93+
tlsConfig.RootCAs = transport.TLSClientConfig.RootCAs
94+
95+
beatTLSConfig := tlsConfig.BuildModuleClientConfig(transport.TLSClientConfig.ServerName)
8396

8497
transport.TLSClientConfig.VerifyConnection = beatTLSConfig.VerifyConnection
8598
transport.TLSClientConfig.InsecureSkipVerify = beatTLSConfig.InsecureSkipVerify
99+
86100
}
101+
87102
return nil
88103
}
89104

‎extension/beatsauthextension/authenticator_test.go‎

Lines changed: 248 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ import (
2121
"context"
2222
"crypto/tls"
2323
"crypto/x509"
24+
"encoding/pem"
25+
"net"
26+
"net/http"
27+
"net/http/httptest"
2428
"testing"
2529

2630
"github.com/stretchr/testify/require"
@@ -29,16 +33,18 @@ import (
2933
"go.opentelemetry.io/collector/component/componenttest"
3034
"go.opentelemetry.io/collector/config/configauth"
3135
"go.opentelemetry.io/collector/config/confighttp"
36+
"go.opentelemetry.io/collector/config/configopaque"
3237
"go.opentelemetry.io/collector/config/configoptional"
3338

3439
"github.com/elastic/elastic-agent-libs/transport/tlscommon"
40+
"github.com/elastic/elastic-agent-libs/transport/tlscommontest"
3541
"github.com/elastic/opentelemetry-collector-components/extension/beatsauthextension/internal/metadata"
3642
)
3743

3844
// It tests whether VerifyConnection is set on tls.Config
3945
func TestVerifyConnection(t *testing.T) {
40-
testCerts := tlscommon.GenTestCerts(t)
41-
fingerprint := tlscommon.GetCertFingerprint(testCerts["ca"])
46+
testCerts := tlscommontest.GenTestCerts(t)
47+
fingerprint := tlscommontest.GetCertFingerprint(testCerts["ca"])
4248

4349
settings := componenttest.NewNopTelemetrySettings()
4450
httpClientConfig := confighttp.NewDefaultClientConfig()
@@ -125,7 +131,6 @@ func TestVerifyConnection(t *testing.T) {
125131
VerificationMode: test.verificationMode.String(),
126132
CATrustedFingerprint: test.CATrustedFingerprint,
127133
CASha256: test.CASHA256,
128-
ServerName: test.serverName,
129134
},
130135
}
131136

@@ -165,6 +170,246 @@ func TestVerifyConnection(t *testing.T) {
165170
}
166171
}
167172

173+
// This test suite is from `tlscommon` package in elastic-agent-libs
174+
// https://github.com/elastic/elastic-agent-libs/blob/531c75610fb3fa147cdde354b8ec5433e1b82dc3/transport/tlscommon/tls_config_test.go#L495
175+
// It is modified to work with beatsauthextension
176+
func TestVerificationMode(t *testing.T) {
177+
settings := componenttest.NewNopTelemetrySettings()
178+
httpClientConfig := confighttp.NewDefaultClientConfig()
179+
httpClientConfig.Auth = configoptional.Some(configauth.Config{
180+
AuthenticatorID: component.NewID(metadata.Type),
181+
})
182+
183+
testcases := map[string]struct {
184+
verificationMode tlscommon.TLSVerificationMode
185+
expectingError bool
186+
187+
// hostname is used to make connection
188+
hostname string
189+
190+
// ignoreCerts do not add the Root CA to the trust chain
191+
ignoreCerts bool
192+
193+
// commonName used in the Certificate
194+
commonName string
195+
196+
// dnsNames is used as the SNA DNSNames
197+
dnsNames []string
198+
199+
// ips is used as the SNA IPAddresses
200+
ips []net.IP
201+
}{
202+
"VerifyFull validates domain": {
203+
verificationMode: tlscommon.VerifyFull,
204+
hostname: "localhost",
205+
dnsNames: []string{"localhost"},
206+
},
207+
"VerifyFull validates IPv4": {
208+
verificationMode: tlscommon.VerifyFull,
209+
hostname: "127.0.0.1",
210+
ips: []net.IP{net.IPv4(127, 0, 0, 1)},
211+
},
212+
"VerifyFull validates IPv6": {
213+
verificationMode: tlscommon.VerifyFull,
214+
hostname: "::1",
215+
ips: []net.IP{net.ParseIP("::1")},
216+
},
217+
"VerifyFull domain mismatch returns error": {
218+
verificationMode: tlscommon.VerifyFull,
219+
hostname: "localhost",
220+
dnsNames: []string{"example.com"},
221+
expectingError: true,
222+
},
223+
"VerifyFull IPv4 mismatch returns error": {
224+
verificationMode: tlscommon.VerifyFull,
225+
hostname: "127.0.0.1",
226+
ips: []net.IP{net.IPv4(10, 0, 0, 1)},
227+
expectingError: true,
228+
},
229+
"VerifyFull IPv6 mismatch returns error": {
230+
verificationMode: tlscommon.VerifyFull,
231+
hostname: "::1",
232+
ips: []net.IP{net.ParseIP("faca:b0de:baba::ca")},
233+
expectingError: true,
234+
},
235+
"VerifyFull does not return error when SNA is empty and legacy Common Name is used": {
236+
verificationMode: tlscommon.VerifyFull,
237+
hostname: "localhost",
238+
commonName: "localhost",
239+
expectingError: false,
240+
},
241+
"VerifyFull does not return error when SNA is empty and legacy Common Name is used with IP address": {
242+
verificationMode: tlscommon.VerifyFull,
243+
hostname: "127.0.0.1",
244+
commonName: "127.0.0.1",
245+
expectingError: false,
246+
},
247+
248+
"VerifyStrict validates domain": {
249+
verificationMode: tlscommon.VerifyStrict,
250+
hostname: "localhost",
251+
dnsNames: []string{"localhost"},
252+
},
253+
"VerifyStrict validates IPv4": {
254+
verificationMode: tlscommon.VerifyStrict,
255+
hostname: "127.0.0.1",
256+
ips: []net.IP{net.IPv4(127, 0, 0, 1)},
257+
},
258+
"VerifyStrict validates IPv6": {
259+
verificationMode: tlscommon.VerifyStrict,
260+
hostname: "::1",
261+
ips: []net.IP{net.ParseIP("::1")},
262+
},
263+
"VerifyStrict domain mismatch returns error": {
264+
verificationMode: tlscommon.VerifyStrict,
265+
hostname: "127.0.0.1",
266+
dnsNames: []string{"example.com"},
267+
expectingError: true,
268+
},
269+
"VerifyStrict IPv4 mismatch returns error": {
270+
verificationMode: tlscommon.VerifyStrict,
271+
hostname: "127.0.0.1",
272+
ips: []net.IP{net.IPv4(10, 0, 0, 1)},
273+
expectingError: true,
274+
},
275+
"VerifyStrict IPv6 mismatch returns error": {
276+
verificationMode: tlscommon.VerifyStrict,
277+
hostname: "::1",
278+
ips: []net.IP{net.ParseIP("faca:b0de:baba::ca")},
279+
expectingError: true,
280+
},
281+
"VerifyStrict returns error when SNA is empty and legacy Common Name is used": {
282+
verificationMode: tlscommon.VerifyStrict,
283+
hostname: "localhost",
284+
commonName: "localhost",
285+
expectingError: true,
286+
},
287+
"VerifyStrict returns error when SNA is empty and legacy Common Name is used with IP address": {
288+
verificationMode: tlscommon.VerifyStrict,
289+
hostname: "127.0.0.1",
290+
commonName: "127.0.0.1",
291+
expectingError: true,
292+
},
293+
"VerifyStrict returns error when SNA is empty": {
294+
verificationMode: tlscommon.VerifyStrict,
295+
hostname: "localhost",
296+
expectingError: true,
297+
},
298+
299+
"VerifyCertificate does not validate domain": {
300+
verificationMode: tlscommon.VerifyCertificate,
301+
hostname: "localhost",
302+
dnsNames: []string{"example.com"},
303+
},
304+
"VerifyCertificate does not validate IPv4": {
305+
verificationMode: tlscommon.VerifyCertificate,
306+
hostname: "127.0.0.1",
307+
dnsNames: []string{"example.com"}, // I believe it cannot be empty
308+
},
309+
"VerifyCertificate does not validate IPv6": {
310+
verificationMode: tlscommon.VerifyCertificate,
311+
hostname: "127.0.0.1",
312+
ips: []net.IP{net.ParseIP("faca:b0de:baba::ca")},
313+
},
314+
315+
"VerifyNone accepts untrusted certificates": {
316+
verificationMode: tlscommon.VerifyNone,
317+
hostname: "127.0.0.1",
318+
ignoreCerts: true,
319+
},
320+
}
321+
caCert, err := tlscommontest.GenCA()
322+
if err != nil {
323+
t.Fatalf("could not generate root CA certificate: %s", err)
324+
}
325+
326+
for name, test := range testcases {
327+
t.Run(name, func(t *testing.T) {
328+
329+
// create certificates for server setup
330+
certs, err := tlscommontest.GenSignedCert(caCert, x509.KeyUsageCertSign, false, test.commonName, test.dnsNames, test.ips, false)
331+
if err != nil {
332+
t.Fatalf("could not generate certificates: %s", err)
333+
}
334+
// start server with generated cer
335+
serverURL := startTestServer(t, []tls.Certificate{certs})
336+
337+
// config to pass to authenticator
338+
cfg := &Config{
339+
TLS: &TLSConfig{
340+
VerificationMode: test.verificationMode.String(),
341+
},
342+
}
343+
344+
// create an instance of authenticator
345+
auth, err := newAuthenticator(cfg, settings)
346+
require.NoError(t, err)
347+
348+
host := extensionsMap{component.NewID(metadata.Type): auth}
349+
// starts the auth extension
350+
err = auth.Start(context.Background(), host)
351+
require.NoError(t, err)
352+
353+
httpClientConfig.TLS.ServerName = test.hostname
354+
httpClientConfig.TLS.CAPem = configopaque.String(string(
355+
pem.EncodeToMemory(&pem.Block{
356+
Type: "CERTIFICATE",
357+
Bytes: caCert.Leaf.Raw,
358+
})))
359+
360+
if test.ignoreCerts {
361+
httpClientConfig.TLS.ServerName = ""
362+
httpClientConfig.TLS.CAPem = ""
363+
}
364+
365+
client, err := httpClientConfig.ToClient(context.Background(), host, settings)
366+
require.NoError(t, err)
367+
368+
resp, err := client.Get(serverURL) //nolint:noctx // It is a test
369+
if err == nil {
370+
resp.Body.Close()
371+
}
372+
373+
if test.expectingError {
374+
if err != nil {
375+
// We got the expected error, no need to check the status code
376+
return
377+
} else {
378+
t.Fatalf("expected error, got: %v", err)
379+
}
380+
}
381+
382+
if err != nil {
383+
t.Fatalf("did not expect an error: %v", err)
384+
}
385+
386+
if resp.StatusCode != 200 {
387+
t.Fatalf("expecting 200 got: %d", resp.StatusCode)
388+
}
389+
})
390+
}
391+
}
392+
393+
// startTestServer starts a HTTP server for testing using the provided
394+
// certificates
395+
//
396+
// All requests are responded with an HTTP 200 OK and a plain
397+
// text string
398+
//
399+
// The HTTP server will shutdown at the end of the test.
400+
func startTestServer(t *testing.T, serverCerts []tls.Certificate) string {
401+
server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
402+
if _, err := w.Write([]byte("SSL test server")); err != nil {
403+
t.Errorf("coluld not write to client: %s", err)
404+
}
405+
}))
406+
server.TLS = &tls.Config{}
407+
server.TLS.Certificates = serverCerts
408+
server.StartTLS()
409+
t.Cleanup(func() { server.Close() })
410+
return server.URL
411+
}
412+
168413
type extensionsMap map[component.ID]component.Component
169414

170415
func (m extensionsMap) GetExtensions() map[component.ID]component.Component {

‎extension/beatsauthextension/config.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ type TLSConfig struct {
3939
VerificationMode string `mapstructure:"verification_mode"`
4040
CATrustedFingerprint string `mapstructure:"ca_trusted_fingerprint"`
4141
CASha256 []string `mapstructure:"ca_sha256"`
42-
ServerName string `mapstructure:"server_name"`
4342
}
4443

4544
func createDefaultConfig() component.Config {

‎extension/beatsauthextension/go.mod‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,14 @@ module github.com/elastic/opentelemetry-collector-components/extension/beatsauth
33
go 1.23.8
44

55
require (
6-
github.com/elastic/elastic-agent-libs v0.18.3
6+
github.com/elastic/elastic-agent-libs v0.21.6
77
github.com/stretchr/testify v1.10.0
88
github.com/tj/assert v0.0.3
99
go.opentelemetry.io/collector/component v1.36.0
1010
go.opentelemetry.io/collector/component/componenttest v0.130.0
1111
go.opentelemetry.io/collector/config/configauth v0.130.0
1212
go.opentelemetry.io/collector/config/confighttp v0.130.0
13+
go.opentelemetry.io/collector/config/configopaque v1.36.0
1314
go.opentelemetry.io/collector/config/configoptional v0.130.0
1415
go.opentelemetry.io/collector/confmap v1.36.0
1516
go.opentelemetry.io/collector/extension v1.36.0
@@ -50,7 +51,6 @@ require (
5051
go.opentelemetry.io/collector/client v1.36.0 // indirect
5152
go.opentelemetry.io/collector/config/configcompression v1.36.0 // indirect
5253
go.opentelemetry.io/collector/config/configmiddleware v0.130.0 // indirect
53-
go.opentelemetry.io/collector/config/configopaque v1.36.0 // indirect
5454
go.opentelemetry.io/collector/config/configtls v1.36.0 // indirect
5555
go.opentelemetry.io/collector/extension/extensionmiddleware v0.130.0 // indirect
5656
go.opentelemetry.io/collector/featuregate v1.36.0 // indirect

‎extension/beatsauthextension/go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ github.com/benbjohnson/clock v1.1.0/go.mod h1:J11/hYXuz8f4ySSvYwY0FKfm+ezbsZBKZx
22
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
33
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
44
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
5-
github.com/elastic/elastic-agent-libs v0.18.3 h1:bZ0e4Mrw7t6GnquTxzvZclGxZsUEIlsZNJPCPy2qO24=
6-
github.com/elastic/elastic-agent-libs v0.18.3/go.mod h1:rWdyrrAFzZwgNNi41Tsqhlt2c2GdXWhCEwcsnqISJ2U=
5+
github.com/elastic/elastic-agent-libs v0.21.6 h1:hvBAi4KHaYf4hn+rTc9m6A35eZjqb1uoE2exklIdWm0=
6+
github.com/elastic/elastic-agent-libs v0.21.6/go.mod h1:xSeIP3NtOIT4N2pPS4EyURmS1Q8mK0lWZ8Wd1Du6q3w=
77
github.com/elastic/go-ucfg v0.8.5 h1:4GB/rMpuh7qTcSFaxJUk97a/JyvFzhi6t+kaskTTLdM=
88
github.com/elastic/go-ucfg v0.8.5/go.mod h1:4E8mPOLSUV9hQ7sgLEJ4bvt0KhMuDJa8joDT2QGAEKA=
99
github.com/elastic/pkcs8 v1.0.0 h1:HhitlUKxhN288kcNcYkjW6/ouvuwJWd9ioxpjnD9jVA=

0 commit comments

Comments
 (0)