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

Fix for DUO Universal Prompt authentication #426

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Conversation

SMxJrz
Copy link
Contributor

@SMxJrz SMxJrz commented Jan 29, 2024

Changing re.sub in line 147 of this file to look for ANY digit in front of the "v" part.

We ran into an issue in our environment because the original code here was looking for "v4" and our DUO setup was returning "v3", so this replacement never happened and resulted in a malformed URL leading to 403 responses from the DUO server.

Changing re.sub in line 147 of this file to look for ANY digit in front of the "v" part. We ran into an issue in our environment because the original code here was looking for v4 and our DUO setup was set to v3, so this replacement never happened and resulted in a malformed URL leading to 403 responses from the DUO server.

Signed-off-by: smxjrz <Smelecs@gmail.com>
@@ -145,7 +145,7 @@ def _perform_authentication_transaction(
ssl_verification_enabled,
):
duo_host = re.sub(
r"/frame/frameless/v4/auth.*",
r"/frame/frameless/v\d+/auth.*",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised it isn't needed to capture this version returned the Duo server, and reuse it in the constructed URLs sent to the same server afterward:

# rg -F 'v4' aws_adfs/_duo_universal_prompt_authenticator.py 
219:    status_for_url = duo_host + "/frame/v4/status"
246:    result_for_url = duo_host + "/frame/v4/oidc/exit"
267:    status_for_url = duo_host + "/frame/v4/status"
537:    duo_url = duo_host + "/frame/v4/prompt"
574:    prompt_for_url = duo_host + "/frame/v4/prompt"

Does everything really work fine with this single change?

Copy link
Contributor Author

@SMxJrz SMxJrz Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pdecat,

We've been using the tool against the DUO traditional prompt in our environment for a while with no issue and when our org made the switch over to the new Universal prompt, I had noticed that the latest version of the library started returning 403 when I tried to get credentials using the tool.

What I observed was that the duo_url before the regex replace call where my change is was making call out to "https://api-[API_ID].duosecurity.com/frame/frameless/v3/auth?sid=[[SID]]&tx=[[TX_ID]]/frame/v4/prompt"

The problem is, the goal of the regex wants to strip out the "frame/frameless/v3/auth?sid=[SID]&tx=[TX_ID]" part but the existing code is trying to match "/frame/frameless/v4/auth.*" and strip it out by replacing it with empty string, so you just have the URL to the DUO API.

The observation that I made, however, was that our DUO API was returning a different versioned /frame/frameless/v3/auth.* which led to the duo_url value in the having the frame/frameless/v3/auth?sid=[[SID]]&tx=[[TX_ID]] in the variable. which led to the following output

Error: Issues during beginning of the authentication process. The error response <Response [403]>

Which is due to the fact that the first call after this code (to /frame/v4/prompt) was not actually being called since the code was invoking "/frame/frameless/v3/auth?sid=[[SID]]&tx=[[TX_ID]]/frame/v4/prompt", where the "/frame/v4/prompt" part was part of tx in the query string of the URI.

Once I tweaked the regex to look for any digit the URI was stripped as expected from the DUO Url and we were able to get our credentials.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for the detailed explanation.

@pdecat pdecat merged commit b05c7eb into venth:master Jan 30, 2024
16 checks passed
@pdecat
Copy link
Collaborator

pdecat commented Feb 1, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants