Add service type to auth event logs (#57646)

* Add service type tracking to auth related events to identify gaps
This commit is contained in:
Dan Adler 2023-10-20 08:42:27 -07:00 committed by GitHub
parent c4557864e5
commit de3f1cd2ec
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 41 additions and 18 deletions

View File

@ -477,11 +477,13 @@ export function setDebugEventLoggingEnabled(enabled: boolean): void {
function handleQueryEvents(url: string): void {
const parsedUrl = new URL(url)
if (parsedUrl.searchParams.has('signup')) {
eventLogger.log('web:auth:signUpCompleted')
const args = { serviceType: parsedUrl.searchParams.get('signup') || '' }
eventLogger.log('web:auth:signUpCompleted', args, args)
}
if (parsedUrl.searchParams.has('signin')) {
eventLogger.log('web:auth:signInCompleted')
const args = { serviceType: parsedUrl.searchParams.get('signin') || '' }
eventLogger.log('web:auth:signInCompleted', args, args)
}
stripURLParameters(url, ['utm_campaign', 'utm_source', 'utm_medium', 'signup', 'signin'])

View File

@ -25,21 +25,21 @@ func SafeRedirectURL(urlStr string) string {
}
// Add a ?signup= or ?signin= parameter to a redirect URL.
func AddPostAuthRedirectParametersToURL(u *url.URL, newUserCreated bool) {
func AddPostAuthRedirectParametersToURL(u *url.URL, newUserCreated bool, serviceType string) {
q := u.Query()
if newUserCreated {
q.Add("signup", "")
q.Add("signup", serviceType)
} else {
q.Add("signin", "")
q.Add("signin", serviceType)
}
u.RawQuery = q.Encode()
}
func AddPostAuthRedirectParametersToString(urlStr string, newUserCreated bool) string {
func AddPostAuthRedirectParametersToString(urlStr string, newUserCreated bool, serviceType string) string {
u, err := url.Parse(urlStr)
if err != nil {
return urlStr
}
AddPostAuthRedirectParametersToURL(u, newUserCreated)
AddPostAuthRedirectParametersToURL(u, newUserCreated, serviceType)
return u.String()
}

View File

@ -164,6 +164,9 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (n
}
const eventName = "ExternalAuthSignupSucceeded"
// SECURITY: This args map is treated as a public argument in the LogEvent call below, so it must not contain
// any sensitive data.
args, err := json.Marshal(map[string]any{
// NOTE: The conventional name should be "service_type", but keeping as-is for
// backwards capability.
@ -185,10 +188,11 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (n
ctx,
db,
usagestats.Event{
EventName: eventName,
UserID: act.UID,
Argument: args,
Source: "BACKEND",
EventName: eventName,
UserID: act.UID,
Argument: args,
PublicArgument: args,
Source: "BACKEND",
},
)
if err != nil {

View File

@ -109,6 +109,10 @@ func (s *sessionIssuerHelper) AuthFailedEventName() database.SecurityEventName {
return database.SecurityEventAzureDevOpsAuthFailed
}
func (s *sessionIssuerHelper) GetServiceID() string {
return s.ServiceID
}
func (s *sessionIssuerHelper) verifyAllowOrgs(ctx context.Context, profile *azuredevops.Profile, token *oauth2.Token) (bool, error) {
if len(s.allowOrgs) == 0 {
return true, nil

View File

@ -39,6 +39,10 @@ func (s *sessionIssuerHelper) AuthFailedEventName() database.SecurityEventName {
return database.SecurityEventBitbucketCloudAuthFailed
}
func (s *sessionIssuerHelper) GetServiceID() string {
return s.baseURL.String()
}
func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2.Token, anonymousUserID, firstSourceURL, lastSourceURL string) (newUserCreated bool, actr *actor.Actor, safeErrMsg string, err error) {
var client bitbucketcloud.Client
if s.client != nil {

View File

@ -46,6 +46,10 @@ func (s *sessionIssuerHelper) AuthFailedEventName() database.SecurityEventName {
return database.SecurityEventGitHubAuthFailed
}
func (s *sessionIssuerHelper) GetServiceID() string {
return s.ServiceID
}
func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2.Token, anonymousUserID, firstSourceURL, lastSourceURL string) (newUserCreated bool, actr *actor.Actor, safeErrMsg string, err error) {
ghUser, err := github.UserFromContext(ctx)

View File

@ -38,6 +38,10 @@ func (s *sessionIssuerHelper) AuthFailedEventName() database.SecurityEventName {
return database.SecurityEventGitLabAuthFailed
}
func (s *sessionIssuerHelper) GetServiceID() string {
return s.ServiceID
}
func (s *sessionIssuerHelper) GetOrCreateUser(ctx context.Context, token *oauth2.Token, anonymousUserID, firstSourceURL, lastSourceURL string) (newUserCreated bool, actr *actor.Actor, safeErrMsg string, err error) {
gUser, err := UserFromContext(ctx)
if err != nil {

View File

@ -119,7 +119,7 @@ func middleware(db database.DB) func(next http.Handler) http.Handler {
return
}
auth.AddPostAuthRedirectParametersToURL(r.URL, newUserCreated)
auth.AddPostAuthRedirectParametersToURL(r.URL, newUserCreated, "http-header")
r = r.WithContext(actor.WithActor(r.Context(), &actor.Actor{UID: userID}))
next.ServeHTTP(w, r)
})

View File

@ -35,6 +35,7 @@ type SessionIssuerHelper interface {
SessionData(token *oauth2.Token) SessionData
AuthSucceededEventName() database.SecurityEventName
AuthFailedEventName() database.SecurityEventName
GetServiceID() string
}
func SessionIssuer(logger log.Logger, db database.DB, s SessionIssuerHelper, sessionKey string) http.Handler {
@ -145,7 +146,7 @@ func SessionIssuer(logger log.Logger, db database.DB, s SessionIssuerHelper, ses
logger.Warn("Failed to set OAuth session data. The session is still secure, but Sourcegraph will be unable to revoke the user's token or redirect the user to the end-session endpoint after the user signs out of Sourcegraph.", log.Error(err))
}
redirectURL := auth.AddPostAuthRedirectParametersToString(state.Redirect, newUserCreated)
redirectURL := auth.AddPostAuthRedirectParametersToString(state.Redirect, newUserCreated, "OAuth::"+s.GetServiceID())
http.Redirect(w, r, auth.SafeRedirectURL(redirectURL), http.StatusFound)
})
}

View File

@ -345,7 +345,7 @@ func AuthCallback(db database.DB, r *http.Request, stateCookieName, usernamePref
}
// Add a ?signup= or ?signin= parameter to the redirect URL.
redirectURL := auth.AddPostAuthRedirectParametersToString(state.Redirect, newUserCreated)
redirectURL := auth.AddPostAuthRedirectParametersToString(state.Redirect, newUserCreated, "OpenIDConnect")
user, err := db.Users().GetByID(r.Context(), actor.UID)
if err != nil {

View File

@ -293,7 +293,7 @@ func TestMiddleware(t *testing.T) {
if want := http.StatusFound; resp.StatusCode != want {
t.Errorf("got status code %v, want %v", resp.StatusCode, want)
}
if got, want := resp.Header.Get("Location"), "/redirect?signin="; got != want {
if got, want := resp.Header.Get("Location"), "/redirect?signin=OpenIDConnect"; got != want {
t.Errorf("got redirect URL %v, want %v", got, want)
}
})

View File

@ -181,7 +181,7 @@ func samlSPHandler(db database.DB) func(w http.ResponseWriter, r *http.Request)
}
// Add a ?signup= or ?signin= parameter to the redirect URL.
redirectURL := auth.AddPostAuthRedirectParametersToString(relayState.ReturnToURL, newUserCreated)
redirectURL := auth.AddPostAuthRedirectParametersToString(relayState.ReturnToURL, newUserCreated, "SAML")
// 🚨 SECURITY: Call auth.SafeRedirectURL to avoid an open-redirect vuln.
http.Redirect(w, r, auth.SafeRedirectURL(redirectURL), http.StatusFound)

View File

@ -374,7 +374,7 @@ func TestMiddleware(t *testing.T) {
if want := http.StatusFound; resp.StatusCode != want {
t.Errorf("got status code %v, want %v", resp.StatusCode, want)
}
if got, want1, want2 := resp.Header.Get("Location"), "http://example.com/?signin=", "/?signin="; got != want1 && got != want2 {
if got, want1, want2 := resp.Header.Get("Location"), "http://example.com/?signin=SAML", "/?signin=SAML"; got != want1 && got != want2 {
t.Errorf("got redirect location %v, want %v or %v", got, want1, want2)
}

View File

@ -270,7 +270,7 @@ func TestMiddleware(t *testing.T) {
}
resp := mocks.doRequest(http.MethodGet, urlStr, "", cookies, false)
assert.Equal(t, http.StatusFound, resp.StatusCode)
wantRedirect := fmt.Sprintf(`%s?signin=`, state.Redirect)
wantRedirect := fmt.Sprintf(`%s?signin=OpenIDConnect`, state.Redirect)
assert.Equal(t, wantRedirect, resp.Header.Get("Location"))
mockrequire.CalledOnce(t, mocks.usersStore.SetIsSiteAdminFunc)
})