/metadata and /spslo support?

[tboyko]

Does this gem break omniauth-saml functionality, such as the /metadata feature?

At a minimum it seems the route examples provided in the readme need to be more lenient, but it looks like there are issues beyond that. For instance, the request_path used in on_path? references a different (and perhaps default when not using omniauth-multi-provider) path and so the logic of omniauth-saml doesn't evaluate correctly here: https://github.com/omniauth/omniauth-saml/blob/946801990c58f0ccba07b91ecea07641af7e5b08/lib/omniauth/strategies/saml.rb#L105

Any guidance is appreciated.

[tboyko]

salsify/omniauth-multi-provider-saml#3

[jturkel]

It looks like the structure of the OmniAuth::Strategies::SAML#other_phase method changed with SLO support in https://github.com/omniauth/omniauth-saml/commit/cd3fc4344547c19d7f7061fe78da1f6af965b970 to just match a path prefix so single logout and metadata might work now. Try updating the routes to include the metadata/SLO routes and pass the appropriate metadata/SLO options to OmniAuth::MultiProvider.register (or fill them in dynamically the callback). That would be fantastic if this worked now!

[tboyko]

Tried it and it doesn't seem to work. Returns Not found. Authentication passthru. :/

[jturkel]

Bummer :(

Taking a quick look at OmniAuth::Strategies::SAML#other_phase it looks like there are potentially two problems:

1) The implementation of OmniAuth::Strategies::SAML#request_path which is called from the first line of other_phase caches the request path. This cache will need to invalidated after invoking setup_phase so the appropriate provider instance id is included in the request_path. 2) The default request_path before setup_phase is invoked will be "#{path_prefix}/#{name}" which won't jive with how omniauth-multi-provider is using the path_prefix because it includes the strategy name.

I'll have to give some thought on the best way to fix this.

[jturkel]

I think your best bet for a quick fix is to monkey patch OmniAuth::Strategies::SAML#other_phase to call setup_phase if the current_path matches an appropriate prefix and then call the original implementation. This will end up invoking setup_phase twice but I suspect that will be fine. If that causes issues, you can monkey patch that method to be a no-op if it's called more than once. I'm going to be offline for a little over a week but hopefully this will work. Good luck!

[tboyko]

Thanks for taking a stab at it. I'll post here if any progress is made.

[jturkel]

I've confirmed that a monkey patch like this does the trick:

module OmniAuthSamlPatch
  def on_auth_path?
    super || on_other_phase_path?
  end

  def on_other_phase_path?
    # TODO: Use appropriate path check
    current_path.start_with?('/users/auth/saml')
  end

  def other_phase
    setup_phase if on_other_phase_path?
    super
  end
end

OmniAuth::Strategies::SAML.prepend(OmniAuthSamlPatch)

[jturkel]

Here's a slightly more robust monkey patch to get this functionality:

# This patches omn-auth-saml to ensure setup_phase is called at the beginning of other_phase
# (which is consistent with how it handles request_phase and callback_phase).
module OmniAuthSamlOtherPhaseSetupPatch
  def on_auth_path?
    # Override this to ensure initialization happens properly in OmniAuth::Strategies::SAML for "other"
    # requests
    current_path.start_with?(options.path_prefix)
  end

  def other_phase
    # Override the other_phase method to call setup_phase before checking to see if the request
    # is on an "other" request path. This ensures omniauth-multi-provider has setup the path
    # prefix properly for the given identity provider. By default omniauth won't call setup_phase until
    # after checking the path.
    setup_phase
    super
  end

  def setup_phase
    # Make sure we only perform setup once since this method will be called twice during the other phase
    unless @setup
      super
      @setup = true
    end
  end
end

OmniAuth::Strategies::SAML.prepend(OmniAuthSamlOtherPhaseSetupPatch)

[tboyko]

In my environment, current_path is nil within def on_auth_path? and causes start_with? to error out. Typo, or something unique to me?

[jturkel]

@tboyko - What versions of omniauth and omniauth-saml are you using? How do you have omniauth configured?

[tboyko]

omniauth 1.4.2 omniauth-multi-provider 0.1.0 omniauth-saml 1.7.0

I run the patch before Rails.application.config.middleware.use OmniAuth::Builder do. I don't think my setup beyond that is terribly unique, at least in the context of already using multi provider.

[jturkel]

We're using omniauth 1.8.1. It might be worth upgrading to see if that fixes the issue.

[tboyko]

When loading a page of the web application in question, def on_auth_path? is called three times before the failure, which occurs on the third call. The first two calls have options.path_prefix set.

On the third call, and right after OmniAuth logs INFO -- omniauth: (saml) Setup endpoint detected, running now., options is still set to a KeyStore classed object, however path_prefix is not set. This causes start_with? to raise a TypeError: no implicit conversion of nil into String.

Does this seem to be an implementation-specific issue?

[jturkel]

I'm not sure but we're not hitting it in the Salsify application. If you can give me access to a reproducible test case (or at least your omniauth configuration), I'd be happy to take a look.

[gsar]

@jturkel your revised monkey patch doesn't appear to avoid calling setup_phase more than once. I tested this with omniauth 1.8.1 and omniauth-saml 1.10.0 by adding log statements to setup_phase within the unless block. The problem seems to be that the @setup instance variable is being set in the calling class instance, which gets created each time (at least in my setup) so it is always false. I think your earlier monkey patch is working better, so I used the following:

