From 59f46e6c9fed2ff96cc67ec8316d75324556d934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anderson=20Val=C3=A9rio?= Date: Fri, 29 Nov 2024 15:14:10 -0300 Subject: [PATCH 01/10] moving audit to pics folder --- oauthproxy.go | 13 +++++++------ pkg/{ => pics}/audit/audit_client.go | 4 ++-- pkg/{ => pics}/audit/audit_error.go | 0 pkg/{ => pics}/audit/audit_event.go | 1 + pkg/{ => pics}/audit/audit_metrics.go | 0 pkg/{ => pics}/audit/signature.go | 0 6 files changed, 10 insertions(+), 8 deletions(-) rename pkg/{ => pics}/audit/audit_client.go (98%) rename pkg/{ => pics}/audit/audit_error.go (100%) rename pkg/{ => pics}/audit/audit_event.go (99%) rename pkg/{ => pics}/audit/audit_metrics.go (100%) rename pkg/{ => pics}/audit/signature.go (100%) diff --git a/oauthproxy.go b/oauthproxy.go index 240dc818..00f472c1 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -25,11 +25,11 @@ import ( sessionsapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/pagewriter" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/app/redirect" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/audit" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/authentication/basic" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/cookies" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/encryption" proxyhttp "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/http" + picsaudit "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/pics/audit" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/ip" @@ -114,7 +114,8 @@ type OAuthProxy struct { appDirector redirect.AppDirector encodeState bool - AuditClient *audit.Client + + picsAuditClient *picsaudit.Client } // NewOAuthProxy creates a new instance of OAuthProxy from the options provided @@ -213,7 +214,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr Validator: redirectValidator, }) - auditClient, err := audit.NewAuditClient(&audit.ClientOpts{ + picsAuditClient, err := picsaudit.NewAuditClient(&picsaudit.ClientOpts{ URL: opts.AuditURL, Enabled: opts.EnableAudit, ProductName: opts.AuditProductName, @@ -256,7 +257,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr redirectValidator: redirectValidator, appDirector: appDirector, encodeState: opts.EncodeState, - AuditClient: auditClient, + picsAuditClient: picsAuditClient, } p.buildServeMux(opts.ProxyPrefix) @@ -925,7 +926,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { if !csrf.CheckOAuthState(nonce) { errorMsg := "Invalid authentication via OAuth2: CSRF token mismatch, potential attack" logger.PrintAuthf(session.Email, req, logger.AuthFailure, errorMsg) - p.AuditClient.CreateFailedLoginAuditEntry(session, appRedirect, req.Header.Get("edisp-org-id"), errorMsg) + p.picsAuditClient.CreateFailedLoginAuditEntry(session, appRedirect, req.Header.Get("edisp-org-id"), errorMsg) p.ErrorPage(rw, req, http.StatusForbidden, "CSRF token mismatch, potential attack", "Login Failed: Unable to find a valid CSRF token. Please try again.") return } @@ -954,7 +955,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) return } - p.AuditClient.CreateSuccessfulLoginAuditEntry(session, appRedirect, req.Header.Get("edisp-org-id")) + p.picsAuditClient.CreateSuccessfulLoginAuditEntry(session, appRedirect, req.Header.Get("edisp-org-id")) http.Redirect(rw, req, appRedirect, http.StatusFound) } else { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unauthorized") diff --git a/pkg/audit/audit_client.go b/pkg/pics/audit/audit_client.go similarity index 98% rename from pkg/audit/audit_client.go rename to pkg/pics/audit/audit_client.go index 2603dfde..46b7079c 100644 --- a/pkg/audit/audit_client.go +++ b/pkg/pics/audit/audit_client.go @@ -33,7 +33,7 @@ type Client struct { func NewAuditClient(opts *ClientOpts) (*Client, error) { if opts.Enabled { log.Print("Audit entries will be created since OAUTH2_PROXY_ENABLE_AUDIT is true") - err := opts.Validate() + err := opts.validate() if err != nil { return nil, err } @@ -154,7 +154,7 @@ func (c *Client) send(msg string) error { return nil } -func (c *ClientOpts) Validate() error { +func (c *ClientOpts) validate() error { err := errors.New("") if strings.TrimSpace(c.URL) == "" { err = errors.New("the OAUTH2_PROXY_AUDIT_URL must be set") diff --git a/pkg/audit/audit_error.go b/pkg/pics/audit/audit_error.go similarity index 100% rename from pkg/audit/audit_error.go rename to pkg/pics/audit/audit_error.go diff --git a/pkg/audit/audit_event.go b/pkg/pics/audit/audit_event.go similarity index 99% rename from pkg/audit/audit_event.go rename to pkg/pics/audit/audit_event.go index c0b9555e..6d1429d0 100644 --- a/pkg/audit/audit_event.go +++ b/pkg/pics/audit/audit_event.go @@ -81,6 +81,7 @@ type ExtensionContent struct { URL string `json:"url,omitempty"` ValueString string `json:"valueString,omitempty"` } + type Extension struct { URL string `json:"url,omitempty"` Extension []*ExtensionContent `json:"extension,omitempty"` diff --git a/pkg/audit/audit_metrics.go b/pkg/pics/audit/audit_metrics.go similarity index 100% rename from pkg/audit/audit_metrics.go rename to pkg/pics/audit/audit_metrics.go diff --git a/pkg/audit/signature.go b/pkg/pics/audit/signature.go similarity index 100% rename from pkg/audit/signature.go rename to pkg/pics/audit/signature.go From c053cb8d0a06e243a5c283c8f7a247513fbb4e78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anderson=20Val=C3=A9rio?= Date: Fri, 29 Nov 2024 15:14:58 -0300 Subject: [PATCH 02/10] isolating changes on oidc provider --- providers/oidc.go | 41 ++---------------------------------- providers/pics_oidc.go | 48 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 39 deletions(-) create mode 100644 providers/pics_oidc.go diff --git a/providers/oidc.go b/providers/oidc.go index 0b664964..dae8f089 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -1,19 +1,15 @@ package providers import ( - "bytes" "context" - b64 "encoding/base64" "errors" "fmt" - "net/http" "net/url" "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" "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" "golang.org/x/oauth2" ) @@ -98,7 +94,8 @@ func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code, codeVerifi // EnrichSession is called after Redeem to allow providers to enrich session fields // such as User, Email, Groups with provider specific API calls. func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { - err := p.enrichFromIntrospectURL(ctx, s) + + err := p.PicsEnrichFromIntrospectURL(ctx, s) if err != nil { logger.Errorf("Warning: Introspect URL request failed: %v", err) } @@ -130,40 +127,6 @@ func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionS return true } -// enrichFromIntrospectURL enriches a session's claims and permissions via the JSON response of -// an OIDC Introspection URL -func (p *OIDCProvider) enrichFromIntrospectURL(ctx context.Context, s *sessions.SessionState) error { - clientSecret, err := p.GetClientSecret() - if err != nil { - return err - } - params := url.Values{} - params.Add("token", s.AccessToken) - basicAuth := b64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", p.ClientID, clientSecret))) - if p.IntrospectURL == nil { - p.IntrospectURL = &url.URL{ - Scheme: p.RedeemURL.Scheme, - Host: p.RedeemURL.Host, - Path: "/authorize/oauth2/v4/introspect", - } - } - logger.Printf("Requesting introspect from '%s'", p.IntrospectURL) - - result := requests.New(p.IntrospectURL.String()). - WithContext(ctx). - WithMethod("POST"). - WithBody(bytes.NewBufferString(params.Encode())). - SetHeader("Authorization", fmt.Sprintf("Basic %s", basicAuth)). - SetHeader("Content-Type", "application/x-www-form-urlencoded"). - Do() - - if result.StatusCode() != http.StatusOK { - return fmt.Errorf("error while requesting introspect claims, status code - %d", result.StatusCode()) - } - s.IntrospectClaims = b64.StdEncoding.EncodeToString(result.Body()) - return nil -} - // RefreshSession uses the RefreshToken to fetch new Access and ID Tokens func (p *OIDCProvider) RefreshSession(ctx context.Context, s *sessions.SessionState) (bool, error) { if s == nil || s.RefreshToken == "" { diff --git a/providers/pics_oidc.go b/providers/pics_oidc.go new file mode 100644 index 00000000..6ca2c7aa --- /dev/null +++ b/providers/pics_oidc.go @@ -0,0 +1,48 @@ +package providers + +import ( + "bytes" + "context" + b64 "encoding/base64" + "fmt" + "net/http" + "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" +) + +// enrichFromIntrospectURL enriches a session's claims and permissions via the JSON response of +// an OIDC Introspection URL +func (p *OIDCProvider) PicsEnrichFromIntrospectURL(ctx context.Context, s *sessions.SessionState) error { + clientSecret, err := p.GetClientSecret() + if err != nil { + return err + } + params := url.Values{} + params.Add("token", s.AccessToken) + basicAuth := b64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", p.ClientID, clientSecret))) + if p.IntrospectURL == nil { + p.IntrospectURL = &url.URL{ + Scheme: p.RedeemURL.Scheme, + Host: p.RedeemURL.Host, + Path: "/authorize/oauth2/v4/introspect", + } + } + logger.Printf("Requesting introspect from '%s'", p.IntrospectURL) + + result := requests.New(p.IntrospectURL.String()). + WithContext(ctx). + WithMethod("POST"). + WithBody(bytes.NewBufferString(params.Encode())). + SetHeader("Authorization", fmt.Sprintf("Basic %s", basicAuth)). + SetHeader("Content-Type", "application/x-www-form-urlencoded"). + Do() + + if result.StatusCode() != http.StatusOK { + return fmt.Errorf("error while requesting introspect claims, status code - %d", result.StatusCode()) + } + s.IntrospectClaims = b64.StdEncoding.EncodeToString(result.Body()) + return nil +} From f0c2b53784f481b672fe90be66e2dd008349e9ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anderson=20Val=C3=A9rio?= Date: Fri, 29 Nov 2024 15:15:55 -0300 Subject: [PATCH 03/10] reverting changes that only cause conflicts --- pkg/apis/options/load_test.go | 1 + pkg/apis/options/providers.go | 1 + pkg/cookies/cookies.go | 4 ++-- pkg/cookies/csrf_test.go | 2 +- pkg/encryption/utils.go | 3 ++- pkg/encryption/utils_test.go | 34 +++++++++++++++--------------- pkg/sessions/persistence/ticket.go | 1 - pkg/util/util.go | 1 + pkg/util/util_test.go | 1 + pkg/validation/options.go | 1 - pkg/validation/providers.go | 1 + providers/providers.go | 2 ++ providers/providers_test.go | 2 +- 13 files changed, 30 insertions(+), 24 deletions(-) diff --git a/pkg/apis/options/load_test.go b/pkg/apis/options/load_test.go index 53954d60..f82395cc 100644 --- a/pkg/apis/options/load_test.go +++ b/pkg/apis/options/load_test.go @@ -414,6 +414,7 @@ sub: } else { input = &TestOptions{} } + err := LoadYAML(configFileName, input) if in.expectedErr != nil { Expect(err).To(MatchError(in.expectedErr.Error())) diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 8480dc35..a90b584c 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -85,6 +85,7 @@ type Provider struct { AllowedGroups []string `json:"allowedGroups,omitempty"` // The code challenge method CodeChallengeMethod string `json:"code_challenge_method,omitempty"` + // URL to call to perform backend logout, `{id_token}` would be replaced by the actual `id_token` if available in the session BackendLogoutURL string `json:"backendLogoutURL"` } diff --git a/pkg/cookies/cookies.go b/pkg/cookies/cookies.go index 858f444b..1e017366 100644 --- a/pkg/cookies/cookies.go +++ b/pkg/cookies/cookies.go @@ -35,12 +35,12 @@ func MakeCookieFromOptions(req *http.Request, name string, value string, opts *o SameSite: ParseSameSite(opts.SameSite), } - warnInvalidDomain(c, req) - if expiration != time.Duration(0) { c.Expires = now.Add(expiration) } + warnInvalidDomain(c, req) + return c } diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index cf60535c..97c1b496 100644 --- a/pkg/cookies/csrf_test.go +++ b/pkg/cookies/csrf_test.go @@ -28,10 +28,10 @@ var _ = Describe("CSRF Cookie Tests", func() { Domains: []string{cookieDomain}, Path: cookiePath, Expire: time.Hour, - CSRFExpire: time.Hour, Secure: true, HTTPOnly: true, CSRFPerRequest: false, + CSRFExpire: time.Hour, } var err error diff --git a/pkg/encryption/utils.go b/pkg/encryption/utils.go index fcc8f6fe..426a3131 100644 --- a/pkg/encryption/utils.go +++ b/pkg/encryption/utils.go @@ -58,7 +58,8 @@ func Validate(cookie *http.Cookie, seed string, expiration time.Duration) (value // creation timestamp stored in the cookie falls within the // window defined by (Now()-expiration, Now()]. t = time.Unix(int64(ts), 0) - if (expiration == time.Duration(0)) || t.After(time.Now().Add(expiration*-1)) && t.Before(time.Now().Add(time.Minute*5)) { // it's a valid cookie. now get the contents + if (expiration == time.Duration(0)) || (t.After(time.Now().Add(expiration*-1)) && t.Before(time.Now().Add(time.Minute*5))) { + // it's a valid cookie. now get the contents rawValue, err := base64.URLEncoding.DecodeString(parts[0]) if err == nil { value = rawValue diff --git a/pkg/encryption/utils_test.go b/pkg/encryption/utils_test.go index f13fbeab..9e69df84 100644 --- a/pkg/encryption/utils_test.go +++ b/pkg/encryption/utils_test.go @@ -106,23 +106,6 @@ func TestSignAndValidate(t *testing.T) { assert.False(t, checkSignature(sha1sig, seed, key, "tampered", epoch)) } -func TestGenerateRandomASCIIString(t *testing.T) { - randomString, err := GenerateRandomASCIIString(96) - assert.NoError(t, err) - - // Only 8-bit characters - assert.Equal(t, 96, len([]byte(randomString))) - - // All non-ascii characters removed should still be the original string - removedChars := strings.Map(func(r rune) rune { - if r > unicode.MaxASCII { - return -1 - } - return r - }, randomString) - assert.Equal(t, removedChars, randomString) -} - func TestValidate(t *testing.T) { seed := "0123456789abcdef" key := "cookie-name" @@ -146,3 +129,20 @@ func TestValidate(t *testing.T) { assert.NoError(t, err) assert.Equal(t, validValue, expectedValue) } + +func TestGenerateRandomASCIIString(t *testing.T) { + randomString, err := GenerateRandomASCIIString(96) + assert.NoError(t, err) + + // Only 8-bit characters + assert.Equal(t, 96, len([]byte(randomString))) + + // All non-ascii characters removed should still be the original string + removedChars := strings.Map(func(r rune) rune { + if r > unicode.MaxASCII { + return -1 + } + return r + }, randomString) + assert.Equal(t, removedChars, randomString) +} diff --git a/pkg/sessions/persistence/ticket.go b/pkg/sessions/persistence/ticket.go index b8f1559a..5020ada9 100644 --- a/pkg/sessions/persistence/ticket.go +++ b/pkg/sessions/persistence/ticket.go @@ -129,7 +129,6 @@ func decodeTicket(encTicket string, cookieOpts *options.Cookie) (*ticket, error) if errSecret != nil { return nil, fmt.Errorf("failed to decode ticket: %v", errSecret) } - return &ticket{ id: ticketID, secret: secret, diff --git a/pkg/util/util.go b/pkg/util/util.go index 6986121f..0f3d70ad 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -18,6 +18,7 @@ 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") } + var pool *x509.CertPool if useSystemPool { rootPool, err := getSystemCertPool() diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index 817d46f4..cdb40668 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -217,6 +217,7 @@ func TestGetCertPool(t *testing.T) { certFile1 := makeTestCertFile(t, root1Cert, tempDir) certFile2 := makeTestCertFile(t, root2Cert, tempDir) + for _, tc := range tests { // Append certs to "known" pool so we can compare them assert.True(t, tc.pool.AppendCertsFromPEM([]byte(root1Cert))) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 52737c32..8c804829 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -77,7 +77,6 @@ func Validate(o *options.Options) error { var redirectURL *url.URL redirectURL, msgs = parseURL(o.RawRedirectURL, "redirect", msgs) - o.SetRedirectURL(redirectURL) if o.RawRedirectURL == "" && !o.Cookie.Secure && !o.ReverseProxy { logger.Print("WARNING: no explicit redirect URL: redirects will default to insecure HTTP") diff --git a/pkg/validation/providers.go b/pkg/validation/providers.go index 44a2009f..ecc2d06d 100644 --- a/pkg/validation/providers.go +++ b/pkg/validation/providers.go @@ -75,6 +75,7 @@ func validateGoogleConfig(provider options.Provider) []string { if !hasGoogleGroups && !hasAdminEmail && !hasSAJSON && !useADC { return msgs } + if !hasGoogleGroups { msgs = append(msgs, "missing setting: google-group") } diff --git a/providers/providers.go b/providers/providers.go index ec7a0db9..ce5f93ef 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -160,7 +160,9 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData, } p.setAllowedGroups(providerConfig.AllowedGroups) + p.BackendLogoutURL = providerConfig.BackendLogoutURL + return p, nil } diff --git a/providers/providers_test.go b/providers/providers_test.go index 279f8a04..5c5df8a8 100644 --- a/providers/providers_test.go +++ b/providers/providers_test.go @@ -132,8 +132,8 @@ func TestScope(t *testing.T) { }{ { name: "oidc: with no scope provided", - configuredScope: "", configuredType: "oidc", + configuredScope: "", expectedScope: "openid email profile", }, { From 9db036e422f8834a86f04a6ea3532b43cbb1f4b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anderson=20Val=C3=A9rio?= Date: Mon, 2 Dec 2024 10:36:58 -0300 Subject: [PATCH 04/10] revert html formatting --- pkg/app/pagewriter/error.html | 4 ++-- pkg/app/pagewriter/sign_in.html | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/app/pagewriter/error.html b/pkg/app/pagewriter/error.html index be8bca92..86df3272 100644 --- a/pkg/app/pagewriter/error.html +++ b/pkg/app/pagewriter/error.html @@ -5,8 +5,8 @@ {{.StatusCode}} {{.Title}} - - + +