Make error messages specific #6108

kimlisa · 2021-03-23T08:12:57Z

resolves #6046

Description

Note: there is a bug with trace marshalling proxyErr: #6108 (comment)

Purpose is to allow users with admin privilege that are able to view audit logs, to be able to debug SSO login failures from the UI as much as possible:

emit audit events when there are failures from creating sso auth requests
since we no longer expose any specific errors when users fail to login from UI, returned error messages are more specific so that it is clear what the error is when viewing audit logs
return generic login failed message for sso CLI login failures (which are errors from creating an auth request) similar in the UI dashbaord where we also display generic error message

a-palchikov · 2021-03-23T17:02:59Z

lib/auth/auth_test.go

+	mockE := &events.MockEmitter{}
+
+	emitSSOLoginFailureEvent(context.Background(), mockE, "test", trace.BadParameter("some error"))
+	event := mockE.LastEvent().(*events.UserLogin)


Instead of type assertion (which might potentially trigger a panic), assert the equivalence with require, e.g. it should suffice to:

require.Equal(t, mockE.LastEvent(), &events.UserLogin{ Metadata: events.Metadata{ Type: events.UserLoginEvent, Code: events.UserSSOLoginFailureCode, }, Method: "test", Status: events.Status{ Success: false, Error: "some error", UserMessage: "some error", }, })

and matching on Method will not be necessary since it's also part of the expectation.

a-palchikov · 2021-03-23T17:29:03Z

