Merge branch '541_remove_self_signed_cert_option' into 'main'

Remove `self_signed_cert` option

See merge request gitlab-org/gitlab-shell!602

6 files changed, 16 insertions, 29 deletions

diff --git a/client/client_test.go b/client/client_test.go
index 48681f7..186f919 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -59,7 +59,7 @@ func TestClients(t *testing.T) {
 
 			secret := "sssh, it's a secret"
 
-			httpClient, err := NewHTTPClientWithOpts(url, tc.relativeURLRoot, tc.caFile, "", false, 1, nil)
+			httpClient, err := NewHTTPClientWithOpts(url, tc.relativeURLRoot, tc.caFile, "", 1, nil)
 			require.NoError(t, err)
 
 			client, err := NewGitlabNetClient("", "", secret, httpClient)
diff --git a/client/httpclient.go b/client/httpclient.go
index 5bbfbce..0a5b149 100644
--- a/client/httpclient.go
+++ b/client/httpclient.go
@@ -71,8 +71,8 @@ func validateCaFile(filename string) error {
 }
 
 // Deprecated: use NewHTTPClientWithOpts - https://gitlab.com/gitlab-org/gitlab-shell/-/issues/484
-func NewHTTPClient(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, selfSignedCert bool, readTimeoutSeconds uint64) *HttpClient {
-	c, err := NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath, selfSignedCert, readTimeoutSeconds, nil)
+func NewHTTPClient(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, readTimeoutSeconds uint64) *HttpClient {
+	c, err := NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath, readTimeoutSeconds, nil)
 	if err != nil {
 		log.WithError(err).Error("new http client with opts")
 	}
@@ -80,7 +80,7 @@ func NewHTTPClient(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, self
 }
 
 // NewHTTPClientWithOpts builds an HTTP client using the provided options
-func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, selfSignedCert bool, readTimeoutSeconds uint64, opts []HTTPClientOpt) (*HttpClient, error) {
+func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath string, readTimeoutSeconds uint64, opts []HTTPClientOpt) (*HttpClient, error) {
 	var transport *http.Transport
 	var host string
 	var err error
@@ -103,7 +103,7 @@ func NewHTTPClientWithOpts(gitlabURL, gitlabRelativeURLRoot, caFile, caPath stri
 			opt(hcc)
 		}
 
-		transport, host, err = buildHttpsTransport(*hcc, selfSignedCert, gitlabURL)
+		transport, host, err = buildHttpsTransport(*hcc, gitlabURL)
 		if err != nil {
 			return nil, err
 		}
@@ -140,7 +140,7 @@ func buildSocketTransport(gitlabURL, gitlabRelativeURLRoot string) (*http.Transp
 	return transport, host
 }
 
-func buildHttpsTransport(hcc httpClientCfg, selfSignedCert bool, gitlabURL string) (*http.Transport, string, error) {
+func buildHttpsTransport(hcc httpClientCfg, gitlabURL string) (*http.Transport, string, error) {
 	certPool, err := x509.SystemCertPool()
 
 	if err != nil {
@@ -162,12 +162,8 @@ func buildHttpsTransport(hcc httpClientCfg, selfSignedCert bool, gitlabURL strin
 		}
 	}
 	tlsConfig := &tls.Config{
-		RootCAs: certPool,
-		// The self_signed_cert config setting is deprecated
-		// The field and its usage is going to be removed in
-		// https://gitlab.com/gitlab-org/gitlab-shell/-/issues/541
-		InsecureSkipVerify: selfSignedCert,
-		MinVersion:         tls.VersionTLS12,
+		RootCAs:    certPool,
+		MinVersion: tls.VersionTLS12,
 	}
 
 	if hcc.HaveCertAndKey() {
diff --git a/client/httpclient_test.go b/client/httpclient_test.go
index f7a6340..7804b51 100644
--- a/client/httpclient_test.go
+++ b/client/httpclient_test.go
@@ -17,7 +17,7 @@ import (
 func TestReadTimeout(t *testing.T) {
 	expectedSeconds := uint64(300)
 
-	client, err := NewHTTPClientWithOpts("http://localhost:3000", "", "", "", false, expectedSeconds, nil)
+	client, err := NewHTTPClientWithOpts("http://localhost:3000", "", "", "", expectedSeconds, nil)
 	require.NoError(t, err)
 
 	require.NotNil(t, client)
@@ -123,7 +123,7 @@ func TestRequestWithUserAgent(t *testing.T) {
 func setup(t *testing.T, username, password string, requests []testserver.TestRequestHandler) *GitlabNetClient {
 	url := testserver.StartHttpServer(t, requests)
 
-	httpClient, err := NewHTTPClientWithOpts(url, "", "", "", false, 1, nil)
+	httpClient, err := NewHTTPClientWithOpts(url, "", "", "", 1, nil)
 	require.NoError(t, err)
 
 	client, err := NewGitlabNetClient(username, password, "", httpClient)
diff --git a/client/httpsclient_test.go b/client/httpsclient_test.go
index debe1bd..2ab0605 100644
--- a/client/httpsclient_test.go
+++ b/client/httpsclient_test.go
@@ -18,7 +18,6 @@ func TestSuccessfulRequests(t *testing.T) {
 	testCases := []struct {
 		desc                                        string
 		caFile, caPath                              string
-		selfSigned                                  bool
 		clientCAPath, clientCertPath, clientKeyPath string // used for TLS client certs
 	}{
 		{
@@ -31,9 +30,8 @@ func TestSuccessfulRequests(t *testing.T) {
 			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
 		},
 		{
-			desc:       "Invalid cert with self signed cert option enabled",
-			caFile:     path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
-			selfSigned: true,
+			desc:   "Invalid cert with self signed cert option enabled",
+			caFile: path.Join(testhelper.TestRoot, "certs/valid/server.crt"),
 		},
 		{
 			desc:   "Client certs with CA",
@@ -48,7 +46,7 @@ func TestSuccessfulRequests(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			client, err := setupWithRequests(t, tc.caFile, tc.caPath, tc.clientCAPath, tc.clientCertPath, tc.clientKeyPath, tc.selfSigned)
+			client, err := setupWithRequests(t, tc.caFile, tc.caPath, tc.clientCAPath, tc.clientCertPath, tc.clientKeyPath)
 			require.NoError(t, err)
 
 			response, err := client.Get(context.Background(), "/hello")
@@ -95,7 +93,7 @@ func TestFailedRequests(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			client, err := setupWithRequests(t, tc.caFile, tc.caPath, "", "", "", false)
+			client, err := setupWithRequests(t, tc.caFile, tc.caPath, "", "", "")
 			if tc.expectedCaFileNotFound {
 				require.Error(t, err)
 				require.ErrorIs(t, err, ErrCafileNotFound)
@@ -109,7 +107,7 @@ func TestFailedRequests(t *testing.T) {
 	}
 }
 
-func setupWithRequests(t *testing.T, caFile, caPath, clientCAPath, clientCertPath, clientKeyPath string, selfSigned bool) (*GitlabNetClient, error) {
+func setupWithRequests(t *testing.T, caFile, caPath, clientCAPath, clientCertPath, clientKeyPath string) (*GitlabNetClient, error) {
 	testhelper.PrepareTestRootDir(t)
 
 	requests := []testserver.TestRequestHandler{
@@ -130,7 +128,7 @@ func setupWithRequests(t *testing.T, caFile, caPath, clientCAPath, clientCertPat
 		opts = append(opts, WithClientCert(clientCertPath, clientKeyPath))
 	}
 
-	httpClient, err := NewHTTPClientWithOpts(url, "", caFile, caPath, selfSigned, 1, opts)
+	httpClient, err := NewHTTPClientWithOpts(url, "", caFile, caPath, 1, opts)
 	if err != nil {
 		return nil, err
 	}
diff --git a/config.yml.example b/config.yml.example
index 3714d73..579bf3c 100644
--- a/config.yml.example
+++ b/config.yml.example
@@ -27,11 +27,6 @@ http_settings:
 #  ca_file: /etc/ssl/cert.pem
 #  ca_path: /etc/pki/tls/certs
 #
-#  The self_signed_cert option is deprecated
-#  When it's set to true, any certificate is accepted, which may make machine-in-the-middle attack possible
-#  Certificates specified in ca_file and ca_path are trusted anyway even if they are self-signed
-#  Issue: https://gitlab.com/gitlab-org/gitlab-shell/-/issues/120
-  self_signed_cert: false
 
 # File used as authorized_keys for gitlab user
 auth_file: "/home/git/.ssh/authorized_keys"
diff --git a/internal/config/config.go b/internal/config/config.go
index c4dc073..ff0c79a 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -38,7 +38,6 @@ type HttpSettingsConfig struct {
 	ReadTimeoutSeconds uint64 `yaml:"read_timeout"`
 	CaFile             string `yaml:"ca_file"`
 	CaPath             string `yaml:"ca_path"`
-	SelfSignedCert     bool   `yaml:"self_signed_cert"`
 }
 
 type Config struct {
@@ -106,7 +105,6 @@ func (c *Config) HttpClient() (*client.HttpClient, error) {
 			c.GitlabRelativeURLRoot,
 			c.HttpSettings.CaFile,
 			c.HttpSettings.CaPath,
-			c.HttpSettings.SelfSignedCert,
 			c.HttpSettings.ReadTimeoutSeconds,
 			nil,
 		)