Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set app session ID in app session cert correctly #39971

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Mar 28, 2024

During my deep dive into App Access for Per-session MFA I discovered the app session ID on the actual app session certificate was set to a random UUID instead of the actual app session ID. Since the app session ID on the app session certs are not actually used for routing, unlike local app certs, this only affected audit events. You would see a random UUID instead of the app session ID.

Note: although this app session ID is a crypto-token and is used as a cookie in http requests, it is not a secret. It does not provide access unless it is paired with the app session bearer token.

@Joerger Joerger changed the title Removed unused random app session uuid in app session certs. Removed unused random app session uuid in app session certs Mar 28, 2024
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@github-actions github-actions bot requested review from strideynet and tcsc March 28, 2024 18:02
@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Mar 28, 2024
@zmb3
Copy link
Collaborator

zmb3 commented Mar 28, 2024

This confused the heck out of me when troubleshooting an earlier issue this year.

I'm not sure the fix is complete though. Should we remove the session ID from certRequest?

What about auditing? IIRC, the session ID that shows up in audit events is different than the session ID that shows up in the certs.

@Joerger Joerger force-pushed the joerger/remove-unused-app-session-uuid branch from 3a18042 to 72738f0 Compare March 28, 2024 23:55
@Joerger
Copy link
Contributor Author

Joerger commented Mar 29, 2024

I'm not sure the fix is complete though. Should we remove the session ID from certRequest?

We do still want to need RouteToApp.SessionID for local app certs. This is used by the Proxy to route local app certs (with the app session ID set to an actual app session ID) to a web session. The proxy then connects to that app using the web session key+cert and forwards the client connection.

What about auditing? IIRC, the session ID that shows up in audit events is different than the session ID that shows up in the certs.

Good point, I thought it was unreferenced but it is in fact used for audit events. Updated the PR to set the session ID correctly instead of not at all.

Note: I originally mentioned this could degrade security, as someone who gained access to the web session key and cert could use it to connect to the app through the proxy as if using local app certs. This was all quite theoretical, and since we are making app sessions secret it really shouldn't be exploitable.

@Joerger Joerger changed the title Removed unused random app session uuid in app session certs Set app session ID in app session cert correctly Mar 29, 2024
@Joerger Joerger requested a review from zmb3 March 29, 2024 00:02
@Joerger
Copy link
Contributor Author

Joerger commented Apr 15, 2024

@strideynet @tcsc friendly ping to review

@Joerger Joerger added this pull request to the merge queue Apr 16, 2024
Merged via the queue into master with commit bb7b9e9 Apr 16, 2024
36 checks passed
@Joerger Joerger deleted the joerger/remove-unused-app-session-uuid branch April 16, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants