Apply config defaults and merge OIDC configuration

internal/config.go

@@ -16,18 +16,29 @@ package internal
 
 import (
 	"errors"
+	"fmt"
 	"os"
 
 	"github.com/tetratelabs/run"
 	"google.golang.org/protobuf/encoding/protojson"
+	"google.golang.org/protobuf/proto"
 
 	configv1 "github.com/tetrateio/authservice-go/config/gen/go/v1"
+	oidcv1 "github.com/tetrateio/authservice-go/config/gen/go/v1/oidc"
 )
 
-var _ run.Config = (*LocalConfigFile)(nil)
+var (
+	_ run.Config = (*LocalConfigFile)(nil)
 
-// ErrInvalidPath is returned when the configuration file path is invalid.
-var ErrInvalidPath = errors.New("invalid path")
+	// ErrInvalidPath is returned when the configuration file path is invalid.
+	ErrInvalidPath = errors.New("invalid path")
+	// ErrInvalidOIDCOverride is returned when the OIDC override is invalid.
+	ErrInvalidOIDCOverride = errors.New("invalid OIDC override")
+	// ErrDuplicateOIDCConfig is returned when the OIDC configuration is duplicated.
+	ErrDuplicateOIDCConfig = errors.New("duplicate OIDC configuration")
+	// ErrMultipleOIDCConfig is returned  ultiple OIDC configurations are set in the same filter chain.
+	ErrMultipleOIDCConfig = errors.New("multiple OIDC configurations")
+)
 
 // LocalConfigFile is a run.Config that loads the configuration file.
 type LocalConfigFile struct {
@@ -61,12 +72,58 @@ func (l *LocalConfigFile) Validate() error {
 		return err
 	}
 
-	// Set reasonable defaults for non-supported values
+	// Validate OIDC configuration overrides
+	for _, fc := range l.Config.Chains {
+		hasOidc := false
+		for _, f := range fc.Filters {
+			if l.Config.DefaultOidcConfig != nil && f.GetOidc() != nil {
+				return fmt.Errorf("%w: in chain %q OIDC filter and default OIDC configuration cannot be used together",
+					ErrDuplicateOIDCConfig, fc.Name)
+			}
+			if l.Config.DefaultOidcConfig == nil && f.GetOidcOverride() != nil {
+				return fmt.Errorf("%w: in chain %q OIDC override filter requires a default OIDC configuration",
+					ErrInvalidOIDCOverride, fc.Name)
+			}
+			if f.GetOidc() != nil || f.GetOidcOverride() != nil {
+				if hasOidc {
+					return fmt.Errorf("%w: ionly one OIDC configuration is allowed in a chain", ErrMultipleOIDCConfig)
+				}
+				hasOidc = true
+			}
+		}
+	}
+
+	// Overrides for non-supported values
 	l.Config.Threads = 1
 
+	// Merge the OIDC overrides with the default OIDC configuration so that
+	// we can properly validate the settings and  all filters have only one
+	// location where the OIDC configuration is defined.
+	mergeOIDCConfigs(&l.Config)
+
+	// Now that all defaults are set and configurations are merged, validate all final settings
 	return l.Config.ValidateAll()
 }
 
