Skip to content

Commit

Permalink
Fix bitly#635: Support specifying alternative provider TLS trust sour…
Browse files Browse the repository at this point in the history
…ce(s) (bitly#645)

* Fix bitly#635: Support specifying alternative provider TLS trust source(s)

* Update pkg/apis/options/options.go

Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>

* Update pkg/validation/options.go

Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>

* Address review comments

* upd CHANGELOG.md

* refactor test to assert textual subjects + add openssl gen cmd

Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
  • Loading branch information
k-wall and JoelSpeed authored Jul 3, 2020
1 parent 390d479 commit b0375e8
Show file tree
Hide file tree
Showing 7 changed files with 163 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

- [#542](https://github.com/oauth2-proxy/oauth2-proxy/pull/542) Move SessionStore tests to independent package (@JoelSpeed)
- [#577](https://github.com/oauth2-proxy/oauth2-proxy/pull/577) Move Cipher and Session Store initialisation out of Validation (@JoelSpeed)
- [#635](https://github.com/oauth2-proxy/oauth2-proxy/pull/635) Support specifying alternative provider TLS trust source(s) (@k-wall)

# v6.0.0

Expand Down
1 change: 1 addition & 0 deletions docs/configuration/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
| `--profile-url` | string | Profile access endpoint | |
| `--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. |
| `--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) |
Expand Down
34 changes: 18 additions & 16 deletions pkg/apis/options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,22 +85,23 @@ type Options struct {

// These options allow for other providers besides Google, with
// potential overrides.
ProviderType string `flag:"provider" cfg:"provider"`
ProviderName string `flag:"provider-display-name" cfg:"provider_display_name"`
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"`
SkipOIDCDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`
OIDCJwksURL string `flag:"oidc-jwks-url" cfg:"oidc_jwks_url"`
LoginURL string `flag:"login-url" cfg:"login_url"`
RedeemURL string `flag:"redeem-url" cfg:"redeem_url"`
ProfileURL string `flag:"profile-url" cfg:"profile_url"`
ProtectedResource string `flag:"resource" cfg:"resource"`
ValidateURL string `flag:"validate-url" cfg:"validate_url"`
Scope string `flag:"scope" cfg:"scope"`
Prompt string `flag:"prompt" cfg:"prompt"`
ApprovalPrompt string `flag:"approval-prompt" cfg:"approval_prompt"` // Deprecated by OIDC 1.0
UserIDClaim string `flag:"user-id-claim" cfg:"user_id_claim"`
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"`
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"`
SkipOIDCDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`
OIDCJwksURL string `flag:"oidc-jwks-url" cfg:"oidc_jwks_url"`
LoginURL string `flag:"login-url" cfg:"login_url"`
RedeemURL string `flag:"redeem-url" cfg:"redeem_url"`
ProfileURL string `flag:"profile-url" cfg:"profile_url"`
ProtectedResource string `flag:"resource" cfg:"resource"`
ValidateURL string `flag:"validate-url" cfg:"validate_url"`
Scope string `flag:"scope" cfg:"scope"`
Prompt string `flag:"prompt" cfg:"prompt"`
ApprovalPrompt string `flag:"approval-prompt" cfg:"approval_prompt"` // Deprecated by OIDC 1.0
UserIDClaim string `flag:"user-id-claim" cfg:"user_id_claim"`

SignatureKey string `flag:"signature-key" cfg:"signature_key"`
AcrValues string `flag:"acr-values" cfg:"acr_values"`
Expand Down Expand Up @@ -267,6 +268,7 @@ func NewFlagSet() *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.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")
Expand Down
24 changes: 24 additions & 0 deletions pkg/util/util.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package util

import (
"crypto/x509"
"fmt"
"io/ioutil"
)

func GetCertPool(paths []string) (*x509.CertPool, error) {
if len(paths) == 0 {
return nil, fmt.Errorf("invalid empty list of Root CAs file paths")
}
pool := x509.NewCertPool()
for _, path := range paths {
data, err := ioutil.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("certificate authority file (%s) could not be read - %s", path, err)
}
if !pool.AppendCertsFromPEM(data) {
return nil, fmt.Errorf("loading certificate authority (%s) failed", path)
}
}
return pool, nil
}
91 changes: 91 additions & 0 deletions pkg/util/util_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package util

import (
"crypto/x509/pkix"
"encoding/asn1"
"io/ioutil"
"os"
"testing"

"github.com/stretchr/testify/assert"
)

// Test certificate created with an OpenSSL command in the following form:
// openssl req -x509 -newkey rsa:4096 -keyout key-unused.pem -out cert.pem -nodes -subj "/CN=oauth-proxy test ca"

var (
testCA1Subj = "CN=oauth-proxy test ca"
testCA1 = `-----BEGIN CERTIFICATE-----
MIICuTCCAaGgAwIBAgIFAKuKEWowDQYJKoZIhvcNAQELBQAwHjEcMBoGA1UEAxMT
b2F1dGgtcHJveHkgdGVzdCBjYTAeFw0xNzEwMjQyMDExMzJaFw0xOTEwMjQyMDEx
MzJaMB4xHDAaBgNVBAMTE29hdXRoLXByb3h5IHRlc3QgY2EwggEiMA0GCSqGSIb3
DQEBAQUAA4IBDwAwggEKAoIBAQC5/kmgKNiECuxlj27yTWBWOMVvIB0AaRhQrMA7
3iSCk/SHhaTabUuXUGRwmCAewT/y9oX3rTdfnSPCn7praU/27lRFBgOGFrTzAZH6
voisF54I3ZxWZgHDJ/ig/KFwd0Y8OATj9/k9uAJSCe6aT7BouJPZVWNGF2dF5BOJ
EwFsJiN2s8HpF14DhxFOMMtlckdMHGxi3wj3E/hBCfGvGGU4Wezz48vEWWC1ajWM
qVq2vVWi1bcNft8FjWa5wTGpdlDQJM7yvKYJPwRkEjgIXtF1ra3JM3WTTFZO9Yhd
QXwO7IWRTdTaypKTNbTDKuWQZsm7xQM9sNcFkukGb3o+uBpLAgMBAAEwDQYJKoZI
hvcNAQELBQADggEBAHJNrUfHhN7VOUF60pG8sOEkx0ztjbtbYMj2N9Kb0oSya+re
Kmb2Z4JgyV7XHCZ03Jch6L7UBI3Y6/Lp1zdwU03LFayVUchLkvFonoXpRRP5UFYN
+36xP3ZL1qBYFphARsCk6/tl36czH4oF5gTlhWCRy3upNzn+INk467hnCKt5xuse
zhm+xQv/VN1poI0S/oCg9HLA9iKpoqGJByN32yoFr3QViLPqkmJ1v8EiH0Ns+1m3
pP5YlVqdRCVrxgT80PIMsvQhfcuIrbbeiRDEUdEX7FqebuGCEa2757MTdW7UYQiB
7kgECMnwAOlJME8aDKnmTBajaMy6xCSC87V7wps=
-----END CERTIFICATE-----
`
testCA2Subj = "CN=oauth-proxy second test ca"
testCA2 = `-----BEGIN CERTIFICATE-----
MIICxzCCAa+gAwIBAgIFAKuMKewwDQYJKoZIhvcNAQELBQAwJTEjMCEGA1UEAxMa
b2F1dGgtcHJveHkgc2Vjb25kIHRlc3QgY2EwHhcNMTcxMDI1MTYxMTQxWhcNMTkx
MDI1MTYxMTQxWjAlMSMwIQYDVQQDExpvYXV0aC1wcm94eSBzZWNvbmQgdGVzdCBj
YTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAKdTkEOJ+QpOHy0PqGDR
fu8NFyo7BJwAnI+P1G32UXMeecCwBgGJEyv6eHEFV6jH/U2K2H0hynaCFxRuIdTA
EeS4s4BAbKqFhQ62I9lF3HVuqRPOe5FYdUl80eQynME22fWQ6/sZdQds0sFqaJBz
R4KQQxVULT19Br/6zwQZZhC1NtzSwCqi4CoO2OM7ctUKRvtC87LNGWapz5I4eh0A
/q4XJaSObsBCAJD7OVMa1LM3sSINUnvvGoSBKTuJ8MRk/BQRAO/PwXxsa+2h+k+w
D6sLExrBgWzAAPQKRKF+nLYVhz9AKn4JBpZt9j4PvTKz1SDcJ5wVEzOfVmii7Ui3
EFcCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAiy58XvhOka3drXv2bwl95FwNtodj
L2MmIdF0pp01O0ryREcC1kogamdOS/UHQs4okuCjwinR/UgU+cFGCDYHfeENtUTw
Ox2OikYD7bXUpNzbQ4QyF0+cKwAgxD4ai5xSV/NUvMkL1aE8tLyxGm6VkhhyvxU1
U9kvLha6KBWOCNd2fBJxgg8RAxFV3vR+xLdEtXnBAeTURrHM19gwMtd16y6gUZTZ
Xbl3Ix0t2+sqi0hpEF/iVFdCp5TXiicSnZCtePzCfHePAEfbh5hS0bq8Lbb9DZ6d
+2jX3AVuYhQPuutxla+vNp2XRcMTbzwXyi/Ig4nHKmPLFXsEbv+4tSwxyQ==
-----END CERTIFICATE-----
`
)

func makeTestCertFile(t *testing.T, pem, dir string) *os.File {
file, err := ioutil.TempFile(dir, "test-certfile")
assert.NoError(t, err)
_, err = file.Write([]byte(pem))
assert.NoError(t, err)
return file
}

func TestGetCertPool_NoRoots(t *testing.T) {
_, err := GetCertPool([]string(nil))
assert.Error(t, err, "invalid empty list of Root CAs file paths")
}

func TestGetCertPool(t *testing.T) {
tempDir, err := ioutil.TempDir("", "certtest")
assert.NoError(t, err)
defer os.RemoveAll(tempDir)
certFile1 := makeTestCertFile(t, testCA1, tempDir)
certFile2 := makeTestCertFile(t, testCA2, tempDir)

certPool, err := GetCertPool([]string{certFile1.Name(), certFile2.Name()})
assert.NoError(t, err)

subj := certPool.Subjects()
got := make([]string, 0)
for i := range subj {
var subject pkix.RDNSequence
_, err := asn1.Unmarshal(subj[i], &subject)
assert.NoError(t, err)
got = append(got, subject.String())
}

expectedSubjects := []string{testCA1Subj, testCA2Subj}
assert.Equal(t, expectedSubjects, got)
}
17 changes: 15 additions & 2 deletions pkg/validation/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,34 @@ import (
"github.com/oauth2-proxy/oauth2-proxy/pkg/ip"
"github.com/oauth2-proxy/oauth2-proxy/pkg/logger"
"github.com/oauth2-proxy/oauth2-proxy/pkg/requests"
"github.com/oauth2-proxy/oauth2-proxy/pkg/util"
"github.com/oauth2-proxy/oauth2-proxy/providers"
)

// Validate checks that required options are set and validates those that they
// are of the correct format
func Validate(o *options.Options) error {
msgs := make([]string, 0)
if o.SSLInsecureSkipVerify {
// TODO: Accept a certificate bundle.
insecureTransport := &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}
http.DefaultClient = &http.Client{Transport: insecureTransport}
} else if len(o.ProviderCAFiles) > 0 {
pool, err := util.GetCertPool(o.ProviderCAFiles)
if err == nil {
transport := &http.Transport{
TLSClientConfig: &tls.Config{
RootCAs: pool,
},
}

http.DefaultClient = &http.Client{Transport: transport}
} else {
msgs = append(msgs, fmt.Sprintf("unable to load provider CA file(s): %v", err))
}
}

msgs := make([]string, 0)
if o.Cookie.Secret == "" {
msgs = append(msgs, "missing setting: cookie-secret")
} else {
Expand Down
13 changes: 13 additions & 0 deletions pkg/validation/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,3 +366,16 @@ func TestRealClientIPHeader(t *testing.T) {
assert.Equal(t, expected, err.Error())
assert.Nil(t, o.GetRealClientIPParser())
}

func TestProviderCAFilesError(t *testing.T) {
file, err := ioutil.TempFile("", "absent.*.crt")
assert.NoError(t, err)
assert.NoError(t, file.Close())
assert.NoError(t, os.Remove(file.Name()))

o := testOptions()
o.ProviderCAFiles = append(o.ProviderCAFiles, file.Name())
err = Validate(o)
assert.Error(t, err)
assert.Contains(t, err.Error(), "unable to load provider CA file(s)")
}

0 comments on commit b0375e8

Please sign in to comment.