Fixed app session start audit events for Device Trust failures#62019
Fixed app session start audit events for Device Trust failures#6201921KennethTran merged 1 commit intomasterfrom
Conversation
zmb3
left a comment
There was a problem hiding this comment.
The PR title could be more descriptive. It refers to "session start" events, but the change is actually about preventing "app session start" events from being emitted for sessions that are unsuccessful.
There was a problem hiding this comment.
I wonder if we should change this doc comment.
Are there any cases where this is being emitted on success?
There was a problem hiding this comment.
There doesn't seem to be any instances where an AuthAttempt has a success in the code field. It seems that successful events have more granular types and success codes. I will change the doc comment for more clarity.
lib/auth/sessions.go
Outdated
|
|
||
| // Validate that the request has the deviceID extension; non-empty value means user authenticated with that device with CA | ||
| if deviceTrustRequired { | ||
| if req.DeviceExtensions.DeviceID == "" { |
There was a problem hiding this comment.
@codingllama do you think this is the right place to check whether a trusted device credential is being presented?
Context:
Prior to this change, when an app session is rejected due to device trust, the request traverses through the proxy, and the app agent itself sends back an HTTP response saying that device trust is required (instead of connecting to the app).
The problem is from the proxy's perspective, traffic is being routed to an agent, so an app session start event gets emitted. This is a problem for cases when you alert on accesses that come from untrusted devices (which is somethign we do internally!)
There was a problem hiding this comment.
I don't mind the early check, but I dislike the addition (and duplication!) of authz-related device trust logic to this handler. I think this is the responsibility of the AccessChecker, so I would prefer an early CheckAccess if possible, like you suggested above.
If for some reason that doesn't work, then we could expose an AccessChecker.CheckDeviceAccess method instead.
WDYT?
There was a problem hiding this comment.
I opted to expose an AccessChecker.CheckDeviceAccess method since we don't have an app session to use CheckAccess on yet at this point. Let me know if this implementation looks good!
lib/auth/sessions.go
Outdated
| for _, role := range checker.Roles() { | ||
| mode := role.GetOptions().DeviceTrustMode | ||
|
|
||
| if mode == constants.DeviceTrustModeRequired || | ||
| mode == constants.DeviceTrustModeRequiredForHumans { | ||
| deviceTrustRequired = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks very similar to (RoleSet).checkAccess():
// Device verification.
var deviceVerificationPassed bool
switch role.GetOptions().DeviceTrustMode {
case constants.DeviceTrustModeOff, constants.DeviceTrustModeOptional, "":
// OK, extensions not enforced.
deviceVerificationPassed = true
case constants.DeviceTrustModeRequiredForHumans:
// Humans must use trusted devices, bots can use untrusted devices.
deviceVerificationPassed = deviceTrusted || state.IsBot
case constants.DeviceTrustModeRequired:
// Only trusted devices allowed for bot human and bot users.
deviceVerificationPassed = deviceTrusted
}
if !deviceVerificationPassed {
logger.LogAttrs(ctx, logutils.TraceLevel, "Access to resource denied, role requires a trusted device",
slog.String("role", role.GetName()),
)
return ErrTrustedDeviceRequired
}
I wonder if we should be running the access check sooner rather than duplicating some of its code here?
There was a problem hiding this comment.
That is true. I am not sure if we really need all of the other checks in roleset.checkAccess() like label matching or login usernames, before issuing an app session certificate, which from my understanding comes from the Auth server. The access checks comes after the proxy connects to the app service, which should be when authorization takes place.
Let me know if doing the access checks earlier would be the better choice here. I could also refactor the device trust section into a utility function to use in these two parts.
lib/auth/sessions.go
Outdated
| ServerMetadata: apievents.ServerMetadata{ | ||
| ServerVersion: teleport.Version, | ||
| ServerID: a.ServerID, | ||
| ServerNamespace: apidefaults.Namespace, | ||
| }, |
There was a problem hiding this comment.
Should we omit ServerMetadata here since we know the access attempt was for an app, and we have app metadata below?
There was a problem hiding this comment.
ServerMetadata tells what specific Auth server emitted the event, which could be useful for audit logs? I also chose to include this to mirror the AppSessionStart emitted in the same function on a successful event.
8aa218d to
83120c3
Compare
lib/auth/sessions.go
Outdated
|
|
||
| // Validate that the request has the deviceID extension; non-empty value means user authenticated with that device with CA | ||
| if deviceTrustRequired { | ||
| if req.DeviceExtensions.DeviceID == "" { |
There was a problem hiding this comment.
I don't mind the early check, but I dislike the addition (and duplication!) of authz-related device trust logic to this handler. I think this is the responsibility of the AccessChecker, so I would prefer an early CheckAccess if possible, like you suggested above.
If for some reason that doesn't work, then we could expose an AccessChecker.CheckDeviceAccess method instead.
WDYT?
|
Somewhat related: #62523. Reviews welcome! |
|
@21KennethTran - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
2d17ca2 to
4f871d0
Compare
2eb50bb to
4317a09
Compare
4317a09 to
c488c31
Compare
c488c31 to
ab2ffa6
Compare
ab2ffa6 to
e35505c
Compare
e35505c to
8813f3a
Compare
8813f3a to
bd545ad
Compare
bd545ad to
0db0e97
Compare
When Device Trust and MFA is enabled for accessing a resource and a user without a trusted device tries to access that resource, their session should get rejected and an auth attempt fail event is generated in the audit log.
Since this type of session rejection is an auth failure, the type of event generated in the audit logs should be an AppSessionStart. The AppMetadata struct is added to this to provide more information about the resource the user tried to access.
Following the recreation steps from #54139 ,
Before:
device-trust-before.mov
After:
device-trust-after2.mov
Here is the audit log fields with the changes from the demo above:
changelog: Reject application session requests and emit a failed session start audit event when a trusted device is required but not provided