Issue: 2236 - adds an option to append CA certificates ()

* 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 <jan@larwig.com>

* 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 <jan@larwig.com>
Co-authored-by: Koen van Zuijlen <8818390+kvanzuijlen@users.noreply.github.com>
Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
This commit is contained in:
emsixteeen 2023-10-25 06:36:17 -04:00 committed by GitHub
parent 601477a52c
commit a5006fd606
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 82 additions and 27 deletions

View File

@ -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)

View File

@ -415,6 +415,7 @@ Provider holds all configuration for a single provider
| `provider` | _[ProviderType](#providertype)_ | Type is the OAuth provider<br/>must be set from the supported providers group,<br/>otherwise 'Google' is set as default |
| `name` | _string_ | Name is the providers display name<br/>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.<br/>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<br/>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 |

View File

@ -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) |

View File

@ -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,

View File

@ -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

View File

@ -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

View File

@ -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)
}

View File

@ -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{