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

GUACAMOLE-1289: Duo upgrade to API v4 #966

Merged
merged 7 commits into from
Apr 5, 2024

Conversation

aleitner
Copy link
Contributor

I continued working off @necouchman's PR #915 to add support for resuming authentication when dealing with the redirect authentication model

@necouchman
Copy link
Contributor

Nice @aleitner - thanks for picking this up and taking it over the line!

@stcbus
Copy link

stcbus commented Mar 29, 2024

@aleitner great news! We are very much looking forward to this, so for testing, do I need to pull from @necouchman 's PR as well or is just pulling from your branch good?

@necouchman
Copy link
Contributor

@stcbus Everything is in this PR - you don't need to pull both of them.

@aleitner aleitner force-pushed the GUACAMOLE-1289-Duo-4 branch 8 times, most recently from 863841b to 3562607 Compare April 3, 2024 05:37
@aleitner aleitner marked this pull request as ready for review April 3, 2024 05:52
@stcbus
Copy link

stcbus commented Apr 4, 2024

Not sure if this is the correct place to mention this or not, but using this branch works perfectly for me, except on the redirect to Duo, I just see a "LOGIN.INFO_DUO_REDIRECT_PENDING" text. I assume it is missing the translation text for that variable in translations?

@mike-jumper
Copy link
Contributor

Not sure if this is the correct place to mention this or not, but using this branch works perfectly for me, except on the redirect to Duo, I just see a "LOGIN.INFO_DUO_REDIRECT_PENDING" text. I assume it is missing the translation text for that variable in translations?

Yes, that string will need to be added here. I don't see any changes present that define that string.

If you still see that text after the string is added, you probably just need to clear browser cache.

@stcbus
Copy link

stcbus commented Apr 4, 2024

Also, just to note (I'm sure it's known!), the docker start.sh script needs updated with the new names of the DUO keys and to incorporate the new variables for redirect URL and timeout.

@aleitner
Copy link
Contributor Author

aleitner commented Apr 5, 2024

Added the missing translations and revised the guacamole.properties setup for duo

@stcbus
Copy link

stcbus commented Apr 5, 2024

Thanks @aleitner ! In the interest of accuracy, I believe the application key is no longer used with v4, so that part can be removed from docker/config as well. Please correct, if wrong (I certainly don't pass it in my testing).

@aleitner
Copy link
Contributor Author

aleitner commented Apr 5, 2024

Thanks @aleitner ! In the interest of accuracy, I believe the application key is no longer used with v4, so that part can be removed from docker/config as well. Please correct, if wrong (I certainly don't pass it in my testing).

Yup, it's no longer necessary so I just removed it. Thanks!

Copy link
Contributor

@jmuehlner jmuehlner left a comment

Choose a reason for hiding this comment

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

LGTM

@stcbus
Copy link

stcbus commented Apr 12, 2024

Let me know if I should report a bug now, but since it's not released quite yet, we just ran into 2 issues I think that maybe are things that need fixed for the extension:

  1. I am getting failures after successful logins, and that can trigger the ban extension now with lots of testing. I don't believe we had that before, so I think maybe somehow Duo extension doesn't report correctly to the ban extension?

  2. In a certain scenario, we get a generic guacamole error occured message. This is 100% reproducible in the following scenario:

  • Log into guac, pass through duo
  • after log in, log out
  • click "relogin" button

Re-loading the page is fine, so something with the relogin button or flow from there, we are rejected and get the below log message:

01:35:12.356 [http-nio-8080-exec-5] WARN o.a.g.event.EventLoggingListener - Authentication attempt from 128.146.93.48 for user "USERNAME failed: Duo Client error. (rejected by "duo") 01:35:12.356 [http-nio-8080-exec-5] ERROR o.a.g.rest.RESTExceptionMapper - Request could not be processed: Duo Client error.

For clarity, attached is the screen where this can be reproduced.

Screenshot 2024-04-11 at 6 42 56 PM

@stcbus
Copy link

stcbus commented Apr 12, 2024

From a quick troubleshooting session, it seems like the re-login button sends you back with the duo token stuff still in the URL field? Maybe that is why?

edit: Also confirmed, the duo token and other query parmeters are there right on log on as well. Positive this is it.

@aleitner
Copy link
Contributor Author

From a quick troubleshooting session, it seems like the re-login button sends you back with the duo token stuff still in the URL field? Maybe that is why?

edit: Also confirmed, the duo token and other query parmeters are there right on log on as well. Positive this is it.

Looking into a fix and I'll add it to my other PR #973

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.

5 participants