lib/auth/oidc.go

 	}
 	if idsub != uisub {
-		log.Debugf("OIDC claim subjects don't match '%v' != '%v'.", idsub, uisub)
-		return nil, trace.BadParameter("invalid subject in UserInfo")
+		log.Debugf("OIDC claim subjects does not match '%v' != '%v'.", idsub, uisub)


I think it was grammatically correct before?

a-palchikov · 2021-03-23T17:41:14Z

lib/utils/cli.go

@@ -196,8 +197,15 @@ func UserMessageFromError(err error) string {
 			}
 			fmt.Fprintln(&buf, AllowNewlines(trace.Unwrap(er).Error()))
 		} else {
-			fmt.Fprintln(&buf, AllowNewlines(err.Error()))
+			strErr := err.Error()
+			// Error can be of type trace.proxyError where error message didn't get captured.


This is somewhat misleading. Did you have a specific error that did not transport the message over the wire? All errors coming from the server over HTTP will be of type trace.proxyError and it's supposed to transport the message with it - if that's not the case here, it's probably a bug to be fixed.

I was trying to purposely cause errors when logging in with sso through CLI and I found errors came back as empty strings. I traced into it and I thought maybe it was by design to prevent sensitive error messages from auth from being exposed?

This is how errors get marshalled: https://github.com/gravitational/trace/blob/master/httplib.go#L87
And this is how errors get unmarshalled: https://github.com/gravitational/trace/blob/master/httplib.go#L59
And this is how requests are made in both client and proxy: https://github.com/gravitational/teleport/blob/master/lib/auth/clt.go#L251

Given tsh client -> proxy -> auth make request
First marshal/unmarshal auth to proxy: when error from auth gets marshalled, the error is of type TraceErr, and when it gets unmarshalled, error is unmarshaled into TraceErr and wrapped in proxyErr

Second marshal/unmarshal proxy to client: When this proxyErr gets marshalled, b/c it is not a type of TraceErr, everything gets stripped except error field, and when this gets unmarshalled it gets unmarshalled into trace error struct that only has message as its field, so the error field never gets saved and message is empty.

Ok, I see. This a bug in trace and should be definitely fixed so it's possible to send errors over the wire multiple times. I'll see how complicated this would be.

Since this is a bug, I went back to all console logins github, oidc, saml, in the webapi layer and made sure to return a generic error message for all errors to hide sensitive data. This is what we did for UI dashboard login failures (and validating auth callbacks): 50078d3

kimlisa · 2021-03-24T05:10:13Z

lib/auth/oidc.go

-		if err := a.emitter.EmitAuditEvent(ctx, &events.UserLogin{
-			Metadata: events.Metadata{
-				Type: events.UserLoginEvent,
-				Code: events.UserSSOLoginFailureCode,
-			},
-			Method: events.LoginMethodOIDC,
-			Status: events.Status{
-				Success:     false,
-				Error:       trace.Unwrap(ctx.Err()).Error(),
-				UserMessage: err.Error(),
-			},
-		}); err != nil {
-			log.WithError(err).Warn("Failed to emit OIDC login failure event.")
-		}
-		// return user-friendly error hiding the actual error in the event
-		// logs for security purposes
-		return nil, trace.ConnectionProblem(nil,
-			"failed to login with %v",
-			conn.GetDisplay())


Since we no longer show the users the actual login errors (but a generic login failed msg), I didn't think it was necessary to hide this error and simply return the actual error for the outer caller to emit the events (also removes double emitting)

a-palchikov · 2021-03-24T16:56:53Z

lib/auth/oidc.go

-			}
+
+		if err := a.emitter.EmitAuditEvent(a.closeCtx, event); err != nil {
+			log.WithError(err).Warn("Failed to emit OIDC login failed event.")


nit: log these at Debug if these aren't critical.

i constructed these to be consistent with others (others use warn), i think it also makes sense to use warn here b/c missing an event for auditing should stand out

a-palchikov · 2021-03-24T16:57:59Z

lib/web/apiserver.go

+	logger := h.log.WithField("auth", "github")
+	logger.Debug("Console login start.")
+
+	generalErr := trace.AccessDenied(ssoLoginConsoleErr)


Even though this seems to reduce the error handling (if that's the intent), it will obscure the actual point of error emission and might complicate analysis as several places would be likely candidates as the error source - it's better to emit the error at each point with the corresponding trace error constructor.

Revised so that each error returns its own trace err so tracing is closer to origin of actual error

russjones

Bot.

- Remove inner emit event for createOIDCClient to prevent double emits on error. - Return generic error message for console login errors

Purpose is to allow users with admin privilege that are able to view audit logs, to be able to debug SSO login failures from the UI as much as possible * Return generic error message for sso console login failures to hide sensitive data from reaching client. Previously errors were returning as empty messages b/c of a trace bug. * Remove emit event for createOIDCClient to allow outer caller to emit event and prevent double emits on error. * Temporarily direct users to check teleports log on errors that come back empty to tsh client.

…6204) Purpose is to allow users with admin privilege that are able to view audit logs, to be able to debug SSO login failures from the UI as much as possible * Return generic error message for sso console login failures to hide sensitive data from reaching client. Previously errors were returning as empty messages b/c of a trace bug. * Remove emit event for createOIDCClient to allow outer caller to emit event and prevent double emits on error. * Temporarily direct users to check teleports log on errors that come back empty to tsh client.

kimlisa requested review from klizhentas, r0mant and russjones as code owners March 23, 2021 08:12

kimlisa marked this pull request as draft March 23, 2021 09:04

kimlisa force-pushed the lisa/emit-sso-login-failures branch 2 times, most recently from 5dce855 to 99a651a Compare March 23, 2021 09:44

a-palchikov reviewed Mar 23, 2021

View reviewed changes

kimlisa force-pushed the lisa/emit-sso-login-failures branch 2 times, most recently from 246a96d to 50078d3 Compare March 24, 2021 04:59

kimlisa marked this pull request as ready for review March 24, 2021 04:59

kimlisa requested a review from alex-kovoy as a code owner March 24, 2021 04:59

kimlisa commented Mar 24, 2021

View reviewed changes

alex-kovoy approved these changes Mar 24, 2021

View reviewed changes

kimlisa force-pushed the lisa/emit-sso-login-failures branch from 50078d3 to 0c10acf Compare March 24, 2021 16:38

a-palchikov approved these changes Mar 24, 2021

View reviewed changes

russjones approved these changes Mar 24, 2021

View reviewed changes

kimlisa force-pushed the lisa/emit-sso-login-failures branch from 0c10acf to b352f18 Compare March 25, 2021 17:02

kimlisa added 3 commits March 25, 2021 10:05

Make error messages specific

cbfaa17

Remove double emit and return generic error message

739c292

- Remove inner emit event for createOIDCClient to prevent double emits on error. - Return generic error message for console login errors

Define trace err closer to the origin of each actual error

342d4b2

kimlisa force-pushed the lisa/emit-sso-login-failures branch from b352f18 to 342d4b2 Compare March 25, 2021 17:05

kimlisa merged commit 940c83c into master Mar 25, 2021

kimlisa deleted the lisa/emit-sso-login-failures branch March 25, 2021 17:36

kimlisa mentioned this pull request Mar 29, 2021

[Backport 6.1] Make SSO login failure event emit more specific errors (#6108) #6204

Merged

webvictim mentioned this pull request May 5, 2021

ami auto branch 1620236164 #6745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error messages specific #6108

Make error messages specific #6108

kimlisa commented Mar 23, 2021 •

edited

Loading

a-palchikov Mar 23, 2021

kimlisa Mar 24, 2021

a-palchikov Mar 23, 2021

kimlisa Mar 24, 2021

a-palchikov Mar 23, 2021

kimlisa Mar 23, 2021

a-palchikov Mar 23, 2021

kimlisa Mar 24, 2021 •

edited

Loading

kimlisa Mar 24, 2021

a-palchikov Mar 24, 2021

kimlisa Mar 25, 2021

a-palchikov Mar 24, 2021

kimlisa Mar 25, 2021

russjones left a comment

Make error messages specific #6108

Make error messages specific #6108

Conversation

kimlisa commented Mar 23, 2021 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimlisa Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones left a comment

Choose a reason for hiding this comment

kimlisa commented Mar 23, 2021 •

edited

Loading

kimlisa Mar 24, 2021 •

edited

Loading