From 87eeeffa1c886f1e9404413b039e60f40a47c9bc Mon Sep 17 00:00:00 2001 From: Gabe Kangas Date: Fri, 23 Dec 2022 20:20:59 -0800 Subject: [PATCH] Prune expired auth requests + add global max limit. Closes #2490 --- auth/fediverse/fediverse.go | 51 ++++++++++++++++++++++--- auth/fediverse/fediverse_test.go | 43 ++++++++++++++++++--- auth/indieauth/client.go | 34 ++++++++++++++++- auth/indieauth/helpers.go | 2 + auth/indieauth/indieauth_test.go | 35 +++++++++++++++++ auth/indieauth/request.go | 6 ++- auth/indieauth/server.go | 9 +++++ controllers/auth/fediverse/fediverse.go | 7 +++- controllers/auth/indieauth/server.go | 2 +- 9 files changed, 175 insertions(+), 14 deletions(-) create mode 100644 auth/indieauth/indieauth_test.go diff --git a/auth/fediverse/fediverse.go b/auth/fediverse/fediverse.go index 8fced2fb2..0aeb1e5cb 100644 --- a/auth/fediverse/fediverse.go +++ b/auth/fediverse/fediverse.go @@ -2,9 +2,13 @@ package fediverse import ( "crypto/rand" + "errors" "io" "strings" + "sync" "time" + + log "github.com/sirupsen/logrus" ) // OTPRegistration represents a single OTP request. @@ -18,19 +22,53 @@ type OTPRegistration struct { // Key by access token to limit one OTP request for a person // to be active at a time. -var pendingAuthRequests = make(map[string]OTPRegistration) +var ( + pendingAuthRequests = make(map[string]OTPRegistration) + lock = sync.Mutex{} +) -const registrationTimeout = time.Minute * 10 +const ( + registrationTimeout = time.Minute * 10 + maxPendingRequests = 1000 +) + +func init() { + go setupExpiredRequestPruner() +} + +// Clear out any pending requests that have been pending for greater than +// the specified timeout value. +func setupExpiredRequestPruner() { + pruneExpiredRequestsTimer := time.NewTicker(registrationTimeout) + + for range pruneExpiredRequestsTimer.C { + lock.Lock() + log.Debugln("Pruning expired OTP requests.") + for k, v := range pendingAuthRequests { + if time.Since(v.Timestamp) > registrationTimeout { + delete(pendingAuthRequests, k) + } + } + lock.Unlock() + } +} // RegisterFediverseOTP will start the OTP flow for a user, creating a new // code and returning it to be sent to a destination. -func RegisterFediverseOTP(accessToken, userID, userDisplayName, account string) (OTPRegistration, bool) { +func RegisterFediverseOTP(accessToken, userID, userDisplayName, account string) (OTPRegistration, bool, error) { request, requestExists := pendingAuthRequests[accessToken] // If a request is already registered and has not expired then return that // existing request. if requestExists && time.Since(request.Timestamp) < registrationTimeout { - return request, false + return request, false, nil + } + + lock.Lock() + defer lock.Unlock() + + if len(pendingAuthRequests)+1 > maxPendingRequests { + return request, false, errors.New("Please try again later. Too many pending requests.") } code, _ := createCode() @@ -43,7 +81,7 @@ func RegisterFediverseOTP(accessToken, userID, userDisplayName, account string) } pendingAuthRequests[accessToken] = r - return r, true + return r, true, nil } // ValidateFediverseOTP will verify a OTP code for a auth request. @@ -54,6 +92,9 @@ func ValidateFediverseOTP(accessToken, code string) (bool, *OTPRegistration) { return false, nil } + lock.Lock() + defer lock.Unlock() + delete(pendingAuthRequests, accessToken) return true, &request } diff --git a/auth/fediverse/fediverse_test.go b/auth/fediverse/fediverse_test.go index 8b4059d9c..c9b3b3550 100644 --- a/auth/fediverse/fediverse_test.go +++ b/auth/fediverse/fediverse_test.go @@ -3,6 +3,8 @@ package fediverse import ( "strings" "testing" + + "github.com/owncast/owncast/utils" ) const ( @@ -13,7 +15,10 @@ const ( ) func TestOTPFlowValidation(t *testing.T) { - r, success := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) + r, success, err := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) + if err != nil { + t.Error(err) + } if !success { t.Error("Registration should be permitted.") @@ -50,8 +55,8 @@ func TestOTPFlowValidation(t *testing.T) { } func TestSingleOTPFlowRequest(t *testing.T) { - r1, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) - r2, s2 := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) + r1, _, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) + r2, s2, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) if r1.Code != r2.Code { t.Error("Only one registration should be permitted.") @@ -65,14 +70,42 @@ func TestSingleOTPFlowRequest(t *testing.T) { func TestAccountCaseInsensitive(t *testing.T) { account := "Account" accessToken := "another-fake-access-token" - r1, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) + r1, _, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, account) _, reg1 := ValidateFediverseOTP(accessToken, r1.Code) // Simulate second auth with account in different case - r2, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, strings.ToUpper(account)) + r2, _, _ := RegisterFediverseOTP(accessToken, userID, userDisplayName, strings.ToUpper(account)) _, reg2 := ValidateFediverseOTP(accessToken, r2.Code) if reg1.Account != reg2.Account { t.Errorf("Account names should be case-insensitive: %s %s", reg1.Account, reg2.Account) } } + +func TestLimitGlobalPendingRequests(t *testing.T) { + for i := 0; i < maxPendingRequests-1; i++ { + at, _ := utils.GenerateRandomString(10) + uid, _ := utils.GenerateRandomString(10) + account, _ := utils.GenerateRandomString(10) + + _, success, error := RegisterFediverseOTP(at, uid, "userDisplayName", account) + if !success { + t.Error("Registration should be permitted.", i, " of ", len(pendingAuthRequests)) + } + if error != nil { + t.Error(error) + } + } + + // This one should fail + at, _ := utils.GenerateRandomString(10) + uid, _ := utils.GenerateRandomString(10) + account, _ := utils.GenerateRandomString(10) + _, success, error := RegisterFediverseOTP(at, uid, "userDisplayName", account) + if success { + t.Error("Registration should not be permitted.") + } + if error == nil { + t.Error("Error should be returned.") + } +} diff --git a/auth/indieauth/client.go b/auth/indieauth/client.go index ac8869927..8f0dd1b34 100644 --- a/auth/indieauth/client.go +++ b/auth/indieauth/client.go @@ -8,16 +8,48 @@ import ( "net/url" "strconv" "strings" + "sync" + "time" "github.com/owncast/owncast/core/data" "github.com/pkg/errors" log "github.com/sirupsen/logrus" ) -var pendingAuthRequests = make(map[string]*Request) +var ( + pendingAuthRequests = make(map[string]*Request) + lock = sync.Mutex{} +) + +const registrationTimeout = time.Minute * 10 + +func init() { + go setupExpiredRequestPruner() +} + +// Clear out any pending requests that have been pending for greater than +// the specified timeout value. +func setupExpiredRequestPruner() { + pruneExpiredRequestsTimer := time.NewTicker(registrationTimeout) + + for range pruneExpiredRequestsTimer.C { + lock.Lock() + log.Debugln("Pruning expired IndieAuth requests.") + for k, v := range pendingAuthRequests { + if time.Since(v.Timestamp) > registrationTimeout { + delete(pendingAuthRequests, k) + } + } + lock.Unlock() + } +} // StartAuthFlow will begin the IndieAuth flow by generating an auth request. func StartAuthFlow(authHost, userID, accessToken, displayName string) (*url.URL, error) { + if len(pendingAuthRequests) >= maxPendingRequests { + return nil, errors.New("Please try again later. Too many pending requests.") + } + serverURL := data.GetServerURL() if serverURL == "" { return nil, errors.New("Owncast server URL must be set when using auth") diff --git a/auth/indieauth/helpers.go b/auth/indieauth/helpers.go index f120327e7..f777c8754 100644 --- a/auth/indieauth/helpers.go +++ b/auth/indieauth/helpers.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "strings" + "time" "github.com/andybalholm/cascadia" "github.com/pkg/errors" @@ -63,6 +64,7 @@ func createAuthRequest(authDestination, userID, displayName, accessToken, baseSe State: state, Redirect: &redirect, Callback: &callbackURL, + Timestamp: time.Now(), }, nil } diff --git a/auth/indieauth/indieauth_test.go b/auth/indieauth/indieauth_test.go new file mode 100644 index 000000000..20c5b5970 --- /dev/null +++ b/auth/indieauth/indieauth_test.go @@ -0,0 +1,35 @@ +package indieauth + +import ( + "testing" + + "github.com/owncast/owncast/utils" +) + +func TestLimitGlobalPendingRequests(t *testing.T) { + // Simulate 10 pending requests + for i := 0; i < maxPendingRequests-1; i++ { + cid, _ := utils.GenerateRandomString(10) + redirectURL, _ := utils.GenerateRandomString(10) + cc, _ := utils.GenerateRandomString(10) + state, _ := utils.GenerateRandomString(10) + me, _ := utils.GenerateRandomString(10) + + _, err := StartServerAuth(cid, redirectURL, cc, state, me) + if err != nil { + t.Error("Registration should be permitted.", i, " of ", len(pendingAuthRequests), err) + } + } + + // This should throw an error + cid, _ := utils.GenerateRandomString(10) + redirectURL, _ := utils.GenerateRandomString(10) + cc, _ := utils.GenerateRandomString(10) + state, _ := utils.GenerateRandomString(10) + me, _ := utils.GenerateRandomString(10) + + _, err := StartServerAuth(cid, redirectURL, cc, state, me) + if err == nil { + t.Error("Registration should not be permitted.") + } +} diff --git a/auth/indieauth/request.go b/auth/indieauth/request.go index a431fc1d6..ada375412 100644 --- a/auth/indieauth/request.go +++ b/auth/indieauth/request.go @@ -1,6 +1,9 @@ package indieauth -import "net/url" +import ( + "net/url" + "time" +) // Request represents a single in-flight IndieAuth request. type Request struct { @@ -15,4 +18,5 @@ type Request struct { CodeChallenge string State string Me *url.URL + Timestamp time.Time } diff --git a/auth/indieauth/server.go b/auth/indieauth/server.go index 98738a0f6..8c91cfc34 100644 --- a/auth/indieauth/server.go +++ b/auth/indieauth/server.go @@ -2,6 +2,7 @@ package indieauth import ( "fmt" + "time" "github.com/owncast/owncast/core/data" "github.com/pkg/errors" @@ -17,6 +18,7 @@ type ServerAuthRequest struct { State string Me string Code string + Timestamp time.Time } // ServerProfile represents basic user-provided data about this Owncast instance. @@ -38,10 +40,16 @@ type ServerProfileResponse struct { var pendingServerAuthRequests = map[string]ServerAuthRequest{} +const maxPendingRequests = 1000 + // StartServerAuth will handle the authentication for the admin user of this // Owncast server. Initiated via a GET of the auth endpoint. // https://indieweb.org/authorization-endpoint func StartServerAuth(clientID, redirectURI, codeChallenge, state, me string) (*ServerAuthRequest, error) { + if len(pendingServerAuthRequests)+1 >= maxPendingRequests { + return nil, errors.New("Please try again later. Too many pending requests.") + } + code := shortid.MustGenerate() r := ServerAuthRequest{ @@ -51,6 +59,7 @@ func StartServerAuth(clientID, redirectURI, codeChallenge, state, me string) (*S State: state, Me: me, Code: code, + Timestamp: time.Now(), } pendingServerAuthRequests[code] = r diff --git a/controllers/auth/fediverse/fediverse.go b/controllers/auth/fediverse/fediverse.go index 51e70dfb6..600bbc371 100644 --- a/controllers/auth/fediverse/fediverse.go +++ b/controllers/auth/fediverse/fediverse.go @@ -28,7 +28,12 @@ func RegisterFediverseOTPRequest(u user.User, w http.ResponseWriter, r *http.Req } accessToken := r.URL.Query().Get("accessToken") - reg, success := fediverseauth.RegisterFediverseOTP(accessToken, u.ID, u.DisplayName, req.FediverseAccount) + reg, success, err := fediverseauth.RegisterFediverseOTP(accessToken, u.ID, u.DisplayName, req.FediverseAccount) + if err != nil { + controllers.WriteSimpleResponse(w, false, "Could not register auth request: "+err.Error()) + return + } + if !success { controllers.WriteSimpleResponse(w, false, "Could not register auth request. One may already be pending. Try again later.") return diff --git a/controllers/auth/indieauth/server.go b/controllers/auth/indieauth/server.go index a994be6eb..92cb4562f 100644 --- a/controllers/auth/indieauth/server.go +++ b/controllers/auth/indieauth/server.go @@ -33,7 +33,7 @@ func handleAuthEndpointGet(w http.ResponseWriter, r *http.Request) { request, err := ia.StartServerAuth(clientID, redirectURI, codeChallenge, state, me) if err != nil { - // Return a human readable, HTML page as an error. JSON is no use here. + _ = controllers.WriteString(w, err.Error(), http.StatusInternalServerError) return }