Wctx parameter getting overridden, breaking functionality

[ischultz]

Issue The WsFederationHandler.HandleChallengeAsync method is completely overriding the existing Wctx parameter with the encrypted authentication properties. This completely breaks functionality that we currently utilize. That is, we can no longer use the Wctx parameter to send our own, custom contextual values to our federation server as these values are no longer sent in the clear. In our case, our federation server utilizes a few contextual values to alter the authentication behavior for the user if allowed. We do not need these contextual values to be encrypted and, unfortunately, the Wctx parameter is the only one guaranteed to be forwarded and returned per the protocol, so this is the only parameter we can utilize for this functionality.

Solution Instead of completely overriding the Wctx parameter, it would be better if the encrypted authentication properties were simply appended to the existing Wctx value instead of the value being completely overridden. Of course, the manner in which the encrypted data should be appended is ambiguous, so this should ultimately be an extension point whereby the current behavior can be overridden and handled differently if necessary.

[Tratcher]

There is an extensibility point at WsFederationOptions.Events.OnRedirectToIdentityProvider.

Copy and modify the code from here and then call redirectContext.HandleResponse() to prevent the normal code from running.

[Tratcher]

Hmm, you may need to do some juggling on the other end using AllowUnsolicitedLogins and OnMessageReceived to unpack the auth properties again. https://github.com/aspnet/Security/blob/26d27d871b7992022c082dc207e3d126e1d9d278/src/Microsoft.AspNetCore.Authentication.WsFederation/WsFederationHandler.cs#L170-L198

[ischultz]

Right, that's what I was planning on doing as a workaround. However, this felt like a bit of a hack, so I was hoping for a cleaner API to be introduced for this instead. In my opinion, our custom event-handling layer shouldn't necessarily be responsible for the encryption/decryption of the authentication properties and all that jazz, especially considering the code already exists in this library.

I'd like to propose, instead, that the encryption and decryption routines be placed into their own virtual functions, and that two additional virtual functions be added for the actual insertion and extraction (or storage and retrieval) of the encrypted payload to/from the request or some other storage location. This provides extensibility points to allow both the existing code to be reused and/or overridden if necessary.

Thoughts?

[ischultz]

Bah, ignore the proposal in my last post. I just realized that we don't really have a way of extending/replacing the WsFederationHandler class. Hrmm..

[Eilon]

@Tratcher - can you read up more on the WCTX field to see if we're not following specs well enough?

[ischultz]

I don't believe there's an issue with how you're using the wctx parameter with exception to perhaps the length of the information you're storing in it (i.e. the encrypted auth properties). The spec states the following:

wctx: This OPTIONAL parameter is an opaque context value that MUST be returned with the issued token if it is passed in the request. Note that this serves roughly the same purpose as the WS-Trust SOAP RST @Context attribute. In order not to exceed URI length limitations, the value of this parameter should be as small as possible.

The issue I've run into is that you're not allowing the wctx field to be used for any other purposes outside of this library. As mentioned above, we utilize this field to pass our own contextual federation values which must be sent in the clear, but code in this library is now prevents this from happening in effort to prevent XSRF attacks via unsolicited logins.

I was able to successfully update our code to work around this issue by essentially doing what @Tratcher recommended above. However, the code just doesn't feel right as I basically had to replicate code from this library (and short-circuit its invocation) in order to preserve its existing functionality.

For example, with AllowUnsolicitedLogins explicitly set to true, we can do the following:

public override async Task RedirectToIdentityProvider(RedirectContext context)
   ...
   federationContextBuilder[ENCRYPTED_PROPERTIES_KEY] = Uri.EscapeDataString(context.Options.StateDataFormat.Protect(context.Properties));
   context.ProtocolMessage.Wctx = federationContextBuilder.ToString();
   ...
   // short-circuit code in WsFederationHandler.RedirectToIdentityProvider and handle redirect ourselves
   context.Response.Redirect(context.ProtocolMessage.CreateSignInUrl());
   context.HandleResponse();
   return;

and

public override Task MessageReceived(MessageReceivedContext context)
{
   ...
   // note: this MessageReceived event will only be invoked if the AllowUnsolicitedLogins property is enabled
   var federationContext = context.ProtocolMessage.Wctx;
   if (federationContext.IsNotNullOrWhitespace())
   {
      var queryValues = federationContext.SafeParseAsQueryString();
      if (queryValues != null && queryValues.Count > 0)
      {
         var encryptedProperties = queryValues[ENCRYPTED_PROPERTIES_KEY];
         if (encryptedProperties.IsNotNullOrWhitespace())
         {
            encryptedProperties = Uri.UnescapeDataString(encryptedProperties);
            context.Properties = context.Options.StateDataFormat.Unprotect(encryptedProperties);
         }
      }
   }
   ...
}

[Tratcher]

we can no longer use the Wctx parameter to send our own, custom contextual values to our federation server

The spec says the wctx field is opaque and only intended for round trip app info, so why is the federation server reading information out of it? If you wanted to send extra data to the federation server why use this field as opposed to defining your own? Are you also relying on that info being round tripped?

[ischultz]

That's correct - we use the wctx parameter as that's the only one guaranteed to be round-tripped. But you're right, we should be utilizing custom query parameters for our needs instead. Our system has evolved from what it once was, so this is somewhat of a legacy issue at this point.

I'm going to close this issue since we're currently using the wctx parameter in an unorthodox manner and because a workaround does exist. This is likely not going to be a problem for anyone else.

Thank you for looking at this with me and apologies for absorbing your time with this!