Ensure Device Trust Requirements Apply to Correct Applications

[21KennethTran]

Fixes and issue introduced in #62019 where device trust requirements from a role were mistakenly applied to all applications instead of the specific applications matched by the role.

Test Plan

• Scenario 1: Original Device Trust
• Have an application with label env: prod
• Have a role with device_trust_mode: required and allows resources with labels env: prod
• After attempting to create an app session from an untrusted device, we should see an Access Denied page and an App Session Start Failure in the audit logs
• Scenario 2: Label-Based
• Have Application A with label env: test and another Application B with label env: prod
• Have Role A with device_trust_mode: required and allows resources with labels env: prod and another Role B with device_trust_mode: off and allows resources with labels *: *
• User with Role A and Role B from an untrusted device should be able to access Application A, but should not be able to access Application B
• Scenario 3: Static Apps
• Same as Scenario 2, but Application A and B are static apps with reasonable keep_alive_interval
• To recreate the case where a.GetApp() does not recognize the app, but passes the request along, start Teleport with env: test label on Application B, then restart Teleport with env:prod label on Application B, and access Application B with an untrusted device
• Right now, the UI doesn't show a access denied page and this event is audited as aAppSessionStart
• After a while, the UI updates the labels on Application B correctly but the same UI and audit issue persist when attempting to access the resource with an untrusted device

[21KennethTran]

Hey @codingllama and @zmb3, tagging you both here as this fixes the device trust logic from my previous PR. Previously, having a required device trust in one role would apply to all of the user's apps when it should only apply to apps defined in the allow section of the role spec.

I plan on adding some more tests for userTraits wrappers.Traits, but let me know if there are other places I should look at!

[codingllama]

Please address the test failures.

I also suggest we do some manual testing and document the test scenarios in the PR description. Ie, something like this:

Test Plan

• Test scenario 1
• Test scenario 2
• (etc)

[zmb3]

@rosstimothy any concerns from a scale perspective of having to fetch the app resource here? This code used to operate only on the contents of the certificate, and now it needs the target resource's labels.

[rosstimothy]

A cache read is going to be cheaper than a backend read. Perhaps we should take the opportunity to add a benchmark test to measure the impact of this change?

[espadolini]

Doesn't this break the use of apps defined statically for which there's only app_servers but not any matching app?

[codingllama]

How do we do RBAC for those scenarios?

What if we did all the new logic here on a best-effort basis - ie, we try to determine whether a device trust failure would happen, but if we fail to do that (app not found, unrelated RBAC errors, etc) we simply let it through so that it fails (or not) downstream?

Alternatively, what if we pushed audit event to the place where RBAC truly happens (lib/srv/app, I presume)? This way we'd avoid the dupe logic and any potential false resolutions.

[espadolini]

Outside of the app service, we assume that RBAC applies equally to all the app_servers for a given app name and then when RBAC is applied by the agent serving the app, it applies RBAC on the app it's serving (so if multiple static apps are registered with inconsistent labels then the users will just see inconsistent behavior and there's not much we can do about that).

If all we have is an app name here and we're not already checking app_servers I'd be a little wary of adding that check since that will require going through all the app servers in storage.

[21KennethTran]

I decided to add a benchmark test to see the impact of a.GetApp. I tested it on number of calls to CreateAppSessionFromReq, and it seems that at least for this specific setup (1 app, 2 roles), calling a.GetApp doesn't add too much overhead if we compare our Fail case which stops at audit emission after a.GetApp, and the success/pass cases which goes through with the rest of function:

BenchmarkCreateAppSession/Success_App_With_Device-12                5047            248953 ns/op          143970 B/op       1826 allocs/op
BenchmarkCreateAppSession/Fail_App_No_Device-12                    29995             38501 ns/op           33638 B/op        406 allocs/op
BenchmarkCreateAppSession/Pass_Static_App-12                        4752            250618 ns/op          141396 B/op       1752 allocs/op

Let me know if I am missing something or if I should be benchmarking with other factors.

[21KennethTran]

Originally moved audit emission (both fail and start events) to authorizeContext, but noticed multiple audit events for 1 user actions (logging in, performing actions on the app) in both success and failure cases.

Moving the logic to handleConnection seems to be a better choice since AppSessionStart events are emitted once alongside new session chunks every 5 minutes, but AppSessionStartFail events are emitted multiple times for the same action, so I created a map to deduplicate the same event.

I still need to test this, and am not sure if using a map is the right choice for handling this issue. Any suggestions on cleaning this up or a better place to insert audit events are appreciated.