# monkey patch to support metadata paths - hacked version of:
#    https://github.com/salsify/omniauth-multi-provider/issues/4#issuecomment-366452170
#
# This patches omn-auth-saml to ensure setup_phase is called at the beginning of other_phase
# (which is consistent with how it handles request_phase and callback_phase).
module OmniAuthSamlOtherPhaseSetupPatch
  def on_auth_path?
    # Override this to ensure initialization happens properly in OmniAuth::Strategies::SAML for "other"
    # requests
    current_path.start_with?(options.path_prefix)
  end

  def on_other_path?
    # Override this to ensure initialization happens properly in OmniAuth::Strategies::SAML for "other"
    # requests
    current_path.match(%r{/(?:metadata|spslo|slo)\z})
  end

  def other_phase
    # Override the other_phase method to call setup_phase before checking to see if the request
    # is on an "other" request path. This ensures omniauth-multi-provider has setup the path
    # prefix properly for the given identity provider. By default omniauth won't call setup_phase until
    # after checking the path.
    setup_phase if on_auth_path? && on_other_path?
    super
  end

  def setup_phase
    # Make sure we only perform setup once since this method will be called twice during the other phase
    unless @setup  # TODO: always false due to the calling class being created anew each time?
      super
      @setup = true
    end
  end
end

OmniAuth::Strategies::SAML.prepend(OmniAuthSamlOtherPhaseSetupPatch)

[shelldweller]

I've been looking into enabling /spslo support. Here's what I found:

OmniAuth::Strategies::SAML is not able to match request path correctly in https://github.com/omniauth/omniauth-saml/blob/master/lib/omniauth/strategies/saml.rb#L75 and thus never initiates signed SAML logout request.

This is because OmniAuth::Strategy resolves the path to /auth/saml/saml (instead of auth/saml/actual-provider-name) in https://github.com/omniauth/omniauth/blob/25363559daf222b12e44111d48b6fe98052c2510/lib/omniauth/strategy.rb#L386.

This is because OmniAuth::MultiProvider::Handler sets request_path to something OmniAuth does not understand.

The solution would be to replace Method (see https://github.com/salsify/omniauth-multi-provider/blob/master/lib/omni_auth/multi_provider/handler.rb#L28) with the actual path https://github.com/salsify/omniauth-multi-provider/blob/master/lib/omni_auth/multi_provider/handler.rb#L57).

While this seems pretty straightforward, omniauth-multi-provider is not specific to SAML and I am not sure how this change might affect other strategies.

[joshIsCoding]

Hey @jturkel, thanks for all your work on this gem. Do you have any updates about implementing a more robust solution for the metadata and SLO paths in a future release, one based perhaps on @shelldweller's proposal above?

[joshIsCoding]

Update: (edited) the metadata itself, when accessed after implementing the monkey patch above, shows an incorrect assertion consumer service (callback) url, ending /(users/)auth/saml/saml/callback. This seems to be because @callback_path is already set before the set_up runs, meaning that the correct path set in options[:callback_path] is ignored. Amending the patch to set @callback_path = nil just before the setup_phase call in other_phase fixed that particular bug.

[erkie]

Updated the full monkey patch with @joshIsCoding suggestion (for ease of copy paste for future me/gem users)

# monkey patch to support metadata paths - hacked version of:
#    https://github.com/salsify/omniauth-multi-provider/issues/4#issuecomment-366452170
#
# This patches omn-auth-saml to ensure setup_phase is called at the beginning of other_phase
# (which is consistent with how it handles request_phase and callback_phase).
module OmniAuthSamlOtherPhaseSetupPatch
  def on_auth_path?
    # Override this to ensure initialization happens properly in OmniAuth::Strategies::SAML for "other"
    # requests
    current_path.start_with?(options.path_prefix)
  end

  def on_other_path?
    # Override this to ensure initialization happens properly in OmniAuth::Strategies::SAML for "other"
    # requests
    current_path.match(%r{/(?:metadata|spslo|slo)\z})
  end

  def other_phase
    # Override the other_phase method to call setup_phase before checking to see if the request
    # is on an "other" request path. This ensures omniauth-multi-provider has setup the path
    # prefix properly for the given identity provider. By default omniauth won't call setup_phase until
    # after checking the path.
    @callback_path = nil
    setup_phase if on_auth_path? && on_other_path?
    super
  end

  def setup_phase
    # Make sure we only perform setup once since this method will be called twice during the other phase
    unless @setup  # TODO: always false due to the calling class being created anew each time?
      super
      @setup = true
    end
  end
end

OmniAuth::Strategies::SAML.prepend(OmniAuthSamlOtherPhaseSetupPatch)

[jclusso]

I have this patch but I get the following response after signing out instead of actually redirecting and I can't figure out why. The page simply says "Redirecting to ...". The IdP is signed out though.

[jclusso]

I found that I had to set the slo_default_relay_state to where I wanted the user to be redirected. Honestly not sure where this is supposed to go, but I just set it to the sign in path.

[ulianadzoba]

Hello! I have implemented SLO for multi provider app using OmniAuthSamlOtherPhaseSetupPatch before middleware config. The problem is that after creating SLO logout request I get redirect loop. There is invalid signature error (idp certificate is added) in the system log. Am I missing something here? Thanks in advance for the help!

[igorkasyanchuk]

I have the same issue. @jclusso if you have a working example please share. I'm currently getting the error invalid signature (but login works).