Skip to content

Commit ba705ec

Browse files
gwossumgwossum
authored
fix: [httpd] https-insecure-certificate not handled properly (#27219)
Fix issue with [httpd] https-insecure-certificate not properly ignoring certificate file permissions. Added tests for verifying the httpd service's TLSConfigManager is properly configured from the configuration. This is a clean cherry-pick from master-1.x. (cherry picked from commit 945caaf)
1 parent 0f3436d commit ba705ec

File tree

2 files changed

+175
-14
lines changed

2 files changed

+175
-14
lines changed

‎services/httpd/service.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ func (s *Service) Open() error {
160160

161161
// Open listener.
162162
tm, err := tlsconfig.NewTLSConfigManager(s.https, s.tlsConfig, s.cert, s.key, false,
163-
tlsconfig.WithAllowInsecure(s.insecureCert),
163+
tlsconfig.WithIgnoreFilePermissions(s.insecureCert),
164164
tlsconfig.WithLogger(s.Logger))
165165
if err != nil {
166166
return fmt.Errorf("httpd: error creating TLS manager: %w", err)

‎services/httpd/service_test.go‎

Lines changed: 174 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,13 @@ package httpd_test
33
import (
44
"crypto/tls"
55
"fmt"
6+
"math/big"
7+
"net"
68
"net/http"
9+
"os"
710
"testing"
811

12+
th "github.com/influxdata/influxdb/pkg/testing/helper"
913
"github.com/influxdata/influxdb/pkg/testing/selfsigned"
1014
"github.com/influxdata/influxdb/pkg/tlsconfig"
1115
"github.com/influxdata/influxdb/services/httpd"
@@ -46,9 +50,7 @@ func TestService_VerifyReloadedConfig(t *testing.T) {
4650

4751
// Open service to initialize certLoader
4852
require.NoError(t, s.Open())
49-
defer func() {
50-
require.NoError(t, s.Close())
51-
}()
53+
defer th.CheckedClose(t, s)()
5254

5355
// Try to verify reload with HTTPS disabled
5456
newConfig := httpd.Config{
@@ -98,9 +100,7 @@ func TestService_VerifyReloadedConfig(t *testing.T) {
98100

99101
// Open service to initialize certLoader
100102
require.NoError(t, s.Open())
101-
defer func() {
102-
require.NoError(t, s.Close())
103-
}()
103+
defer th.CheckedClose(t, s)()
104104

105105
// Get the initial certificate serial number for comparison
106106
initialCert, err := tlsconfig.LoadCertificate(ss1.CertPath, ss1.KeyPath)
@@ -173,9 +173,7 @@ func TestService_VerifyReloadedConfig(t *testing.T) {
173173

174174
// Open service to initialize certLoader
175175
require.NoError(t, s.Open())
176-
defer func() {
177-
require.NoError(t, s.Close())
178-
}()
176+
defer th.CheckedClose(t, s)()
179177

180178
// Try to verify reload with non-existent certificate (one example of VerifyLoad failure)
181179
newConfig := httpd.Config{
@@ -188,7 +186,7 @@ func TestService_VerifyReloadedConfig(t *testing.T) {
188186
require.Nil(t, applyFunc)
189187
})
190188

191-
t.Run("HTTPS enabled, no certificate change", func(t *testing.T) {
189+
t.Run("HTTPS enabled, no certificate change (same paths)", func(t *testing.T) {
192190
// Create initial certificates
193191
ss := selfsigned.NewSelfSignedCert(t)
194192

@@ -204,9 +202,7 @@ func TestService_VerifyReloadedConfig(t *testing.T) {
204202

205203
// Open service to initialize certLoader
206204
require.NoError(t, s.Open())
207-
defer func() {
208-
require.NoError(t, s.Close())
209-
}()
205+
defer th.CheckedClose(t, s)()
210206

211207
// Get the certificate serial number
212208
cert, err := tlsconfig.LoadCertificate(ss.CertPath, ss.KeyPath)
@@ -255,3 +251,168 @@ func TestService_VerifyReloadedConfig(t *testing.T) {
255251
}
256252
})
257253
}
254+
255+
// openService creates, opens, and registers cleanup for an httpd.Service.
256+
func openService(t *testing.T, config httpd.Config) *httpd.Service {
257+
t.Helper()
258+
s := httpd.NewService(config)
259+
s.WithLogger(zap.NewNop())
260+
require.NoError(t, s.Open())
261+
t.Cleanup(th.CheckedClose(t, s))
262+
return s
263+
}
264+
265+
// insecureHTTPSClient returns an *http.Client that skips TLS certificate verification.
266+
func insecureHTTPSClient() *http.Client {
267+
return &http.Client{Transport: &http.Transport{
268+
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
269+
}}
270+
}
271+
272+
// pingHTTPS performs a GET /ping over HTTPS using the given client and returns the response.
273+
func pingHTTPS(t *testing.T, addr net.Addr, client *http.Client) *http.Response {
274+
t.Helper()
275+
resp, err := client.Get(fmt.Sprintf("https://%s/ping", addr))
276+
require.NoError(t, err)
277+
t.Cleanup(func() { resp.Body.Close() })
278+
return resp
279+
}
280+
281+
// requireServingCertSerial verifies the service is serving TLS with the expected certificate serial.
282+
func requireServingCertSerial(t *testing.T, s *httpd.Service, expectedSerial *big.Int) {
283+
t.Helper()
284+
resp := pingHTTPS(t, s.Addr(), insecureHTTPSClient())
285+
require.NotNil(t, resp.TLS)
286+
require.NotEmpty(t, resp.TLS.PeerCertificates)
287+
require.Equal(t, expectedSerial, resp.TLS.PeerCertificates[0].SerialNumber)
288+
}
289+
290+
// loadCertSerial loads a certificate and returns its serial number.
291+
func loadCertSerial(t *testing.T, certPath, keyPath string) *big.Int {
292+
t.Helper()
293+
cert, err := tlsconfig.LoadCertificate(certPath, keyPath)
294+
require.NoError(t, err)
295+
return cert.Leaf.SerialNumber
296+
}
297+
298+
// TestService_Open_TLSConfigManager verifies that the TLSConfigManager created
299+
// in Service.Open is configured properly based on the Service's configuration.
300+
func TestService_Open_TLSConfigManager(t *testing.T) {
301+
t.Run("HTTPS disabled creates non-TLS listener", func(t *testing.T) {
302+
s := openService(t, httpd.Config{
303+
BindAddress: "localhost:",
304+
HTTPSEnabled: false,
305+
})
306+
307+
resp, err := http.Get(fmt.Sprintf("http://%s/ping", s.Addr()))
308+
require.NoError(t, err)
309+
resp.Body.Close()
310+
require.Equal(t, http.StatusNoContent, resp.StatusCode)
311+
require.Nil(t, resp.TLS)
312+
})
313+
314+
t.Run("HTTPS enabled creates TLS listener with correct certificate", func(t *testing.T) {
315+
ss := selfsigned.NewSelfSignedCert(t)
316+
s := openService(t, httpd.Config{
317+
BindAddress: "localhost:",
318+
HTTPSEnabled: true,
319+
HTTPSCertificate: ss.CertPath,
320+
HTTPSPrivateKey: ss.KeyPath,
321+
})
322+
323+
requireServingCertSerial(t, s, loadCertSerial(t, ss.CertPath, ss.KeyPath))
324+
})
325+
326+
t.Run("HTTPS enabled with missing certificate fails Open", func(t *testing.T) {
327+
s := httpd.NewService(httpd.Config{
328+
BindAddress: "localhost:",
329+
HTTPSEnabled: true,
330+
HTTPSCertificate: "/nonexistent/cert.pem",
331+
HTTPSPrivateKey: "/nonexistent/key.pem",
332+
})
333+
s.WithLogger(zap.NewNop())
334+
335+
err := s.Open()
336+
require.Error(t, err)
337+
require.ErrorContains(t, err, "error creating TLS manager")
338+
})
339+
340+
t.Run("HTTPS enabled applies base TLS config MinVersion", func(t *testing.T) {
341+
ss := selfsigned.NewSelfSignedCert(t)
342+
s := openService(t, httpd.Config{
343+
BindAddress: "localhost:",
344+
HTTPSEnabled: true,
345+
HTTPSCertificate: ss.CertPath,
346+
HTTPSPrivateKey: ss.KeyPath,
347+
TLS: &tls.Config{MinVersion: tls.VersionTLS13},
348+
})
349+
350+
pingURI := fmt.Sprintf("https://%s/ping", s.Addr())
351+
352+
// A client restricted to TLS 1.2 should fail the handshake.
353+
tls12Client := &http.Client{Transport: &http.Transport{
354+
TLSClientConfig: &tls.Config{
355+
InsecureSkipVerify: true,
356+
MinVersion: tls.VersionTLS12,
357+
MaxVersion: tls.VersionTLS12,
358+
},
359+
}}
360+
_, err := tls12Client.Get(pingURI)
361+
require.Error(t, err, "TLS 1.2 client should be rejected when server requires TLS 1.3")
362+
363+
// A client using TLS 1.3 should succeed.
364+
resp := pingHTTPS(t, s.Addr(), insecureHTTPSClient())
365+
require.Equal(t, http.StatusNoContent, resp.StatusCode)
366+
require.Equal(t, uint16(tls.VersionTLS13), resp.TLS.Version)
367+
})
368+
369+
t.Run("insecureCert=false rejects permissive key file permissions", func(t *testing.T) {
370+
ss := selfsigned.NewSelfSignedCert(t)
371+
require.NoError(t, os.Chmod(ss.KeyPath, 0644))
372+
373+
s := httpd.NewService(httpd.Config{
374+
BindAddress: "localhost:",
375+
HTTPSEnabled: true,
376+
HTTPSCertificate: ss.CertPath,
377+
HTTPSPrivateKey: ss.KeyPath,
378+
HTTPSInsecureCertificate: false,
379+
})
380+
s.WithLogger(zap.NewNop())
381+
382+
err := s.Open()
383+
require.Error(t, err, "Open should fail when key file permissions are too permissive")
384+
require.ErrorContains(t, err, "error creating TLS manager")
385+
})
386+
387+
t.Run("insecureCert=true allows permissive key file permissions", func(t *testing.T) {
388+
ss := selfsigned.NewSelfSignedCert(t)
389+
require.NoError(t, os.Chmod(ss.KeyPath, 0644))
390+
391+
s := openService(t, httpd.Config{
392+
BindAddress: "localhost:",
393+
HTTPSEnabled: true,
394+
HTTPSCertificate: ss.CertPath,
395+
HTTPSPrivateKey: ss.KeyPath,
396+
HTTPSInsecureCertificate: true,
397+
})
398+
399+
resp := pingHTTPS(t, s.Addr(), insecureHTTPSClient())
400+
require.Equal(t, http.StatusNoContent, resp.StatusCode)
401+
require.NotNil(t, resp.TLS)
402+
})
403+
404+
t.Run("HTTPS enabled with combined cert and key file", func(t *testing.T) {
405+
ss := selfsigned.NewSelfSignedCert(t, selfsigned.WithCombinedFile())
406+
s := openService(t, httpd.Config{
407+
BindAddress: "localhost:",
408+
HTTPSEnabled: true,
409+
HTTPSCertificate: ss.CertPath,
410+
HTTPSPrivateKey: "",
411+
})
412+
413+
resp := pingHTTPS(t, s.Addr(), insecureHTTPSClient())
414+
require.Equal(t, http.StatusNoContent, resp.StatusCode)
415+
require.NotNil(t, resp.TLS)
416+
require.NotEmpty(t, resp.TLS.PeerCertificates)
417+
})
418+
}

0 commit comments

Comments
 (0)