From 7e6fc3192de73b7f9e1de84cca2f14db8d188b22 Mon Sep 17 00:00:00 2001 From: Ignasi Barrera Date: Tue, 13 Feb 2024 17:03:22 +0100 Subject: [PATCH 1/2] Apply config defaults and merge OIDC configuration --- internal/config.go | 56 ++++++++++++++++++-- internal/config_test.go | 50 ++++++++++++++++- internal/oidc/jwks.go | 6 +++ internal/server/authz.go | 13 ++--- internal/testdata/duplicate-oidc.json | 40 ++++++++++++++ internal/testdata/invalid-oidc-override.json | 27 ++++++++++ internal/testdata/oidc-override.json | 31 +++++++++++ internal/testdata/oidc.json | 27 ++++++++++ 8 files changed, 236 insertions(+), 14 deletions(-) create mode 100644 internal/testdata/duplicate-oidc.json create mode 100644 internal/testdata/invalid-oidc-override.json create mode 100644 internal/testdata/oidc-override.json create mode 100644 internal/testdata/oidc.json diff --git a/internal/config.go b/internal/config.go index fb9437d..df193d1 100644 --- a/internal/config.go +++ b/internal/config.go @@ -16,18 +16,27 @@ 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") +) // LocalConfigFile is a run.Config that loads the configuration file. type LocalConfigFile struct { @@ -61,12 +70,51 @@ func (l *LocalConfigFile) Validate() error { return err } - // Set reasonable defaults for non-supported values + // Validate OIDC configuration overrides + for _, fc := range l.Config.Chains { + 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) + } + } + } + + // 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) diff --git a/internal/config_test.go b/internal/config_test.go index 8d7dac5..d7794d3 100644 --- a/internal/config_test.go +++ 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,8 @@ 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}}, {"valid", "testdata/mock.json", errCheck{is: nil}}, } @@ -96,6 +100,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 +160,4 @@ func TestConfigToJSONString(t *testing.T) { require.JSONEq(t, tt.want, got) }) } - } diff --git a/internal/oidc/jwks.go b/internal/oidc/jwks.go index 1baea02..60696b7 100644 --- a/internal/oidc/jwks.go +++ b/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), diff --git a/internal/server/authz.go b/internal/server/authz.go index 88bd85f..cfd9741 100644 --- a/internal/server/authz.go +++ b/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 { diff --git a/internal/testdata/duplicate-oidc.json b/internal/testdata/duplicate-oidc.json new file mode 100644 index 0000000..47b6b3a --- /dev/null +++ b/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" + } + } + } + ] + } + ] +} diff --git a/internal/testdata/invalid-oidc-override.json b/internal/testdata/invalid-oidc-override.json new file mode 100644 index 0000000..afa39ee --- /dev/null +++ b/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" + } + } + } + ] + } + ] +} diff --git a/internal/testdata/oidc-override.json b/internal/testdata/oidc-override.json new file mode 100644 index 0000000..fec178c --- /dev/null +++ b/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" + } + } + } + ] + } + ] +} diff --git a/internal/testdata/oidc.json b/internal/testdata/oidc.json new file mode 100644 index 0000000..1f0c682 --- /dev/null +++ b/internal/testdata/oidc.json @@ -0,0 +1,27 @@ +{ + "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" + } + } + } + ] + } + ] +} From b34faf9b45422459d3a068d050926bdb5b5b7c3f Mon Sep 17 00:00:00 2001 From: Ignasi Barrera Date: Tue, 13 Feb 2024 17:10:21 +0100 Subject: [PATCH 2/2] validate only one OIDC filter per chain is set --- internal/config.go | 9 ++++++ internal/config_test.go | 1 + internal/testdata/multiple-oidc.json | 42 ++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 internal/testdata/multiple-oidc.json diff --git a/internal/config.go b/internal/config.go index df193d1..1133440 100644 --- a/internal/config.go +++ b/internal/config.go @@ -36,6 +36,8 @@ var ( 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. @@ -72,6 +74,7 @@ func (l *LocalConfigFile) Validate() error { // 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", @@ -81,6 +84,12 @@ func (l *LocalConfigFile) Validate() error { 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 + } } } diff --git a/internal/config_test.go b/internal/config_test.go index d7794d3..b0b27e9 100644 --- a/internal/config_test.go +++ b/internal/config_test.go @@ -58,6 +58,7 @@ func TestValidateConfig(t *testing.T) { {"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}}, } diff --git a/internal/testdata/multiple-oidc.json b/internal/testdata/multiple-oidc.json new file mode 100644 index 0000000..292d326 --- /dev/null +++ b/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" + } + } + } + ] + } + ] +}