From a5006fd60623fda8d80f12f18172bc2d9acf1a8d Mon Sep 17 00:00:00 2001 From: emsixteeen Date: Wed, 25 Oct 2023 06:36:17 -0400 Subject: [PATCH] Issue: 2236 - adds an option to append CA certificates (#2237) * adding append option for custom CA certs * updated test for changed GetCertPool signature, added testing to check functionality of empty and non-empty store * adding legacy options as well * update associated documentation * fixing code climate complaints - reduce number of return statements * Apply suggestions from code review Changes caFilesAppend (and variants) to useSystemTrustStore Co-authored-by: Jan Larwig * Apply suggestions from code review Fixes extra whitespaces and grammar. Co-authored-by: Koen van Zuijlen <8818390+kvanzuijlen@users.noreply.github.com> * fix indentation * update changelog --------- Co-authored-by: Jan Larwig Co-authored-by: Koen van Zuijlen <8818390+kvanzuijlen@users.noreply.github.com> Co-authored-by: Joel Speed --- CHANGELOG.md | 1 + docs/docs/configuration/alpha_config.md | 1 + docs/docs/configuration/overview.md | 1 + pkg/apis/options/legacy_options.go | 3 ++ pkg/apis/options/providers.go | 4 +- pkg/util/util.go | 33 ++++++++++++- pkg/util/util_test.go | 64 ++++++++++++++++--------- pkg/validation/options.go | 2 +- 8 files changed, 82 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c087c2e..bf6e7c96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.5.1 +- [#2237](https://github.com/oauth2-proxy/oauth2-proxy/pull/2237) adds an option to append CA certificates (@emsixteeen) - [#2128](https://github.com/oauth2-proxy/oauth2-proxy/pull/2128) Update dependencies (@vllvll) - [#2274](https://github.com/oauth2-proxy/oauth2-proxy/pull/2274) Upgrade golang.org/x/net to v0.17.0 (@pierluigilenoci) - [#2282](https://github.com/oauth2-proxy/oauth2-proxy/pull/2282) Fixed checking Google Groups membership using Google Application Credentials (@kvanzuijlen) diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index a72dc639..8141f477 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -415,6 +415,7 @@ Provider holds all configuration for a single provider | `provider` | _[ProviderType](#providertype)_ | Type is the OAuth provider
must be set from the supported providers group,
otherwise 'Google' is set as default | | `name` | _string_ | Name is the providers display name
if set, it will be shown to the users in the login page. | | `caFiles` | _[]string_ | CAFiles is a list of paths to CA certificates that should be used when connecting to the provider.
If not specified, the default Go trust sources are used instead | +| `useSystemTrustStore` | _bool_ | UseSystemTrustStore determines if your custom CA files and the system trust store are used
If set to true, your custom CA files and the system trust store are used otherwise only your custom CA files. | | `loginURL` | _string_ | LoginURL is the authentication endpoint | | `loginURLParameters` | _[[]LoginURLParameter](#loginurlparameter)_ | LoginURLParameters defines the parameters that can be passed from the start URL to the IdP login URL | | `redeemURL` | _string_ | RedeemURL is the token redemption endpoint | diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index a3629492..36848ca6 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -154,6 +154,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--prompt` | string | [OIDC prompt](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest); if present, `approval-prompt` is ignored | `""` | | `--provider` | string | OAuth provider | google | | `--provider-ca-file` | string \| list | Paths to CA certificates that should be used when connecting to the provider. If not specified, the default Go trust sources are used instead. | +| `--use-system-trust-store` | bool | Determines if `provider-ca-file` files and the system trust store are used. If set to true, your custom CA files and the system trust store are used otherwise only your custom CA files. | false | | `--provider-display-name` | string | Override the provider's name with the given string; used for the sign-in page | (depends on provider) | | `--ping-path` | string | the ping endpoint that can be used for basic health checks | `"/ping"` | | `--ping-user-agent` | string | a User-Agent that can be used for basic health checks | `""` (don't check user agent) | diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 99be34b9..1bb06104 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -507,6 +507,7 @@ type LegacyProvider struct { ProviderType string `flag:"provider" cfg:"provider"` ProviderName string `flag:"provider-display-name" cfg:"provider_display_name"` ProviderCAFiles []string `flag:"provider-ca-file" cfg:"provider_ca_files"` + UseSystemTrustStore bool `flag:"use-system-trust-store" cfg:"use_system_trust_store"` OIDCIssuerURL string `flag:"oidc-issuer-url" cfg:"oidc_issuer_url"` InsecureOIDCAllowUnverifiedEmail bool `flag:"insecure-oidc-allow-unverified-email" cfg:"insecure_oidc_allow_unverified_email"` InsecureOIDCSkipIssuerVerification bool `flag:"insecure-oidc-skip-issuer-verification" cfg:"insecure_oidc_skip_issuer_verification"` @@ -561,6 +562,7 @@ func legacyProviderFlagSet() *pflag.FlagSet { flagSet.String("provider", "google", "OAuth provider") flagSet.String("provider-display-name", "", "Provider display name") flagSet.StringSlice("provider-ca-file", []string{}, "One or more paths to CA certificates that should be used when connecting to the provider. If not specified, the default Go trust sources are used instead.") + flagSet.Bool("use-system-trust-store", false, "Determines if 'provider-ca-file' files and the system trust store are used. If set to true, your custom CA files and the system trust store are used otherwise only your custom CA files.") flagSet.String("oidc-issuer-url", "", "OpenID Connect issuer URL (ie: https://accounts.google.com)") flagSet.Bool("insecure-oidc-allow-unverified-email", false, "Don't fail if an email address in an id_token is not verified") flagSet.Bool("insecure-oidc-skip-issuer-verification", false, "Do not verify if issuer matches OIDC discovery URL") @@ -659,6 +661,7 @@ func (l *LegacyProvider) convert() (Providers, error) { ClientSecretFile: l.ClientSecretFile, Type: ProviderType(l.ProviderType), CAFiles: l.ProviderCAFiles, + UseSystemTrustStore: l.UseSystemTrustStore, LoginURL: l.LoginURL, RedeemURL: l.RedeemURL, ProfileURL: l.ProfileURL, diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 9599f239..8820f345 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -59,7 +59,9 @@ type Provider struct { // CAFiles is a list of paths to CA certificates that should be used when connecting to the provider. // If not specified, the default Go trust sources are used instead CAFiles []string `json:"caFiles,omitempty"` - + // UseSystemTrustStore determines if your custom CA files and the system trust store are used + // If set to true, your custom CA files and the system trust store are used otherwise only your custom CA files. + UseSystemTrustStore bool `json:"useSystemTrustStore,omitempty"` // LoginURL is the authentication endpoint LoginURL string `json:"loginURL,omitempty"` // LoginURLParameters defines the parameters that can be passed from the start URL to the IdP login URL diff --git a/pkg/util/util.go b/pkg/util/util.go index a4425d9b..0f3d70ad 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -14,11 +14,40 @@ import ( "time" ) -func GetCertPool(paths []string) (*x509.CertPool, error) { +func GetCertPool(paths []string, useSystemPool bool) (*x509.CertPool, error) { if len(paths) == 0 { return nil, fmt.Errorf("invalid empty list of Root CAs file paths") } - pool := x509.NewCertPool() + + var pool *x509.CertPool + if useSystemPool { + rootPool, err := getSystemCertPool() + if err != nil { + return nil, fmt.Errorf("unable to get SystemCertPool when append is true - #{err}") + } + pool = rootPool + } else { + pool = x509.NewCertPool() + } + + return loadCertsFromPaths(paths, pool) + +} + +func getSystemCertPool() (*x509.CertPool, error) { + rootPool, err := x509.SystemCertPool() + if err != nil { + return nil, err + } + + if rootPool == nil { + return nil, fmt.Errorf("SystemCertPool is empty") + } + + return rootPool, nil +} + +func loadCertsFromPaths(paths []string, pool *x509.CertPool) (*x509.CertPool, error) { for _, path := range paths { // Cert paths are a configurable option data, err := os.ReadFile(path) // #nosec G304 diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index b8eff502..167c3e59 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -190,7 +190,7 @@ func makeTestCertFile(t *testing.T, pem, dir string) *os.File { } func TestGetCertPool_NoRoots(t *testing.T) { - _, err := GetCertPool([]string(nil)) + _, err := GetCertPool([]string(nil), false) assert.Error(t, err, "invalid empty list of Root CAs file paths") } @@ -204,34 +204,52 @@ func TestGetCertPool(t *testing.T) { } }(tempDir) + rootPool, _ := x509.SystemCertPool() + cleanPool := x509.NewCertPool() + + tests := []struct { + appendCerts bool + pool *x509.CertPool + }{ + {false, cleanPool}, + {true, rootPool}, + } + certFile1 := makeTestCertFile(t, root1Cert, tempDir) certFile2 := makeTestCertFile(t, root2Cert, tempDir) - certPool, err := GetCertPool([]string{certFile1.Name(), certFile2.Name()}) - assert.NoError(t, err) + for _, tc := range tests { + // Append certs to "known" pool so we can compare them + assert.True(t, tc.pool.AppendCertsFromPEM([]byte(root1Cert))) + assert.True(t, tc.pool.AppendCertsFromPEM([]byte(root2Cert))) - cert1Block, _ := pem.Decode([]byte(cert1Cert)) - cert1, _ := x509.ParseCertificate(cert1Block.Bytes) - assert.Equal(t, cert1.Subject.String(), cert1CertSubj) + certPool, err := GetCertPool([]string{certFile1.Name(), certFile2.Name()}, tc.appendCerts) + assert.NoError(t, err) + assert.True(t, tc.pool.Equal(certPool)) - cert2Block, _ := pem.Decode([]byte(cert2Cert)) - cert2, _ := x509.ParseCertificate(cert2Block.Bytes) - assert.Equal(t, cert2.Subject.String(), cert2CertSubj) + cert1Block, _ := pem.Decode([]byte(cert1Cert)) + cert1, _ := x509.ParseCertificate(cert1Block.Bytes) + assert.Equal(t, cert1.Subject.String(), cert1CertSubj) - cert3Block, _ := pem.Decode([]byte(cert3Cert)) - cert3, _ := x509.ParseCertificate(cert3Block.Bytes) - assert.Equal(t, cert3.Subject.String(), cert3CertSubj) + cert2Block, _ := pem.Decode([]byte(cert2Cert)) + cert2, _ := x509.ParseCertificate(cert2Block.Bytes) + assert.Equal(t, cert2.Subject.String(), cert2CertSubj) - opts := x509.VerifyOptions{ - Roots: certPool, + cert3Block, _ := pem.Decode([]byte(cert3Cert)) + cert3, _ := x509.ParseCertificate(cert3Block.Bytes) + assert.Equal(t, cert3.Subject.String(), cert3CertSubj) + + opts := x509.VerifyOptions{ + Roots: certPool, + } + + // "cert1" and "cert2" should be valid because "root1" and "root2" are in the certPool + // "cert3" should not be valid because "root3" is not in the certPool + _, err1 := cert1.Verify(opts) + assert.NoError(t, err1) + _, err2 := cert2.Verify(opts) + assert.NoError(t, err2) + _, err3 := cert3.Verify(opts) + assert.Error(t, err3) } - - // "cert1" and "cert2" should be valid because "root1" and "root2" are in the certPool - // "cert3" should not be valid because "root3" is not in the certPool - _, err1 := cert1.Verify(opts) - assert.NoError(t, err1) - _, err2 := cert2.Verify(opts) - assert.NoError(t, err2) - _, err3 := cert3.Verify(opts) - assert.Error(t, err3) } diff --git a/pkg/validation/options.go b/pkg/validation/options.go index a3ce0518..8c804829 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -37,7 +37,7 @@ func Validate(o *options.Options) error { } http.DefaultClient = &http.Client{Transport: insecureTransport} } else if len(o.Providers[0].CAFiles) > 0 { - pool, err := util.GetCertPool(o.Providers[0].CAFiles) + pool, err := util.GetCertPool(o.Providers[0].CAFiles, o.Providers[0].UseSystemTrustStore) if err == nil { transport := http.DefaultTransport.(*http.Transport).Clone() transport.TLSClientConfig = &tls.Config{