+// mergeOIDCConfigs merges the OIDC overrides with the default OIDC configuration so that
+// all filters have only one location where the OIDC configuration is defined.
+func mergeOIDCConfigs(cfg *configv1.Config) {
+	for _, fc := range cfg.Chains {
+		for _, f := range fc.Filters {
+			// Merge the OIDC overrides and populate the normal OIDC field instead so that
+			// consumers of the config always have an up-to-date object
+			if f.GetOidcOverride() != nil {
+				oidc := proto.Clone(cfg.DefaultOidcConfig).(*oidcv1.OIDCConfig)
+				proto.Merge(oidc, f.GetOidcOverride())
+				f.Type = &configv1.Filter_Oidc{Oidc: oidc}
+			}
+		}
+	}
+	// Clear the default config as it has already been merged. This way there is only one
+	// location for the OIDC settings.
+	cfg.DefaultOidcConfig = nil
+}
+
 func ConfigToJSONString(c *configv1.Config) string {
 	b, _ := protojson.Marshal(c)
 	return string(b)

internal/config_test.go

@@ -15,6 +15,7 @@
 package internal
 
 import (
+	"fmt"
 	"os"
 	"testing"
 
@@ -25,6 +26,7 @@ import (
 
 	configv1 "github.com/tetrateio/authservice-go/config/gen/go/v1"
 	mockv1 "github.com/tetrateio/authservice-go/config/gen/go/v1/mock"
+	oidcv1 "github.com/tetrateio/authservice-go/config/gen/go/v1/oidc"
 )
 
 type errCheck struct {
@@ -44,7 +46,7 @@ func (e errCheck) Check(t *testing.T, err error) {
 	}
 }
 
-func TestLoadConfig(t *testing.T) {
+func TestValidateConfig(t *testing.T) {
 	tests := []struct {
 		name  string
 		path  string
@@ -54,6 +56,9 @@ func TestLoadConfig(t *testing.T) {
 		{"unexisting", "unexisting", errCheck{is: os.ErrNotExist}},
 		{"invalid-config", "testdata/invalid-config.json", errCheck{msg: `unknown field "foo"`}},
 		{"invalid-values", "testdata/invalid-values.json", errCheck{as: &configv1.ConfigMultiError{}}},
+		{"duplicate-oidc", "testdata/duplicate-oidc.json", errCheck{is: ErrDuplicateOIDCConfig}},
+		{"invalid-oidc-override", "testdata/invalid-oidc-override.json", errCheck{is: ErrInvalidOIDCOverride}},
+		{"multiple-oidc", "testdata/multiple-oidc.json", errCheck{is: ErrMultipleOIDCConfig}},
 		{"valid", "testdata/mock.json", errCheck{is: nil}},
 	}
 
@@ -96,6 +101,49 @@ func TestLoadMock(t *testing.T) {
 	require.True(t, proto.Equal(want, &cfg.Config))
 }
 
+func TestLoadOIDC(t *testing.T) {
+	want := &configv1.Config{
+		ListenAddress: "0.0.0.0",
+		ListenPort:    8080,
+		LogLevel:      "debug",
+		Threads:       1,
+		Chains: []*configv1.FilterChain{
+			{
+				Name: "oidc",
+				Filters: []*configv1.Filter{
+					{
+						Type: &configv1.Filter_Oidc{
+							Oidc: &oidcv1.OIDCConfig{
+								AuthorizationUri: "http://fake",
+								TokenUri:         "http://fake",
+								CallbackUri:      "http://fake",
+								JwksConfig:       &oidcv1.OIDCConfig_Jwks{Jwks: "fake-jwks"},
+								ClientId:         "fake-client-id",
+								ClientSecret:     "fake-client-secret",
+								CookieNamePrefix: "",
+								IdToken:          &oidcv1.TokenConfig{Preamble: "Bearer", Header: "authorization"},
+								ProxyUri:         "http://fake",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range []string{"oidc", "oidc-override"} {
+		t.Run(tc, func(t *testing.T) {
+			var cfg LocalConfigFile
+			g := run.Group{Logger: telemetry.NoopLogger()}
+			g.Register(&cfg)
+			err := g.Run("", "--config-path", fmt.Sprintf("testdata/%s.json", tc))
+
+			require.NoError(t, err)
+			require.True(t, proto.Equal(want, &cfg.Config))
+		})
+	}
+}
+
 func TestConfigToJSONString(t *testing.T) {
 	tests := []struct {
 		name   string
@@ -113,5 +161,4 @@ func TestConfigToJSONString(t *testing.T) {
 			require.JSONEq(t, tt.want, got)
 		})
 	}
-
 }

internal/oidc/jwks.go

@@ -39,6 +39,9 @@ var (
 	_ run.Service = (*DefaultJWKSProvider)(nil)
 )
 
+// DefaultFetchInterval is the default interval to use when none is set.
+const DefaultFetchInterval = 1200 * time.Second
+
 // JWKSProvider provides a JWKS set for a given OIDC configuration.
 type JWKSProvider interface {
 	// Get the JWKS for the given OIDC configuration
@@ -105,6 +108,9 @@ func (j *DefaultJWKSProvider) fetchDynamic(ctx context.Context, config *oidcv1.O
 		transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: config.SkipVerifyPeerCert}
 		client := &http.Client{Transport: transport}
 		refreshInterval := time.Duration(config.PeriodicFetchIntervalSec) * time.Second
+		if refreshInterval == 0 {
+			refreshInterval = DefaultFetchInterval
+		}
 
 		j.cache.Configure(config.JwksUri,
 			jwk.WithHTTPClient(client),

internal/server/authz.go

@@ -110,19 +110,16 @@ func (e *ExtAuthZFilter) Check(ctx context.Context, req *envoy.CheckRequest) (re
 
 			log.Debug("applying filter", "type", fmt.Sprintf("%T", f.Type), "index", i)
 
+			// Note that the  Default_Oidc or the Oidc_Override types can't reach this point. The configurations have
+			// already been merged when loaded from the configuration file and populated accordingly in the Oidc settings.
 			switch ft := f.Type.(type) {
 			case *configv1.Filter_Mock:
 				h = authz.NewMockHandler(ft.Mock)
 			case *configv1.Filter_Oidc:
 				// TODO(nacx): Check if the Oidc setting is enough or we have to pull the default Oidc settings
-				h, err = authz.NewOIDCHandler(ft.Oidc, e.jwks)
-			case *configv1.Filter_OidcOverride:
-				// TODO(nacx): Check if the OidcOverride is enough or we have to pull the default Oidc settings
-				h, err = authz.NewOIDCHandler(ft.OidcOverride, e.jwks)
-			}
-
-			if err != nil {
-				return nil, err
+				if h, err = authz.NewOIDCHandler(ft.Oidc, e.jwks); err != nil {
+					return nil, err
+				}
 			}
 
 			if err = h.Process(ctx, req, resp); err != nil {

internal/testdata/duplicate-oidc.json

@@ -0,0 +1,40 @@
+{
+  "listen_address": "0.0.0.0",
+  "listen_port": 8080,
+  "log_level": "debug",
+  "default_oidc_config": {
+    "authorization_uri": "http://fake",
+    "token_uri": "http://fake",
+    "callback_uri": "http://fake",
+    "proxy_uri": "http://fake",
+    "jwks": "fake-jwks",
+    "client_id": "fake-client-id",
+    "client_secret": "fake-client-secret",
+    "id_token": {
+      "preamble": "Bearer",
+      "header": "authorization"
+    }
+  },
+  "chains": [
+    {
+      "name": "oidc",
+      "filters": [
+        {
+          "oidc": {
+            "authorization_uri": "http://fake",
+            "token_uri": "http://fake",
+            "callback_uri": "http://fake",
+            "proxy_uri": "http://fake",
+            "jwks": "fake-jwks",
+            "client_id": "fake-client-id",
+            "client_secret": "fake-client-secret",
+            "id_token": {
+              "preamble": "Bearer",
+              "header": "authorization"
+            }
+          }
+        }
+      ]
+    }
+  ]
+}

internal/testdata/invalid-oidc-override.json

@@ -0,0 +1,27 @@
+{
+  "listen_address": "0.0.0.0",
+  "listen_port": 8080,
+  "log_level": "debug",
+  "chains": [
+    {
+      "name": "oidc",
+      "filters": [
+        {
+          "oidc_override": {
+            "authorization_uri": "http://fake",
+            "token_uri": "http://fake",
+            "callback_uri": "http://fake",
+            "proxy_uri": "http://fake",
+            "jwks": "fake-jwks",
+            "client_id": "fake-client-id",
+            "client_secret": "fake-client-secret",
+            "id_token": {
+              "preamble": "Bearer",
+              "header": "authorization"
+            }
+          }
+        }
+      ]
+    }
+  ]
+}

internal/testdata/multiple-oidc.json

@@ -0,0 +1,42 @@
+{
+  "listen_address": "0.0.0.0",
+  "listen_port": 8080,
+  "log_level": "debug",
+  "chains": [
+    {
+      "name": "oidc",
+      "filters": [
+        {
+          "oidc": {
+            "authorization_uri": "http://fake",
+            "token_uri": "http://fake",
+            "callback_uri": "http://fake",
+            "proxy_uri": "http://fake",
+            "jwks": "fake-jwks",
+            "client_id": "fake-client-id",
+            "client_secret": "fake-client-secret",
+            "id_token": {
+              "preamble": "Bearer",
+              "header": "authorization"
+            }
+          }
+        },
+        {
+          "oidc": {
+            "authorization_uri": "http://fake",
+            "token_uri": "http://fake",
+            "callback_uri": "http://fake",
+            "proxy_uri": "http://fake",
+            "jwks": "fake-jwks",
+            "client_id": "fake-client-id",
+            "client_secret": "fake-client-secret",
+            "id_token": {
+              "preamble": "Bearer",
+              "header": "authorization"
+            }
+          }
+        }
+      ]
+    }
+  ]
+}

internal/testdata/oidc-override.json

@@ -0,0 +1,31 @@
+{
+  "listen_address": "0.0.0.0",
+  "listen_port": 8080,
+  "log_level": "debug",
+  "default_oidc_config": {
+    "authorization_uri": "http://default",
+    "token_uri": "http://default",
+    "callback_uri": "http://fake",
+    "proxy_uri": "http://fake"
+  },
+  "chains": [
+    {
+      "name": "oidc",
+      "filters": [
+        {
+          "oidc_override": {
+            "authorization_uri": "http://fake",
+            "token_uri": "http://fake",
+            "jwks": "fake-jwks",
+            "client_id": "fake-client-id",
+            "client_secret": "fake-client-secret",
+            "id_token": {
+              "preamble": "Bearer",
+              "header": "authorization"
+            }
+          }
+        }
+      ]
+    }
+  ]
+}