Emit Audit Log Trail on Session Reject#62384
Conversation
21KennethTran
force-pushed
the
kennethtran/ip-pinning-audit
branch
from
ac9cef1 to
d6a2a61
Compare
zmb3
left a comment
zmb3
left a comment
There was a problem hiding this comment.
I can confirm I'm now seeing auth attempt failures for things like IP pinning and device trust:
I'm also seeing some new events for things I can't explain:
These seem to happen when I tsh ssh zmb@host as Teleport user zac. I'm struggling to understand why I'm seeing activity from other users (admin, zac5, root-admin, and leaf-admin).
(Note: I'm sure you didn't introduce this behavior and it's just an artifact of the new audit logging, but I"d like to understand it)
|
Hey @zmb3, @fspmarshall, @rudream, Apologies for the unresponsiveness. I would like to pick back up on this PR. A change I have been working on is moving emitters to Let me know if there are any concerns with having audit event emission here. |
| defer func() { | ||
| if err != nil { | ||
| if a.emitter != nil { | ||
| if user, _ := UserFromContext(ctx); user != nil { |
There was a problem hiding this comment.
Style nit - the more levels of nesting you create, the harder the code is to read.
We can use several techniques to improve this.
- Combine the if statements.
Instead of:
if foo {
if bar {
// stuff
}
}
We can do:
if foo && bar {
// stuff
}
- Invert the conditions and return early. This keeps the happy path left-aligned.
if err == nil || emitter == nil {
return // nothing we can do
}
user, _ := UserFromContext()
if user == nil {
return
}
| methodName = "unknown" | ||
| } | ||
|
|
||
| userMsg := fmt.Sprintf("access denied to method %s: %s", methodName, errorMsg) |
There was a problem hiding this comment.
| userMsg := fmt.Sprintf("access denied to method %s: %s", methodName, errorMsg) | |
| userMsg := fmt.Sprintf("authorization failed for method %s: %s", methodName, errorMsg) |
fspmarshall
left a comment
fspmarshall
left a comment
There was a problem hiding this comment.
I worry we might have a case of misleading naming here. A lot of what the Authorize method does isn't really authorization. And also I worry that we're setting ourselves up for serious event spam if we just bracket this entire method in a catch-all failure event. Have you considered a more targeted approach?
| defer func() { | ||
| if err != nil { | ||
| err = a.convertAuthorizerError(err) | ||
| user, _ := UserFromContext(ctx) | ||
| if user == nil { | ||
| return | ||
| } | ||
|
|
||
| identity := user.GetIdentity() | ||
| username := identity.Username | ||
|
|
||
| // Filter out failures caused by system components so that we only audit events for real users or bots | ||
| if len(identity.SystemRoles) == 0 { | ||
| errorMsg := trace.UserMessage(err) | ||
| if errorMsg == "" { | ||
| errorMsg = err.Error() | ||
| } | ||
|
|
||
| methodName, _ := grpc.Method(ctx) | ||
| if methodName == "" { | ||
| // If no gRPC method is found in the context, attempt to get the request method from the context | ||
| // (e.g. for kube requests). If that also fails, default to "unknown". | ||
| if val := GetRequestMethod(ctx); val != "" { | ||
| methodName = val | ||
| } else { | ||
| methodName = "unknown" | ||
| } | ||
| } | ||
|
|
||
| userMsg := fmt.Sprintf("authorization failed for method %s: %s component %s", methodName, errorMsg, a.TEST) | ||
|
|
||
| event := &apievents.AuthAttempt{ | ||
| Metadata: apievents.Metadata{ | ||
| Type: events.AuthAttemptEvent, | ||
| Code: events.AuthAttemptFailureCode, |
There was a problem hiding this comment.
Indentation appears pretty messed up here. And seems to be missing control flow that I'd assume some of this indentation aught to be related to, like I'd expect some of this to be wrapped in an if err != nil block, but it isn't. Looks like maybe this was pasted incompletely from somewhere else and not formatted?
There was a problem hiding this comment.
My bad, I was testing some changes/logs and decided to commit part of them. Not sure why the lines and indentation were malformed, but should be corrected now.
21KennethTran
force-pushed
the
kennethtran/ip-pinning-audit
branch
from
1300331 to
c99ac75
Compare
21KennethTran
force-pushed
the
kennethtran/ip-pinning-audit
branch
from
c99ac75 to
67059ad
Compare
21KennethTran
force-pushed
the
kennethtran/ip-pinning-audit
branch
from
576fba4 to
2eedcdf
Compare
3 participants
Currently, when the Authorizer rejects a connection (e.g., due to IP pinning), the Auth Server returns a gRPC error to the client but does not emit an audit event.
Added an auditing block within
authz.Authorizer. This ensures that anytrace.AccessDeniederror returned when an authorization attempt fails is captured and emitted as aAuthAttemptFailure(T3007W) event.Addresses #48123
Recreation Steps for IP Pinning (Local):
Note: With these changes, we now see multiple of these Session Rejected Events with the same contents, instead of just one. If we look at the
usermessageof each of these events, we now see the individual RPC calls that are being called/rejected.When failing a session due to locks:
Test Plan
tsh loginas testuser with access privileges to ssh onto a servertsh sshonto a servertsh db loginto postgres database, and we should observe 1 audit log"message": "access denied to method /proto.AuthService/ListResources: lock targeting User:\"testuser\" is in force: locking testuser"tsh db loginafter deleting ipv6 loopback, then we should also observe 1 audit logtsh kube login, then delete ipv6 loopback, thenkubectl get podsshould give us 5 audit logsauth_service.device_trust.mode: off, thentsh loginauth_service.device_trust.mode: offtsh sshshould reject session and leave an audit logtsh app loginshould not reject session and should not leave audit log (global device trust does not affect app access unless user roles target the app)