diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ee774db..4318673b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,13 @@ - [#2615](https://github.com/oauth2-proxy/oauth2-proxy/pull/2615) feat(cookies): add option to set a limit on the number of per-request CSRF cookies oauth2-proxy sets (@bh-tt) - [#2605](https://github.com/oauth2-proxy/oauth2-proxy/pull/2605) fix: show login page on broken cookie (@Primexz) - [#2743](https://github.com/oauth2-proxy/oauth2-proxy/pull/2743) feat: allow use more possible google admin-sdk api scopes (@BobDu) +- [#2359](https://github.com/oauth2-proxy/oauth2-proxy/pull/2359) feat: add SourceHut (sr.ht) provider(@bitfehler) +- [#2524](https://github.com/oauth2-proxy/oauth2-proxy/pull/2524) fix: regex substitution for $ signs in upstream path handling before running envsubst (@dashkan / @tuunit) +- [#3104](https://github.com/oauth2-proxy/oauth2-proxy/pull/3104) feat(cookie): add feature support for cookie-secret-file (@sandy2008) +- [#3055](https://github.com/oauth2-proxy/oauth2-proxy/pull/3055) feat: support non-default authorization request response mode also for OIDC providers (@stieler-it) +- [#3138](https://github.com/oauth2-proxy/oauth2-proxy/pull/3138) feat: make google_groups argument optional when using google provider (@sourava01) +- [#3093](https://github.com/oauth2-proxy/oauth2-proxy/pull/3093) feat: differentiate between "no available key" and error for redis sessions (@nobletrout) + # V7.10.0 diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 6a8f52e5..7c216dfb 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -128,6 +128,7 @@ Provider specific options can be found on their respective subpages. | flag: `--cookie-refresh`
toml: `cookie_refresh` | duration | refresh the cookie after this duration; `0` to disable; not supported by all providers [^1] | | | flag: `--cookie-samesite`
toml: `cookie_samesite` | string | set SameSite cookie attribute (`"lax"`, `"strict"`, `"none"`, or `""`). | `""` | | flag: `--cookie-secret`
toml: `cookie_secret` | string | the seed string for secure cookies (optionally base64 encoded) | | +| flag: `--cookie-secret-file`
toml: `cookie_secret_file` | string | For defining a separate cookie secret file to read the encryption key from | | | flag: `--cookie-secure`
toml: `cookie_secure` | bool | set [secure (HTTPS only) cookie flag](https://owasp.org/www-community/controls/SecureFlag) | true | [^1]: The following providers support `--cookie-refresh`: ADFS, Azure, GitLab, Google, Keycloak and all other Identity Providers which support the full [OIDC specification](https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokens) diff --git a/docs/docs/configuration/providers/google.md b/docs/docs/configuration/providers/google.md index 26af87ab..ac2a7dfa 100644 --- a/docs/docs/configuration/providers/google.md +++ b/docs/docs/configuration/providers/google.md @@ -8,7 +8,7 @@ title: Google (default) | Flag | Toml Field | Type | Description | Default | | ---------------------------------------------- | -------------------------------------------- | ------ | ------------------------------------------------------------------------------------------------ | -------------------------------------------------- | | `--google-admin-email` | `google_admin_email` | string | the google admin to impersonate for api calls | | -| `--google-group` | `google_groups` | string | restrict logins to members of this google group (may be given multiple times). | | +| `--google-group` | `google_groups` | string | restrict logins to members of this google group (may be given multiple times). If not specified and service account or default credentials are configured, all user groups will be allowed. | | | `--google-service-account-json` | `google_service_account_json` | string | the path to the service account json credentials | | | `--google-use-application-default-credentials` | `google_use_application_default_credentials` | bool | use application default credentials instead of service account json (i.e. GKE Workload Identity) | | | `--google-target-principal` | `google_target_principal` | bool | the target principal to impersonate when using ADC | defaults to the service account configured for ADC | diff --git a/docs/docs/configuration/providers/sourcehut.md b/docs/docs/configuration/providers/sourcehut.md new file mode 100644 index 00000000..88d14622 --- /dev/null +++ b/docs/docs/configuration/providers/sourcehut.md @@ -0,0 +1,25 @@ +--- +id: sourcehut +title: SourceHut +--- + +1. Create a new OAuth client: https://meta.sr.ht/oauth2 +2. Under `Redirection URI` enter the correct URL, i.e. + `https://internal.yourcompany.com/oauth2/callback` + +To use the provider, start with `--provider=sourcehut`. + +If you are hosting your own SourceHut instance, make sure you set the following +to the appropriate URLs: + +```shell + --login-url="https:///oauth2/authorize" + --redeem-url="https:///oauth2/access-token" + --profile-url="https:///query" + --validate-url="https:///profile" +``` + +The default configuration allows everyone with an account to authenticate. +Restricting access is currently only supported by +[email](#email-authentication). + diff --git a/pkg/apis/options/cookie.go b/pkg/apis/options/cookie.go index 22b74a6c..3653b7d0 100644 --- a/pkg/apis/options/cookie.go +++ b/pkg/apis/options/cookie.go @@ -1,8 +1,11 @@ package options import ( + "errors" + "os" "time" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/spf13/pflag" ) @@ -10,6 +13,7 @@ import ( type Cookie struct { Name string `flag:"cookie-name" cfg:"cookie_name"` Secret string `flag:"cookie-secret" cfg:"cookie_secret"` + SecretFile string `flag:"cookie-secret-file" cfg:"cookie_secret_file"` Domains []string `flag:"cookie-domain" cfg:"cookie_domains"` Path string `flag:"cookie-path" cfg:"cookie_path"` Expire time.Duration `flag:"cookie-expire" cfg:"cookie_expire"` @@ -18,8 +22,8 @@ type Cookie struct { HTTPOnly bool `flag:"cookie-httponly" cfg:"cookie_httponly"` SameSite string `flag:"cookie-samesite" cfg:"cookie_samesite"` CSRFPerRequest bool `flag:"cookie-csrf-per-request" cfg:"cookie_csrf_per_request"` - CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` CSRFPerRequestLimit int `flag:"cookie-csrf-per-request-limit" cfg:"cookie_csrf_per_request_limit"` + CSRFExpire time.Duration `flag:"cookie-csrf-expire" cfg:"cookie_csrf_expire"` } func cookieFlagSet() *pflag.FlagSet { @@ -27,6 +31,7 @@ func cookieFlagSet() *pflag.FlagSet { flagSet.String("cookie-name", "_oauth2_proxy", "the name of the cookie that the oauth_proxy creates") flagSet.String("cookie-secret", "", "the seed string for secure cookies (optionally base64 encoded)") + flagSet.String("cookie-secret-file", "", "For defining a separate cookie secret file to read the encryption key from") flagSet.StringSlice("cookie-domain", []string{}, "Optional cookie domains to force cookies to (ie: `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match).") flagSet.String("cookie-path", "/", "an optional cookie path to force cookies to (ie: /poc/)*") flagSet.Duration("cookie-expire", time.Duration(168)*time.Hour, "expire timeframe for cookie") @@ -43,16 +48,33 @@ func cookieFlagSet() *pflag.FlagSet { // cookieDefaults creates a Cookie populating each field with its default value func cookieDefaults() Cookie { return Cookie{ - Name: "_oauth2_proxy", - Secret: "", - Domains: nil, - Path: "/", - Expire: time.Duration(168) * time.Hour, - Refresh: time.Duration(0), - Secure: true, - HTTPOnly: true, - SameSite: "", - CSRFPerRequest: false, - CSRFExpire: time.Duration(15) * time.Minute, + Name: "_oauth2_proxy", + Secret: "", + SecretFile: "", + Domains: nil, + Path: "/", + Expire: time.Duration(168) * time.Hour, + Refresh: time.Duration(0), + Secure: true, + HTTPOnly: true, + SameSite: "", + CSRFPerRequest: false, + CSRFPerRequestLimit: 0, + CSRFExpire: time.Duration(15) * time.Minute, } } + +// GetSecret returns the cookie secret, reading from file if SecretFile is set +func (c *Cookie) GetSecret() (secret string, err error) { + if c.Secret != "" || c.SecretFile == "" { + return c.Secret, nil + } + + fileSecret, err := os.ReadFile(c.SecretFile) + if err != nil { + logger.Errorf("error reading cookie secret file %s: %s", c.SecretFile, err) + return "", errors.New("could not read cookie secret file") + } + + return string(fileSecret), nil +} diff --git a/pkg/apis/options/cookie_test.go b/pkg/apis/options/cookie_test.go new file mode 100644 index 00000000..a1486fed --- /dev/null +++ b/pkg/apis/options/cookie_test.go @@ -0,0 +1,70 @@ +package options + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCookieGetSecret(t *testing.T) { + t.Run("returns secret when Secret is set", func(t *testing.T) { + c := &Cookie{ + Secret: "my-secret", + SecretFile: "", + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "my-secret", secret) + }) + + t.Run("returns secret when both Secret and SecretFile are set", func(t *testing.T) { + c := &Cookie{ + Secret: "my-secret", + SecretFile: "/some/file", + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "my-secret", secret) + }) + + t.Run("reads from file when only SecretFile is set", func(t *testing.T) { + // Create a temporary file + tmpfile, err := os.CreateTemp("", "cookie-secret-test") + assert.NoError(t, err) + defer os.Remove(tmpfile.Name()) + + _, err = tmpfile.Write([]byte("file-secret")) + assert.NoError(t, err) + tmpfile.Close() + + c := &Cookie{ + Secret: "", + SecretFile: tmpfile.Name(), + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "file-secret", secret) + }) + + t.Run("returns error when file does not exist", func(t *testing.T) { + c := &Cookie{ + Secret: "", + SecretFile: "/nonexistent/file", + } + secret, err := c.GetSecret() + assert.Error(t, err) + assert.Equal(t, "", secret) + assert.Contains(t, err.Error(), "could not read cookie secret file") + }) + + t.Run("returns empty when both Secret and SecretFile are empty", func(t *testing.T) { + c := &Cookie{ + Secret: "", + SecretFile: "", + } + secret, err := c.GetSecret() + assert.NoError(t, err) + assert.Equal(t, "", secret) + }) +} diff --git a/pkg/apis/options/load.go b/pkg/apis/options/load.go index b198c4ff..c302e8e7 100644 --- a/pkg/apis/options/load.go +++ b/pkg/apis/options/load.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "reflect" + "regexp" "strings" "github.com/a8m/envsubst" @@ -155,7 +156,8 @@ func LoadYAML(configFileName string, into interface{}) error { return nil } -// Performs the heavy lifting of the LoadYaml function +// loadAndParseYaml reads the config from the filesystem and +// execute the environment variable substitution func loadAndParseYaml(configFileName string) ([]byte, error) { if configFileName == "" { return nil, errors.New("no configuration file provided") @@ -166,12 +168,26 @@ func loadAndParseYaml(configFileName string) ([]byte, error) { return nil, fmt.Errorf("unable to load config file: %w", err) } - // We now parse over the yaml with env substring, and fill in the ENV's - buffer, err := envsubst.Bytes(unparsedBuffer) + modifiedBuffer, err := normalizeSubstitution(unparsedBuffer) + if err != nil { + return nil, fmt.Errorf("error normalizing substitution string : %w", err) + } + + buffer, err := envsubst.Bytes(modifiedBuffer) if err != nil { return nil, fmt.Errorf("error in substituting env variables : %w", err) } return buffer, nil - +} + +// normalizeSubstitution normalizes dollar signs ($) with numerals like +// $1 or $2 properly by correctly escaping them +func normalizeSubstitution(unparsedBuffer []byte) ([]byte, error) { + unparsedString := string(unparsedBuffer) + + regexPattern := regexp.MustCompile(`\$(\d+)`) + + substitutedString := regexPattern.ReplaceAllString(unparsedString, `$$$$1`) + return []byte(substitutedString), nil } diff --git a/pkg/apis/options/load_test.go b/pkg/apis/options/load_test.go index fefbc2e7..06123c37 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -487,6 +487,31 @@ sub: StringOption: "Bob", }, }), + Entry("with a config file containing $ signs for things other than environment variables", loadYAMLTableInput{ + configFile: []byte(` +stringOption: /$1 +stringSliceOption: +- /$1 +- ^/(.*)$ +- api/$1 +- api/(.*)$ +- ^/api/(.*)$ +- /api/$1`), + input: &TestOptions{}, + expectedOutput: &TestOptions{ + StringOption: "/$1", + TestOptionSubStruct: TestOptionSubStruct{ + StringSliceOption: []string{ + "/$1", + "^/(.*)$", + "api/$1", + "api/(.*)$", + "^/api/(.*)$", + "/api/$1", + }, + }, + }, + }), ) }) diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 212341c9..0f254575 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -150,6 +150,9 @@ const ( // OIDCProvider is the provider type for OIDC OIDCProvider ProviderType = "oidc" + + // SourceHutProvider is the provider type for SourceHut + SourceHutProvider ProviderType = "sourcehut" ) type KeycloakOptions struct { diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index eab87869..3b8efaf3 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -219,13 +219,22 @@ func (c *csrf) encodeCookie() (string, error) { return "", err } - return encryption.SignedValue(c.cookieOpts.Secret, c.cookieName(), encrypted, c.time.Now()) + secret, err := c.cookieOpts.GetSecret() + if err != nil { + return "", fmt.Errorf("error getting cookie secret: %v", err) + } + return encryption.SignedValue(secret, c.cookieName(), encrypted, c.time.Now()) } // decodeCSRFCookie validates the signature then decrypts and decodes a CSRF // cookie into a CSRF struct func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) { - val, t, ok := encryption.Validate(cookie, opts.Secret, opts.Expire) + secret, err := opts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + + val, t, ok := encryption.Validate(cookie, secret, opts.Expire) if !ok { return nil, errors.New("CSRF cookie failed validation") } @@ -235,15 +244,18 @@ func decodeCSRFCookie(cookie *http.Cookie, opts *options.Cookie) (*csrf, error) return nil, err } - // Valid cookie, Unmarshal the CSRF + return unmarshalCSRF(decrypted, opts, t) +} + +// unmarshalCSRF unmarshals decrypted data into a CSRF struct +func unmarshalCSRF(decrypted []byte, opts *options.Cookie, csrfTime time.Time) (*csrf, error) { clock := clock.Clock{} - clock.Set(t) + clock.Set(csrfTime) + csrf := &csrf{cookieOpts: opts, time: clock} - err = msgpack.Unmarshal(decrypted, csrf) - if err != nil { + if err := msgpack.Unmarshal(decrypted, csrf); err != nil { return nil, fmt.Errorf("error unmarshalling data to CSRF: %v", err) } - return csrf, nil } @@ -290,5 +302,9 @@ func decrypt(data []byte, opts *options.Cookie) ([]byte, error) { } func makeCipher(opts *options.Cookie) (encryption.Cipher, error) { - return encryption.NewCFBCipher(encryption.SecretBytes(opts.Secret)) + secret, err := opts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + return encryption.NewCFBCipher(encryption.SecretBytes(secret)) } diff --git a/pkg/sessions/cookie/session_store.go b/pkg/sessions/cookie/session_store.go index 3947177f..095bc0e7 100644 --- a/pkg/sessions/cookie/session_store.go +++ b/pkg/sessions/cookie/session_store.go @@ -54,16 +54,18 @@ func (s *SessionStore) Load(req *http.Request) (*sessions.SessionState, error) { // always http.ErrNoCookie return nil, err } - val, _, ok := encryption.Validate(c, s.Cookie.Secret, s.Cookie.Expire) + + secret, err := s.Cookie.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + + val, _, ok := encryption.Validate(c, secret, s.Cookie.Expire) if !ok { return nil, errors.New("cookie signature not valid") } - session, err := sessions.DecodeSessionState(val, s.CookieCipher, true) - if err != nil { - return nil, err - } - return session, nil + return sessions.DecodeSessionState(val, s.CookieCipher, true) } // Clear clears any saved session information by writing a cookie to @@ -121,7 +123,11 @@ func (s *SessionStore) makeSessionCookie(req *http.Request, value []byte, now ti strValue := string(value) if strValue != "" { var err error - strValue, err = encryption.SignedValue(s.Cookie.Secret, s.Cookie.Name, value, now) + secret, err := s.Cookie.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + strValue, err = encryption.SignedValue(secret, s.Cookie.Name, value, now) if err != nil { return nil, err } @@ -146,7 +152,11 @@ func (s *SessionStore) makeCookie(req *http.Request, name string, value string, // NewCookieSessionStore initialises a new instance of the SessionStore from // the configuration given func NewCookieSessionStore(opts *options.SessionOptions, cookieOpts *options.Cookie) (sessions.SessionStore, error) { - cipher, err := encryption.NewCFBCipher(encryption.SecretBytes(cookieOpts.Secret)) + secret, err := cookieOpts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + cipher, err := encryption.NewCFBCipher(encryption.SecretBytes(secret)) if err != nil { return nil, fmt.Errorf("error initialising cipher: %v", err) } diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index 581a7f45..7855db45 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -146,7 +146,11 @@ func decodeTicketFromRequest(req *http.Request, cookieOpts *options.Cookie) (*ti } // An existing cookie exists, try to retrieve the ticket - val, _, ok := encryption.Validate(requestCookie, cookieOpts.Secret, cookieOpts.Expire) + secret, err := cookieOpts.GetSecret() + if err != nil { + return nil, fmt.Errorf("error getting cookie secret: %v", err) + } + val, _, ok := encryption.Validate(requestCookie, secret, cookieOpts.Expire) if !ok { return nil, fmt.Errorf("session ticket cookie failed validation: %v", err) } diff --git a/pkg/sessions/redis/redis_store.go b/pkg/sessions/redis/redis_store.go index 18d79b80..4e846e9b 100644 --- a/pkg/sessions/redis/redis_store.go +++ b/pkg/sessions/redis/redis_store.go @@ -49,9 +49,12 @@ func (store *SessionStore) Save(ctx context.Context, key string, value []byte, e // cookie within the HTTP request object func (store *SessionStore) Load(ctx context.Context, key string) ([]byte, error) { value, err := store.Client.Get(ctx, key) - if err != nil { + if err == redis.Nil { + return nil, fmt.Errorf("session does not exist") + } else if err != nil { return nil, fmt.Errorf("error loading redis session: %v", err) } + return value, nil } diff --git a/pkg/validation/cookie.go b/pkg/validation/cookie.go index b515809d..5f2dd8ac 100644 --- a/pkg/validation/cookie.go +++ b/pkg/validation/cookie.go @@ -3,6 +3,7 @@ package validation import ( "fmt" "net/http" + "os" "sort" "time" @@ -11,7 +12,7 @@ import ( ) func validateCookie(o options.Cookie) []string { - msgs := validateCookieSecret(o.Secret) + msgs := validateCookieSecret(o.Secret, o.SecretFile) if o.Expire != time.Duration(0) && o.Refresh >= o.Expire { msgs = append(msgs, fmt.Sprintf( @@ -49,9 +50,27 @@ func validateCookieName(name string) []string { return msgs } -func validateCookieSecret(secret string) []string { - if secret == "" { - return []string{"missing setting: cookie-secret"} +func validateCookieSecret(secret string, secretFile string) []string { + if secret == "" && secretFile == "" { + return []string{"missing setting: cookie-secret or cookie-secret-file"} + } + if secret == "" && secretFile != "" { + fileData, err := os.ReadFile(secretFile) + if err != nil { + return []string{"could not read cookie secret file: " + secretFile} + } + // Validate the file content as a secret + secretBytes := encryption.SecretBytes(string(fileData)) + switch len(secretBytes) { + case 16, 24, 32: + // Valid secret size found + return []string{} + } + // Invalid secret size found, return a message + return []string{fmt.Sprintf( + "cookie_secret from file must be 16, 24, or 32 bytes to create an AES cipher, but is %d bytes", + len(secretBytes)), + } } secretBytes := encryption.SecretBytes(secret) diff --git a/pkg/validation/cookie_test.go b/pkg/validation/cookie_test.go index 1f0dc5cd..d11134da 100644 --- a/pkg/validation/cookie_test.go +++ b/pkg/validation/cookie_test.go @@ -1,6 +1,7 @@ package validation import ( + "os" "strings" "testing" "time" @@ -29,9 +30,23 @@ func TestValidateCookie(t *testing.T) { "a.cba.localhost", } + // Create a temporary file for the valid secret file test + tmpfile, err := os.CreateTemp("", "cookie-secret-test") + if err != nil { + t.Fatalf("Failed to create temporary file: %v", err) + } + defer os.Remove(tmpfile.Name()) + + // Write a valid 32-byte secret to the file + _, err = tmpfile.Write([]byte(validSecret)) + if err != nil { + t.Fatalf("Failed to write to temporary file: %v", err) + } + tmpfile.Close() + invalidNameMsg := "invalid cookie name: \"_oauth2;proxy\"" longNameMsg := "cookie name should be under 256 characters: cookie name is 260 characters" - missingSecretMsg := "missing setting: cookie-secret" + missingSecretMsg := "missing setting: cookie-secret or cookie-secret-file" invalidSecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 6 bytes" invalidBase64SecretMsg := "cookie_secret must be 16, 24, or 32 bytes to create an AES cipher, but is 10 bytes" refreshLongerThanExpireMsg := "cookie_refresh (\"1h0m0s\") must be less than cookie_expire (\"15m0s\")" @@ -271,6 +286,38 @@ func TestValidateCookie(t *testing.T) { }, errStrings: []string{}, }, + { + name: "with valid secret file", + cookie: options.Cookie{ + Name: validName, + Secret: "", + SecretFile: tmpfile.Name(), + Domains: domains, + Path: "", + Expire: 24 * time.Hour, + Refresh: 0, + Secure: true, + HTTPOnly: true, + SameSite: "", + }, + errStrings: []string{}, + }, + { + name: "with nonexistent secret file", + cookie: options.Cookie{ + Name: validName, + Secret: "", + SecretFile: "/nonexistent/file.txt", + Domains: domains, + Path: "", + Expire: 24 * time.Hour, + Refresh: 0, + Secure: true, + HTTPOnly: true, + SameSite: "", + }, + errStrings: []string{"could not read cookie secret file: /nonexistent/file.txt"}, + }, } for _, tc := range testCases { diff --git a/pkg/validation/options_test.go b/pkg/validation/options_test.go index 2d5e9560..5c283545 100644 --- a/pkg/validation/options_test.go +++ b/pkg/validation/options_test.go @@ -48,25 +48,45 @@ func TestNewOptions(t *testing.T) { assert.NotEqual(t, nil, err) expected := errorMsg([]string{ - "missing setting: cookie-secret", + "missing setting: cookie-secret or cookie-secret-file", "provider has empty id: ids are required for all providers", "provider missing setting: client-id", "missing setting: client-secret or client-secret-file"}) assert.Equal(t, expected, err.Error()) } -func TestGoogleGroupOptions(t *testing.T) { +func TestGoogleGroupOptionsWithoutServiceAccountJSON(t *testing.T) { o := testOptions() - o.Providers[0].GoogleConfig.Groups = []string{"googlegroup"} + o.Providers[0].GoogleConfig.AdminEmail = "admin@example.com" err := Validate(o) assert.NotEqual(t, nil, err) expected := errorMsg([]string{ - "missing setting: google-admin-email", "missing setting: google-service-account-json or google-use-application-default-credentials"}) assert.Equal(t, expected, err.Error()) } +func TestGoogleGroupOptionsWithoutAdminEmail(t *testing.T) { + o := testOptions() + o.Providers[0].GoogleConfig.UseApplicationDefaultCredentials = true + err := Validate(o) + assert.NotEqual(t, nil, err) + + expected := errorMsg([]string{ + "missing setting: google-admin-email"}) + assert.Equal(t, expected, err.Error()) +} + +func TestGoogleGroupOptionsWithoutGroups(t *testing.T) { + o := testOptions() + // Set admin email and application default credentials but no groups - should still require them + o.Providers[0].GoogleConfig.AdminEmail = "admin@example.com" + o.Providers[0].GoogleConfig.UseApplicationDefaultCredentials = true + err := Validate(o) + // Should pass validation since google-group is now optional + assert.Equal(t, nil, err) +} + func TestGoogleGroupInvalidFile(t *testing.T) { o := testOptions() o.Providers[0].GoogleConfig.Groups = []string{"test_group"} diff --git a/pkg/validation/providers.go b/pkg/validation/providers.go index b1106b35..4527b841 100644 --- a/pkg/validation/providers.go +++ b/pkg/validation/providers.go @@ -94,18 +94,14 @@ func validateClientSecret(provider options.Provider) []string { func validateGoogleConfig(provider options.Provider) []string { msgs := []string{} - hasGoogleGroups := len(provider.GoogleConfig.Groups) >= 1 hasAdminEmail := provider.GoogleConfig.AdminEmail != "" hasSAJSON := provider.GoogleConfig.ServiceAccountJSON != "" useADC := provider.GoogleConfig.UseApplicationDefaultCredentials - if !hasGoogleGroups && !hasAdminEmail && !hasSAJSON && !useADC { + if !hasAdminEmail && !hasSAJSON && !useADC { return msgs } - if !hasGoogleGroups { - msgs = append(msgs, "missing setting: google-group") - } if !hasAdminEmail { msgs = append(msgs, "missing setting: google-admin-email") } diff --git a/providers/google.go b/providers/google.go index 0e1e2156..097e3567 100644 --- a/providers/google.go +++ b/providers/google.go @@ -103,17 +103,24 @@ func NewGoogleProvider(p *ProviderData, opts options.GoogleOptions) (*GoogleProv } if opts.ServiceAccountJSON != "" || opts.UseApplicationDefaultCredentials { - // Backwards compatibility with `--google-group` option - if len(opts.Groups) > 0 { - provider.setAllowedGroups(opts.Groups) - } - - provider.setGroupRestriction(opts) + provider.configureGroups(opts) } return provider, nil } +func (p *GoogleProvider) configureGroups(opts options.GoogleOptions) { + adminService := getAdminService(opts) + // Backwards compatibility with `--google-group` option + if len(opts.Groups) > 0 { + p.setAllowedGroups(opts.Groups) + p.groupValidator = p.setGroupRestriction(opts.Groups, adminService) + return + } + + p.groupValidator = p.populateAllGroups(adminService) +} + func claimsFromIDToken(idToken string) (*claims, error) { // id_token is a base64 encode ID token payload @@ -209,18 +216,13 @@ func (p *GoogleProvider) EnrichSession(_ context.Context, s *sessions.SessionSta } // SetGroupRestriction configures the GoogleProvider to restrict access to the -// specified group(s). AdminEmail has to be an administrative email on the domain that is -// checked. CredentialsFile is the path to a json file containing a Google service -// account credentials. -// -// TODO (@NickMeves) - Unit Test this OR refactor away from groupValidator func -func (p *GoogleProvider) setGroupRestriction(opts options.GoogleOptions) { - adminService := getAdminService(opts) - p.groupValidator = func(s *sessions.SessionState) bool { +// specified group(s). +func (p *GoogleProvider) setGroupRestriction(groups []string, adminService *admin.Service) func(*sessions.SessionState) bool { + return func(s *sessions.SessionState) bool { // Reset our saved Groups in case membership changed // This is used by `Authorize` on every request - s.Groups = make([]string, 0, len(opts.Groups)) - for _, group := range opts.Groups { + s.Groups = make([]string, 0, len(groups)) + for _, group := range groups { if userInGroup(adminService, group, s.Email) { s.Groups = append(s.Groups, group) } @@ -229,6 +231,25 @@ func (p *GoogleProvider) setGroupRestriction(opts options.GoogleOptions) { } } +// populateAllGroups configures the GoogleProvider to allow access with all +// groups and populate session with all groups of the user when no specific +// groups are configured. +func (p *GoogleProvider) populateAllGroups(adminService *admin.Service) func(s *sessions.SessionState) bool { + return func(s *sessions.SessionState) bool { + // Get all groups of the user + groups, err := getUserGroups(adminService, s.Email) + if err != nil { + logger.Errorf("Failed to get user groups for %s: %v", s.Email, err) + s.Groups = []string{} + return true // Allow access even if we can't get groups + } + + // Populate session with all user groups + s.Groups = groups + return true // Always allow access when no specific groups are configured + } +} + // https://developers.google.com/admin-sdk/directory/reference/rest/v1/members/hasMember#authorization-scopes var possibleScopesList = [...]string{ admin.AdminDirectoryGroupMemberReadonlyScope, @@ -269,6 +290,10 @@ func getOauth2TokenSource(ctx context.Context, opts options.GoogleOptions, scope return conf.TokenSource(ctx) } +// getAdminService retrieves an oauth token for the admin api of Google +// AdminEmail has to be an administrative email on the domain that is +// checked. CredentialsFile is the path to a json file containing a Google service +// account credentials. func getAdminService(opts options.GoogleOptions) *admin.Service { ctx := context.Background() var client *http.Client @@ -339,6 +364,38 @@ func getTargetPrincipal(ctx context.Context, opts options.GoogleOptions) (target return targetPrincipal } +// getUserGroups retrieves all groups that a user is a member of using the Google Admin Directory API +func getUserGroups(service *admin.Service, email string) ([]string, error) { + var allGroups []string + var pageToken string + + for { + req := service.Groups.List().UserKey(email).MaxResults(200) + if pageToken != "" { + req = req.PageToken(pageToken) + } + + groupsResp, err := req.Do() + if err != nil { + return nil, fmt.Errorf("failed to list groups for user %s: %v", email, err) + } + + for _, group := range groupsResp.Groups { + if group.Email != "" { + allGroups = append(allGroups, group.Email) + } + } + + // Check if there are more pages + if groupsResp.NextPageToken == "" { + break + } + pageToken = groupsResp.NextPageToken + } + + return allGroups, nil +} + func userInGroup(service *admin.Service, group string, email string) bool { // Use the HasMember API to checking for the user's presence in each group or nested subgroups req := service.Members.HasMember(group, email) diff --git a/providers/google_test.go b/providers/google_test.go index 23bca7ea..dc061203 100644 --- a/providers/google_test.go +++ b/providers/google_test.go @@ -289,3 +289,39 @@ func TestGoogleProvider_userInGroup(t *testing.T) { result = userInGroup(service, "group@example.com", "non-member-out-of-domain@otherexample.com") assert.False(t, result) } + +func TestGoogleProvider_getUserGroups(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/admin/directory/v1/groups" && r.URL.Query().Get("userKey") == "test@example.com" { + response := `{ + "kind": "admin#directory#groups", + "groups": [ + { + "kind": "admin#directory#group", + "id": "1", + "email": "group1@example.com", + "name": "Group 1" + }, + { + "kind": "admin#directory#group", + "id": "2", + "email": "group2@example.com", + "name": "Group 2" + } + ] + }` + fmt.Fprintln(w, response) + } else { + http.NotFound(w, r) + } + })) + defer ts.Close() + + client := &http.Client{} + adminService, err := admin.NewService(context.Background(), option.WithHTTPClient(client), option.WithEndpoint(ts.URL)) + assert.NoError(t, err) + + groups, err := getUserGroups(adminService, "test@example.com") + assert.NoError(t, err) + assert.Equal(t, []string{"group1@example.com", "group2@example.com"}, groups) +} diff --git a/providers/oidc.go b/providers/oidc.go index 43b5227e..15598aba 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -61,6 +61,11 @@ func (p *OIDCProvider) GetLoginURL(redirectURI, state, nonce string, extraParams if !p.SkipNonce { extraParams.Add("nonce", nonce) } + // Response mode should only be set if a non default mode is requested + if p.AuthRequestResponseMode != "" { + extraParams.Add("response_mode", p.AuthRequestResponseMode) + } + loginURL := makeLoginURL(p.Data(), redirectURI, state, extraParams) return loginURL.String() } diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 6a49f8ff..81a70eb4 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -275,3 +275,32 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { }) } } + +func TestOIDCProviderResponseModeConfigured(t *testing.T) { + providerData := &ProviderData{ + LoginURL: &url.URL{ + Scheme: "http", + Host: "my.test.idp", + Path: "/oauth/authorize", + }, + AuthRequestResponseMode: "form_post", + } + p := NewOIDCProvider(providerData, options.OIDCOptions{}) + + result := p.GetLoginURL("https://my.test.app/oauth", "", "", url.Values{}) + assert.Contains(t, result, "response_mode=form_post") +} + +func TestOIDCProviderResponseModeNotConfigured(t *testing.T) { + providerData := &ProviderData{ + LoginURL: &url.URL{ + Scheme: "http", + Host: "my.test.idp", + Path: "/oauth/authorize", + }, + } + p := NewOIDCProvider(providerData, options.OIDCOptions{}) + + result := p.GetLoginURL("https://my.test.app/oauth", "", "", url.Values{}) + assert.NotContains(t, result, "response_mode") +} diff --git a/providers/providers.go b/providers/providers.go index c4f7c96c..ec00f412 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -69,6 +69,8 @@ func NewProvider(providerConfig options.Provider) (Provider, error) { return NewNextcloudProvider(providerData), nil case options.OIDCProvider: return NewOIDCProvider(providerData, providerConfig.OIDCConfig), nil + case options.SourceHutProvider: + return NewSourceHutProvider(providerData), nil default: return nil, fmt.Errorf("unknown provider type %q", providerConfig.Type) } @@ -185,7 +187,8 @@ func parseCodeChallengeMethod(providerConfig options.Provider) string { func providerRequiresOIDCProviderVerifier(providerType options.ProviderType) (bool, error) { switch providerType { case options.BitbucketProvider, options.DigitalOceanProvider, options.FacebookProvider, options.GitHubProvider, - options.GoogleProvider, options.KeycloakProvider, options.LinkedInProvider, options.LoginGovProvider, options.NextCloudProvider: + options.GoogleProvider, options.KeycloakProvider, options.LinkedInProvider, options.LoginGovProvider, + options.NextCloudProvider, options.SourceHutProvider: return false, nil case options.OIDCProvider, options.ADFSProvider, options.AzureProvider, options.CidaasProvider, options.GitLabProvider, options.KeycloakOIDCProvider, options.MicrosoftEntraIDProvider: diff --git a/providers/srht.go b/providers/srht.go new file mode 100644 index 00000000..aa72229c --- /dev/null +++ b/providers/srht.go @@ -0,0 +1,108 @@ +package providers + +import ( + "bytes" + "context" + "fmt" + "net/url" + + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" +) + +type SourceHutProvider struct { + *ProviderData +} + +var _ Provider = (*SourceHutProvider)(nil) + +const ( + SourceHutProviderName = "SourceHut" + SourceHutDefaultScope = "meta.sr.ht/PROFILE:RO" +) + +var ( + // Default Login URL for SourceHut. + // Pre-parsed URL of https://meta.sr.ht/oauth2/authorize. + SourceHutDefaultLoginURL = &url.URL{ + Scheme: "https", + Host: "meta.sr.ht", + Path: "/oauth2/authorize", + } + + // Default Redeem URL for SourceHut. + // Pre-parsed URL of https://meta.sr.ht/oauth2/access-token. + SourceHutDefaultRedeemURL = &url.URL{ + Scheme: "https", + Host: "meta.sr.ht", + Path: "/oauth2/access-token", + } + + // Default Profile URL for SourceHut. + // Pre-parsed URL of https://meta.sr.ht/query. + SourceHutDefaultProfileURL = &url.URL{ + Scheme: "https", + Host: "meta.sr.ht", + Path: "/query", + } + + // Default Validation URL for SourceHut. + // Pre-parsed URL of https://meta.sr.ht/profile. + SourceHutDefaultValidateURL = &url.URL{ + Scheme: "https", + Host: "meta.sr.ht", + Path: "/profile", + } +) + +// NewSourceHutProvider creates a SourceHutProvider using the passed ProviderData +func NewSourceHutProvider(p *ProviderData) *SourceHutProvider { + p.setProviderDefaults(providerDefaults{ + name: SourceHutProviderName, + loginURL: SourceHutDefaultLoginURL, + redeemURL: SourceHutDefaultRedeemURL, + profileURL: SourceHutDefaultProfileURL, + validateURL: SourceHutDefaultValidateURL, + scope: SourceHutDefaultScope, + }) + + return &SourceHutProvider{ProviderData: p} +} + +// EnrichSession uses the SourceHut userinfo endpoint to populate the session's +// email and username. +func (p *SourceHutProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { + json, err := requests.New(p.ProfileURL.String()). + WithContext(ctx). + WithMethod("POST"). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", "Bearer "+s.AccessToken). + WithBody(bytes.NewBufferString(`{"query": "{ me { username, email } }"}`)). + Do(). + UnmarshalSimpleJSON() + if err != nil { + logger.Errorf("failed making request %v", err) + return err + } + + email, err := json.GetPath("data", "me", "email").String() + if err != nil { + return fmt.Errorf("unable to extract email from userinfo endpoint: %v", err) + } + s.Email = email + + username, err := json.GetPath("data", "me", "username").String() + if err != nil { + return fmt.Errorf("unable to extract username from userinfo endpoint: %v", err) + } + s.PreferredUsername = username + s.User = username + + return nil +} + +// ValidateSession validates the AccessToken +func (p *SourceHutProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool { + return validateToken(ctx, p, s.AccessToken, makeOIDCHeader(s.AccessToken)) +} diff --git a/providers/srht_test.go b/providers/srht_test.go new file mode 100644 index 00000000..fd51bf7f --- /dev/null +++ b/providers/srht_test.go @@ -0,0 +1,77 @@ +package providers + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" +) + +func testSourceHutProvider(hostname string) *SourceHutProvider { + p := NewSourceHutProvider( + &ProviderData{ + ProviderName: "SourceHut", + LoginURL: &url.URL{}, + RedeemURL: &url.URL{}, + ProfileURL: &url.URL{}, + ValidateURL: &url.URL{}, + Scope: ""}, + ) + p.ProviderName = "SourceHut" + + if hostname != "" { + updateURL(p.Data().LoginURL, hostname) + updateURL(p.Data().RedeemURL, hostname) + updateURL(p.Data().ProfileURL, hostname) + updateURL(p.Data().ValidateURL, hostname) + } + return p +} + +func testSourceHutBackend(payloads map[string][]string) *httptest.Server { + return httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + index := 0 + payload, ok := payloads[r.URL.Path] + if !ok { + w.WriteHeader(404) + } else if payload[index] == "" { + w.WriteHeader(204) + } else { + w.WriteHeader(200) + w.Write([]byte(payload[index])) + } + })) +} + +func TestSourceHutProvider_ValidateSessionWithBaseUrl(t *testing.T) { + b := testSourceHutBackend(map[string][]string{}) + defer b.Close() + + bURL, _ := url.Parse(b.URL) + p := testSourceHutProvider(bURL.Host) + + session := CreateAuthorizedSession() + + valid := p.ValidateSession(context.Background(), session) + assert.False(t, valid) +} + +func TestSourceHutProvider_ValidateSessionWithUserEmails(t *testing.T) { + b := testSourceHutBackend(map[string][]string{ + "/query": {`{"data":{"me":{"username":"bitfehler","email":"ch@bitfehler.net"}}}`}, + "/profile": {`ok`}, + }) + defer b.Close() + + bURL, _ := url.Parse(b.URL) + p := testSourceHutProvider(bURL.Host) + + session := CreateAuthorizedSession() + + valid := p.ValidateSession(context.Background(), session) + assert.True(t, valid) +}