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 #915

Closed
wants to merge 4 commits into from

Conversation

necouchman
Copy link
Contributor

I've taken a stab at updating the Duo MFA client to use the new API. I'm still in the process of getting my free account set up to test this out, so opening this in draft mode for the time being while I get it tested. A couple of notes for this PR:

@necouchman
Copy link
Contributor Author

Uh-oh, I suspect that I've over-looked something very important, here, in the authentication flow with this module. Since the Duo module is just doing the 2FA verification and not actual authentication, I suspect that this is never going to work as-implemented - that is, the DuoAuthenticationSessionManager is never going to get referenced, because it's going to want the initial authentication (username + password) on the redirect back.

@mike-jumper
Copy link
Contributor

I've extracted the SSOAuthenticationSession and related classes from the SSO base module and put those up in guacamole-ext so that I can re-use them in the Duo module. It was a reasonably clean move, I think?

Sounds good to me. Does this solve the issue you mention above regarding losing auth state in the redirect to/from Duo?

I've had to disable the Maven Enforcer plugin for the Duo module. See Upgrade OkHttp dependency to 4.3+ duosecurity/duo_universal_java#23 and Dependency version conflict duosecurity/duo_universal_java#18 for discussion and information on this.

We've had to fix similar issues before from other dependencies through explicitly choosing a version of the conflicting dependency and adding an override. For example:

<!-- Resolve version conflict (see below - transitive
dependencies of cas-client-core disagree on 2.3.0.1 vs.
2.3.1) -->
<exclusion>
<groupId>org.glassfish.jaxb</groupId>
<artifactId>jaxb-core</artifactId>
</exclusion>

<!-- Force use of version 2.3.1 (transitive dependencies of
cas-client-core disagree on 2.3.0. vs. 2.3.1) -->
<dependency>
<groupId>javax.xml.bind</groupId>
<artifactId>jaxb-api</artifactId>
<version>2.3.1</version>
</dependency>

Perhaps that would work here? I don't think we should disable Maven Enforcer for any of the modules, as it exists to prevent weird breakage from occurring when two versions of the same dependency get YOLO'd together in the same classpath, when something expects dependency version X but actually gets version Y, etc.

@necouchman
Copy link
Contributor Author

Does this solve the issue you mention above regarding losing auth state in the redirect to/from Duo?

I don't think it resolves it...but at the moment I also have an issue with the parameters at redirect ending up after the #, which means they probably aren't getting processed correctly. So I need to work that out, as well.

Perhaps that would work here? I don't think we should disable Maven Enforcer for any of the modules, as it exists to prevent weird breakage from occurring when two versions of the same dependency get YOLO'd together in the same classpath, when something expects dependency version X but actually gets version Y, etc.

Okay, I'll give that a shot. I agree that disabling it completely is probably not the right thing to do. Part of the issue, though, if you read the linked issues, is that the Duo team believes there may be issues with forcing a particular version - that is, I think they want both versions to be available to each of the modules that requires it, and they're hesitant to override it one way or the other.

@necouchman
Copy link
Contributor Author

Does this solve the issue you mention above regarding losing auth state in the redirect to/from Duo?

Yeah, this is definitely not solved. So, what currently happens is:

  • Open Guacamole page, get normal Username + Password prompt.
  • Redirect to Duo happens, complete Duo authentication.
  • Redirect back to Guacamole happens, but you get sent to the Username + Password prompt.
  • If you enter the same username and password, again, it attempts to resume the session with the state and duo_code. There's some other issue in this step that I need to figure out, but one thing at a time.

So, there's something about that redirect back from the Duo authentication to the Guacamole interface that isn't fully resuming the authentication session. My guess is that the AuthenticationSessionManager implementation may need to store more than just the Duo session? Unless there's something else I'm missing?

@necouchman
Copy link
Contributor Author

We've had to fix similar issues before from other dependencies through explicitly choosing a version of the conflicting dependency and adding an override.

I'm unable to get this to work for at least one of the dependencies:

Require upper bound dependencies error for com.squareup.okhttp3:okhttp:3.14.9 paths to dependency are:
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.squareup.okhttp3:okhttp:3.14.9
and
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.duosecurity:duo-universal-sdk:1.1.3
    +-com.squareup.retrofit2:retrofit:2.9.0
      +-com.squareup.okhttp3:okhttp:3.14.9
and
+-org.apache.guacamole:guacamole-auth-duo:1.5.3
  +-com.duosecurity:duo-universal-sdk:1.1.3
    +-com.squareup.okhttp3:logging-interceptor:4.9.1
      +-com.squareup.okhttp3:okhttp:4.9.1

It doesn't matter which version of okhttp I try to provide in the pom.xml file, it still fails with the issue trying to load the dependency.

Which is okay for the time being, because I still have to figure out how to handle resuming the entire authentication flow, as the Duo redirect is completely breaking that.

@necouchman
Copy link
Contributor Author

Thanks @aleitner for taking care of this with #966!

@necouchman necouchman closed this Apr 7, 2024
This pull request was closed.
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