epierce / cas-server-extension-duo

Duo Security Two-Factor Authentication for CAS
10 stars 4 forks source link

Problems when using Renew parameter #4

Open bafbl opened 9 years ago

bafbl commented 9 years ago

A problem has come up that seems related to a combination between the renew=true parameter and the duo extension.

User Behavior

Here is what we're seeing: (Fyi, All our duo-enforcement is done per-user right now) -Normal login flows work for both Duo-enforced and not-Duo-Enforced users -An app that uses the renew parameter to disable SSO works for Duo-enforced users but does not work for not-Duo-enforced users

Log & Protocol Behavior

Here is what happens when a non-duo-enforced user goes through the renew=true flow: -The user gets a username/password login panel, of course -The log reports TICKET_GRANTING_TICKET_CREATED, (two) SERVICE_TICKET_CREATED events and an SERVICE_TICKET_VALIDATED event for the second ST -The http response to the /cas/validate request says 'no'

Note: While we have one app that consistently experiences this, we may have other apps that use the Renew option that do not have this problem. We're still verifying this.

Code Behavior

When I've attached a debugger and traced the process, it looks like this is a problem with that second ST... it is created without access to the Credentials so it breaks the Renew check done during the validate.

Somehow this flow is different (probably First ST is returned to app?) for Duo-enforced users than not. Still looking into that, too.

Ideas for a solution

I don't understand spring web flow enough to see the exactly where the second ST is created nor how that the behavior would be different based on the second-factor prompt for duo-enforced people. This is probably the 'correct' way to address it.

Lacking the knowledge to address it in SWF, I'm tempted to change GenerateServiceTicketAction and look for a Credentials object in the flow context and use the grantServiceTicket(TGT, Service, Creds) method if the Credential object exists. This will make the second ST have fromNewLogin=true.

Also, I'm going to trace though the ST-validation process to see why SERVICE_TICKET_VALIDATED is logged when the validation actually fails.

bafbl commented 9 years ago

I've kludged a fix for this as I hypothesized in my initial report. Essentially, I added checking to see if valid credentials are in the WebFlow context and, if so, pushed them into the service-ticket-creating request.

I know that I should be reporting this as a GIT Pull or something, but I'm just seeking initial feedback.

Inside GenerateServiceTicketAction.java, I replaced

       final String serviceTicketId = this.centralAuthenticationService
           .grantServiceTicket(ticketGrantingTicket, service);

with

    // Did our WebFlow context include an actual login process that stored credentials
    final Credentials creds = (Credentials) context.getFlowScope().get("credentials");
    boolean useCredsForServiceTicket;

    // If we have creds in memory, then we should use them to build service ticket
    // that has fromNewLogin=true
  if ( creds == null )
          useCredsForServiceTicket = false;
  else
  {
          // Okay, we have credentials, but sometimes we have empty UsernamePassword Creds
          // so handle them specifically
          if ( creds instanceof UsernamePasswordCredentials )
          {
                  UsernamePasswordCredentials usernamePasswordCreds = (UsernamePasswordCredentials) creds;

                          if ( usernamePasswordCreds.getUsername() == null || usernamePasswordCreds.getPassword() == null )
                                  useCredsForServiceTicket = false;
                  else
                          useCredsForServiceTicket = true;
          }
          else
                  // Assume that non-username/password creds are always complete
                  // TODO: Can assumption be avoided? Perhaps use CredentialsToPrincipalResolver
                  useCredsForServiceTicket = true;
  }

   final String serviceTicketId;

   if ( useCredsForServiceTicket )
    {
          log.info("Creating service ticket with credentials: " + creds);
          serviceTicketId = this.centralAuthenticationService
                                                          .grantServiceTicket(ticketGrantingTicket,
                                                              service, creds);
     }
    else
    {
           log.info("Creating service ticket via SSO (without credentials)");
           serviceTicketId = this.centralAuthenticationService
                          .grantServiceTicket(ticketGrantingTicket,
                              service);
     }
epierce commented 9 years ago

I think your evaluation of the problem is correct, but I'm pretty sure the problem is in DetermineTwoFactorAction. We don't use renew=true on any of our services, so it's something I didn't even think to test. It shouldn't take too long to put a fix together - I may have something for you this weekend.

bafbl commented 9 years ago

Thank you for your response and offer to help.

In doing more testing, while my my kludge helps some cases, it is demonstrating its inelegance by interfering with ST creation after Duo auth. In essence, the credential that is now always passed into createServiceTicket for that first service is battling with its !equals( ) with the TGT in this code from CentralAuthenticationServiceImpl. Roughly speaking, the Credentials & TGT usernames are the same, but the equals includes attributes (LOA, etc) as well.

grantServiceTicket(...)
....

    if (credentials != null) {
        try {
            final Authentication authentication = this.authenticationManager
                .authenticate(credentials);
            final Authentication originalAuthentication = ticketGrantingTicket.getAuthentication();

            if (!(authentication.getPrincipal().equals(originalAuthentication.getPrincipal()) && authentication.getAttributes().equals(originalAuthentication.getAttributes()))) {
                throw new TicketCreationException();
            }
        } catch (final AuthenticationException e) {
            throw new TicketCreationException(e);
        }
    }

I'm going to follow your pointer and will post back here if I find anything.

Thanks again.

epierce commented 9 years ago

I think I figured out the problem. When you authenticate to a service with renew=true set, AuthenticationViaFormAction creates the service ticket and returns "warn"

SWF then sends the user to the determineIfTwoFactor decision point. Here's where I think the issue is. If it gets noMultiFactorNeeded back, then the user is sent to passwordPolicyCheck. However, that's a typo in login-webflow.xml. If you're not using LPPE, line 143 should be: <transition on="noMultiFactorNeeded" to="warn" />

I just made this change in the develop branch. Please try it out and let me know if that solves it for you.

Thanks, -Eric

bafbl commented 9 years ago

Thanks so much for your attention on this today. I see what you mean and how this should improve things.

...Early feedback... Unfortunately, my first attempt at patching our login-webflow.xml file led to an uncaught exception on normal logins. This doesn't make much sense to me, so I'm reverting our login-webflow to the development version you upgraded.

I'll post details when I know I have a sane webflow config.