test coverage

Author: mrogala-sauce

command/run/run.go

@@ -131,7 +131,7 @@ func (c *command) runE(cmd *cobra.Command, _ []string) (cmdErr error) {
 	// Configure Kerberos as first because HTTP Transport
 	// for various forwarder elements may require reaching hosts behind
 	// Kerberos authenticated upstream proxy
-	var kerberosAdapter *forwarder.KerberosAdapter = nil
+	var kerberosAdapter forwarder.KerberosAdapter = nil
 
 	// use separate flag for determining if Kerberos is enabled
 	// than presence of config file, this may change in the future
@@ -340,7 +340,7 @@ func (c *command) configureHeadersModifiers() {
 }
 
 // configure upstream proxy transport - connect headers and/or Kerberos auth
-func (c *command) configureTransportProxy(tr *http.Transport, kerberosAdapter *forwarder.KerberosAdapter) {
+func (c *command) configureTransportProxy(tr *http.Transport, kerberosAdapter forwarder.KerberosAdapter) {
 	headersToAllocate := len(c.connectHeaders)
 
 	if c.kerberosConfig.AuthUpstreamProxy {

dialvia/http_test.go

@@ -88,6 +88,50 @@ func TestHTTPProxyDialerDialContext(t *testing.T) {
 		}
 	})
 
+	t.Run("CONNECT headers from GetProxyConnectHeaders", func(t *testing.T) {
+
+		d.GetProxyConnectHeader = func(_ context.Context, proxyURL *url.URL, _ string) (http.Header, error) {
+			authHeader := make(http.Header, 1)
+			authHeader.Set("Proxy-Authorization", "TEST-PROXY-AUTHORIZATION")
+			return authHeader, nil
+
+		}
+
+		errCh := make(chan error, 1)
+		go func() {
+			errCh <- serveOne(l, func(conn net.Conn) error {
+				pbr := bufio.NewReader(conn)
+				req, err := http.ReadRequest(pbr)
+				if err != nil {
+					return err
+				}
+
+				if req.Method != http.MethodConnect {
+					return fmt.Errorf("HTTP CONNECT method expected")
+				}
+
+				if req.Header.Get("Proxy-Authorization") != "TEST-PROXY-AUTHORIZATION" {
+					return fmt.Errorf("Proxy-Authorization header expected but not present")
+				}
+
+				return proxyutil.NewResponse(404, nil, req).Write(conn)
+			})
+		}()
+
+		conn, err := d.DialContext(ctx, "tcp", "foobar.com:80")
+		if err == nil {
+			t.Fatal("err is nil")
+		}
+		t.Log(err)
+		if conn != nil {
+			t.Fatal("conn is not nil")
+		}
+
+		if err := <-errCh; err != nil {
+			t.Fatal(err)
+		}
+	})
+
 	t.Run("response with data", func(t *testing.T) {
 		errCh := make(chan error, 1)
 		go func() {

docs/content/cli/forwarder_run.md

@@ -694,7 +694,7 @@ List of hosts for which Kerberos (SPNEGO) authorization tokens will be injected
 * Value Format: `<value>` (you can use empty command line switch to enable)
 * Default Value: `false`
 
-Authenticate to a configured upstream proxy with Kerberos (using `Proxy-Authorization` HTTP header in CONNECT requests). Please note that if forwarder configuration results in multiple proxies available (like PAC for example), forwarder will try to authenticate to each one of them.
+Authenticate to a configured upstream proxy with Kerberos (using `Proxy-Authorization` HTTP header). Please note that if forwarder configuration results in multiple proxies available (like PAC for example), forwarder will try to authenticate to each one of them.
 
 
 ### --kerberos-run-diagnostics {#kerberos-run-diagnostics}
@@ -704,7 +704,7 @@ Authenticate to a configured upstream proxy with Kerberos (using `Proxy-Authoriz
 * Default Value: `false`
 
 
-Running forwarder with `--kerberos-run-diagnostics` switch will print debugging information about Kerberos connection and an error when there are discrepancies between supported encryption types and keytab entry:
+Running forwarder with `--kerberos-run-diagnostics` switch will print debugging information about Kerberos connection or known configuration erros - for example an error when there are discrepancies between supported encryption types and keytab entry:
 
 ```
  msg="fatal error exiting" error="kerberos configuration potential problems: default_tkt_enctypes specifies 17 but this enctype is not available in the client's keytab\ndefault_tkt_enctypes specifies 23 but this enctype is not available in the client's keytab\npreferred_preauth_types specifies 17 but this enctype is not available in the client's keytab\npreferred_preauth_types specifies 15 but this enctype is not available in the client's keytab\npreferred_preauth_types specifies 14 but this enctype is not available in the client's keytab"

http_proxy.go

@@ -155,7 +155,7 @@ type HTTPProxy struct {
 	proxy           *martian.Proxy
 	mitmCACert      *x509.Certificate
 	proxyFunc       ProxyFunc
-	kerberosAdapter *KerberosAdapter
+	kerberosAdapter KerberosAdapter
 	localhost       []string
 
 	tlsConfig *tls.Config
@@ -165,7 +165,7 @@ type HTTPProxy struct {
 // NewHTTPProxy creates a new HTTP proxy.
 // It is the caller's responsibility to call Close on the returned server.
 func NewHTTPProxy(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMatcher, rt http.RoundTripper, log log.StructuredLogger,
-	kerberosAdapter *KerberosAdapter,
+	kerberosAdapter KerberosAdapter,
 ) (*HTTPProxy, error) {
 	hp, err := newHTTPProxy(cfg, pr, cm, rt, log, kerberosAdapter)
 	if err != nil {
@@ -205,7 +205,7 @@ func NewHTTPProxy(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMatcher,
 }
 
 // NewHTTPProxyHandler is like NewHTTPProxy but returns http.Handler instead of *HTTPProxy.
-func NewHTTPProxyHandler(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMatcher, rt http.RoundTripper, log log.StructuredLogger, kerberosAdapter *KerberosAdapter) (http.Handler, error) {
+func NewHTTPProxyHandler(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMatcher, rt http.RoundTripper, log log.StructuredLogger, kerberosAdapter KerberosAdapter) (http.Handler, error) {
 	hp, err := newHTTPProxy(cfg, pr, cm, rt, log, kerberosAdapter)
 	if err != nil {
 		return nil, err
@@ -214,7 +214,7 @@ func NewHTTPProxyHandler(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMa
 	return hp.handler(), nil
 }
 
-func newHTTPProxy(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMatcher, rt http.RoundTripper, log log.StructuredLogger, kerberosAdapter *KerberosAdapter) (*HTTPProxy, error) {
+func newHTTPProxy(cfg *HTTPProxyConfig, pr PACResolver, cm *CredentialsMatcher, rt http.RoundTripper, log log.StructuredLogger, kerberosAdapter KerberosAdapter) (*HTTPProxy, error) {
 	if err := cfg.Validate(); err != nil {
 		return nil, err
 	}
@@ -353,7 +353,7 @@ func (hp *HTTPProxy) upstreamProxyURL() *url.URL {
 	// to auth upstream proxy and clear existing auth data
 	// so http.RoundTripper would not try to add custom Authorization header
 
-	if hp.kerberosAdapter != nil && hp.kerberosAdapter.configuration.AuthUpstreamProxy {
+	if hp.kerberosAdapter != nil && hp.kerberosAdapter.GetConfig().AuthUpstreamProxy {
 
 		proxyURL.User = nil
 		return proxyURL
@@ -385,7 +385,7 @@ func (hp *HTTPProxy) pacProxy(r *http.Request) (*url.URL, error) {
 	// to auth upstream proxy and clear existing auth data
 	// so http.RoundTripper would not try to add custom Authorization header
 
-	if hp.kerberosAdapter != nil && hp.kerberosAdapter.configuration.AuthUpstreamProxy {
+	if hp.kerberosAdapter != nil && hp.kerberosAdapter.GetConfig().AuthUpstreamProxy {
 
 		proxyURL.User = nil
 		return proxyURL, nil
@@ -426,7 +426,7 @@ func (hp *HTTPProxy) middlewareStack() (martian.RequestResponseModifier, *martia
 		stack.AddRequestModifier(hp.injectKerberosSPNEGOAuthentication())
 	}
 
-	if hp.kerberosAdapter != nil && hp.kerberosAdapter.configuration.AuthUpstreamProxy && hp.proxyFunc != nil {
+	if hp.kerberosAdapter != nil && hp.kerberosAdapter.GetConfig().AuthUpstreamProxy && hp.proxyFunc != nil {
 		stack.AddRequestModifier(hp.injectKerberosUpstreamProxyAuthorizationHeader())
 	}
 
@@ -487,7 +487,7 @@ func (hp *HTTPProxy) injectKerberosSPNEGOAuthentication() martian.RequestModifie
 
 	return martian.RequestModifierFunc(func(req *http.Request) error {
 		// TODO: use a map or something faster than array lookup
-		if slices.Contains(hp.kerberosAdapter.configuration.KerberosEnabledHosts, strings.ToLower(req.URL.Hostname())) {
+		if slices.Contains(hp.kerberosAdapter.GetConfig().KerberosEnabledHosts, strings.ToLower(req.URL.Hostname())) {
 			spn, err := hp.kerberosAdapter.GetSPNForHost(req.URL.Hostname())
 			if err != nil {
 				return err

http_proxy_test.go

@@ -18,6 +18,7 @@ import (
 	"testing"
 
 	"github.com/saucelabs/forwarder/log/slog"
+	"github.com/stretchr/testify/assert"
 	"golang.org/x/net/http2"
 )
 
@@ -199,3 +200,43 @@ func TestErrorResponse(t *testing.T) {
 		}
 	})
 }
+
+type KerberosAdapterMock struct {
+	KDCConnected bool
+}
+
+func (a *KerberosAdapterMock) ConnectToKDC() error {
+	a.KDCConnected = true
+	return nil
+}
+
+func (a *KerberosAdapterMock) GetConfig() *KerberosConfig {
+	return DefaultKerberosConfig()
+}
+
+func (a *KerberosAdapterMock) GetSPNForHost(hostname string) (string, error) {
+	return hostname, nil
+}
+func (a *KerberosAdapterMock) GetSPNEGOHeaderValue(spn string) (string, error) {
+	return "TEST-TOKEN", nil
+}
+
+func (a *KerberosAdapterMock) GetProxyAuthHeader(_ context.Context, proxyURL *url.URL, _ string) (http.Header, error) {
+	return nil, nil
+}
+
+func TestKerberosAuth(t *testing.T) {
+	cfg := DefaultHTTPProxyConfig()
+	cfg.ProxyLocalhost = AllowProxyLocalhost
+
+	kerberosAdapter := KerberosAdapterMock{}
+
+	_, err := NewHTTPProxyHandler(cfg, nil, nil, nil, slog.Default(), &kerberosAdapter)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Run("KDC connected", func(t *testing.T) {
+		assert.True(t, kerberosAdapter.KDCConnected)
+	})
+}

kerberos.go

@@ -34,19 +34,27 @@ type KerberosConfig struct {
 	KerberosEnabledHosts []string
 }
 
+type KerberosAdapter interface {
+	ConnectToKDC() error
+	GetSPNForHost(hostname string) (string, error)
+	GetSPNEGOHeaderValue(spn string) (string, error)
+	GetConfig() *KerberosConfig
+	GetProxyAuthHeader(_ context.Context, proxyURL *url.URL, _ string) (http.Header, error)
+}
+
 func DefaultKerberosConfig() *KerberosConfig {
 	return &KerberosConfig{
 		// default zero values are fine
 	}
 }
 
-type KerberosAdapter struct {
+type KerberosClient struct {
 	configuration KerberosConfig
 	krb5client    client.Client
 	log           log.StructuredLogger
 }
 
-func NewKerberosAdapter(cnf KerberosConfig, log log.StructuredLogger) (*KerberosAdapter, error) {
+func NewKerberosAdapter(cnf KerberosConfig, log log.StructuredLogger) (*KerberosClient, error) {
 	// technically this should not happen as adapter should not be initialized without
 	// proper config present, but better safe than sorry
 	if cnf.CfgFilePath == "" {
@@ -57,6 +65,13 @@ func NewKerberosAdapter(cnf KerberosConfig, log log.StructuredLogger) (*Kerberos
 		return nil, fmt.Errorf("kerberos keytab file not specified")
 	}
 
+	if cnf.UserName == "" {
+		return nil, fmt.Errorf("kerberos username not specified")
+	}
+	if cnf.UserRealm == "" {
+		return nil, fmt.Errorf("kerberos user realm not specified")
+	}
+
 	krb5Config, err := config.Load(cnf.CfgFilePath)
 	if err != nil {
 		return nil, fmt.Errorf("error loading kerberos config file %s: %w", cnf.CfgFilePath, err)
@@ -67,19 +82,16 @@ func NewKerberosAdapter(cnf KerberosConfig, log log.StructuredLogger) (*Kerberos
 		return nil, fmt.Errorf("error loading kerberos keytab file %s: %w", cnf.KeyTabFilePath, err)
 	}
 
-	if cnf.UserName == "" {
-		return nil, fmt.Errorf("kerberos username not specified")
-	}
-	if cnf.UserRealm == "" {
-		return nil, fmt.Errorf("kerberos user realm not specified")
-	}
-
 	krb5Client := client.NewWithKeytab(cnf.UserName, cnf.UserRealm, krb5Keytab, krb5Config)
 
-	return &KerberosAdapter{configuration: cnf, krb5client: *krb5Client, log: log}, nil
+	return &KerberosClient{configuration: cnf, krb5client: *krb5Client, log: log}, nil
+}
+
+func (a *KerberosClient) GetConfig() *KerberosConfig {
+	return &a.configuration
 }
 
-func (a *KerberosAdapter) ConnectToKDC() error {
+func (a *KerberosClient) ConnectToKDC() error {
 	a.log.Debug("Logging to KDC server")
 	loginErr := a.krb5client.Login()
 	if loginErr != nil && !a.configuration.RunDiagnostics {
@@ -113,14 +125,14 @@ func (a *KerberosAdapter) ConnectToKDC() error {
 	return nil
 }
 
-func (a *KerberosAdapter) GetSPNForHost(hostname string) (string, error) {
+func (a *KerberosClient) GetSPNForHost(hostname string) (string, error) {
 	// static for now but in the future we may want to do DNS queries for CNAME
 	return "HTTP/" + hostname, nil
 }
 
 // GetSPNEGOHeaderValue accepts SPN service name and returns header value that should
 // be put inside Authorization or Proxy-Authorization header.
-func (a *KerberosAdapter) GetSPNEGOHeaderValue(spn string) (string, error) {
+func (a *KerberosClient) GetSPNEGOHeaderValue(spn string) (string, error) {
 	a.log.Debug("Generating SPNEGO header value for SPN: ", spn)
 
 	cli := spnego.SPNEGOClient(&a.krb5client, spn)
@@ -141,7 +153,7 @@ func (a *KerberosAdapter) GetSPNEGOHeaderValue(spn string) (string, error) {
 	return "Negotiate " + base64.StdEncoding.EncodeToString(nb), nil
 }
 
-func (a *KerberosAdapter) GetProxyAuthHeader(_ context.Context, proxyURL *url.URL, _ string) (http.Header, error) {
+func (a *KerberosClient) GetProxyAuthHeader(_ context.Context, proxyURL *url.URL, _ string) (http.Header, error) {
 	spn := "HTTP/" + proxyURL.Hostname()
 
 	SPNEGOHeaderValue, err := a.GetSPNEGOHeaderValue(spn)

kerberos_test.go

@@ -10,14 +10,32 @@ import (
 	"testing"
 
 	"github.com/saucelabs/forwarder/log/slog"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestKerberosAdapterFailsWithoutConfig(t *testing.T) {
 	cnf := KerberosConfig{}
 
 	_, err := NewKerberosAdapter(cnf, slog.Default())
-	// TODO: match expected error message
-	if err != nil {
-		t.Fatal(err)
-	}
+	assert.NotNil(t, err)
+	assert.ErrorContains(t, err, "kerberos config file (krb5.conf) not specified")
+
+	cnf.CfgFilePath = "/tmp/test.cfg"
+
+	_, err = NewKerberosAdapter(cnf, slog.Default())
+	assert.NotNil(t, err)
+	assert.ErrorContains(t, err, "kerberos keytab file not specified")
+
+	cnf.KeyTabFilePath = "/tmp/keytab"
+
+	_, err = NewKerberosAdapter(cnf, slog.Default())
+	assert.NotNil(t, err)
+	assert.ErrorContains(t, err, "kerberos username not specified")
+
+	cnf.UserName = "user1"
+
+	_, err = NewKerberosAdapter(cnf, slog.Default())
+	assert.NotNil(t, err)
+	assert.ErrorContains(t, err, "kerberos user realm not specified")
+
 }