[SECOAUTH-142] NoOpOAuthRememberMeServices breaks OAuth dance

[dsyer]

Priority: Minor
Original Assignee: Dave Syer
Reporter: Alex Rau
Created At: Fri, 21 Oct 2011 17:49:37 +0100
Last Updated on Jira: Mon, 24 Dec 2012 17:49:17 +0000

The default OAuth remember-me service being used is HttpSessionOAuthRememberMeServices. This implementation stores all OAuth tokens in the http session (triggered by OAuthConsumerContextFilter).

Avoiding this behavior (saving all tokens in the session) should be presumably possible with the NoOpOAuthRememberMeServices class. However the empty implementation does not even survive the OAuth dance which means using this class to turn off saving OAuth tokens in the session is no option. This is caused by the fact that NoOpOAuthRememberMeServices does not store request tokens during the dance which leads to a failure of the handshake.

A bare minimum (working) implementation of the NoOpOAuthRememberMeServices would be the following code which only stores request tokens (and no access tokens) in the session and does so only during the OAuth dance. As request tokens get replaced by access tokens by the caller (OAuthConsumerContextFilter) request tokens are implicitly removed after the dance from the set of tokens (except when the dance fails for other reasons).

[code]

package com.addresspush.service.utils;

import java.util.HashMap;
import java.util.Map;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;

import org.springframework.security.oauth.consumer.rememberme.OAuthRememberMeServices;
import org.springframework.security.oauth.consumer.token.OAuthConsumerToken;

/**

• Basic, no-op implementation of the remember-me services.
• @author Alexander Rau
  */
  public class OAuthDanceOnlyRemembermeService implements OAuthRememberMeServices {

public static final String REMEMBERED_TOKENS_KEY = OAuthDanceOnlyRemembermeService.class.getName()

• "#REMEMBERED_TOKENS";

@SuppressWarnings("unchecked")
public Map<String, OAuthConsumerToken> loadRememberedTokens(HttpServletRequest request, HttpServletResponse response) {

HttpSession session = request.getSession(false);

Map<String, OAuthConsumerToken> rememberedTokens = null;

if (session != null) { rememberedTokens = (Map<String, OAuthConsumerToken>) session.getAttribute(REMEMBERED_TOKENS_KEY); }

return rememberedTokens;
}

public void rememberTokens(Map<String, OAuthConsumerToken> tokens, HttpServletRequest request,
HttpServletResponse response) {

HttpSession session = request.getSession(false);

if (session == null) { return; }

Map<String, OAuthConsumerToken> requestTokensOnly = new HashMap<String, OAuthConsumerToken>();

for (Map.Entry<String, OAuthConsumerToken> token : tokens.entrySet()) { if (!token.getValue().isAccessToken()) requestTokensOnly.put(token.getKey(), token.getValue()); }

session.setAttribute(REMEMBERED_TOKENS_KEY, requestTokensOnly);

}

}
[code]

Comments:

david_syer on Fri, 5 Oct 2012 13:35:39 +0100

As I understand it a 2-legged system (consumer and provider) would not need any request tokens, so the existing NoOp* implementation is perfect since it doesn't store anything in the session at all. If that's correct then the proposal for a service that stores only request tokens is probably also valid, but as such is a new feature. Is that correct?

rauar on Mon, 8 Oct 2012 08:48:00 +0100

It's been a long time since I reported this issue - so I'm not completely back in the issue yet.

However from what I reported and another quick look into my current implementation the use-case leading to this issue is:

• I'm using 3-legged OAuth - I'm accessing multiple Google Contacts for an update on behalf of the current user
• this requires multiple OAuth dances (one per contact being accessed) for a single user action (1 browser request)... therefore the implementation switches access tokens dynamically
• all access tokens are stored in the DB and retrieved from there when required. So saving the access tokens in the HTTP session is a) not necessary and b) should be therefore avoided completely for security reasons (session hijacking etc).

For b) I first tried using NoOpOAuthRememberMeServices. This implementation however causes exceptions during any OAuth dance. The thing is that the OAuth dance always needs request tokens but NoOpOAuthRememberMeServices always returns null and the OAuth dance will always fail therefore. I'm not sure what the NoOpOAuthRememberMeServices could be used for to be honest.

The implementation I provided is based on the original HTTPRememberMeServices and stores request tokens in the session (for following the contract with the OAuth implementation) but does not store access tokens (which need to be treated securely).

I think a property/switch like "ignoreAccessTokens" in HttpSessionOAuthRememberMeServices for enabling/disabling saving access tokens would be already a solution for this issue (beside that I don't see any reason to keep NoOpOAuthRememberMeServices alive).

david_syer on Mon, 8 Oct 2012 10:39:55 +0100

OK, I don't quite understand your use case, but it makes sense to have an option that doesn't store access tokens, so I'll leave this as a feature request.

BTW I haven't tried it myself but I don't think returning nulls makes the NoOp* implementation useless, it just means that it can't be used for 3-legged flow. It would be great if someone would confirm that, but it's my working assumption and I haven't seen anything to contradict it yet.

rauar on Mon, 8 Oct 2012 10:51:41 +0100

Hi Dave,

do you think the NoOpOauthRememberMeService does work for some OAuth flows ? I just think that every OAuth dance (2- and 3-legged) requires (per spec) that request tokens must be provided to/available for the OAuth implementation during the dance and NoOp does never provide them. This contradiction always leads to OAuth dance failures (you can't have a successful dance with it) - that's why I think the NoOpOAuthRememberMeServices are a bit pointless.

But I'm fine with your proposal anyway

david_syer on Mon, 8 Oct 2012 11:44:48 +0100

Well, I'm happy to be proved wrong, but only a working sample app would convince me and I don't think sparklr-tonr has a 2-legged flow, so someone would need to add it. From what I recall looking at a related OAuth1 issue recently, the 2 -legged consumer doesn't have to send a request token and a default is provided by the framework in that case. Or maybe I misunderstood that.

rauar on Wed, 19 Dec 2012 23:29:51 +0000

Dave,

I spent some more time to get back into this issue. I hope I can clear up a few things

1. I think you are right that this affects 3-legged OAuth only (because 2-legged OAuth does not seem to have temporary request tokens). NoOpOAuthRememberMeServices loses request tokens during a dance which is why 3-legged OAuth will always fail with this implementation as rememberme service.

2. HttpSessionOAuthRememberMeServices does store requests tokens in the HTTP session and therefore the 3-legged dance can technically succeed. However it stores the final access token as well in the session. If required the application is most probably storing the access token anyway in a separate persistent token repository - which makes the access token in the HTTP session superfluous.

3. Request tokens are designed to be short lived and may be used only once - the provider should (must?) invalidate a request token after use. Based on the purpose and nature of request tokens it should be ok to temporarily store them in the HTTP session.

4. Access tokens are long-lived and may be used many times (therefore the common need to store it in a repository or store). Placing them in the HTTP session (and keeping them there longer than necessary) is in my opinion not a good idea from a security standpoint. Especially as it does not need to be stored there at all (only the request token needs to be there during the dance).

So what I would propose is:

a) add a switch/property to HttpSessionOAuthRememberMeServices which enables/disables storing access tokens in the HTTP session (disabling it would be the more secure setting)
b) keep NoOpOAuthRememberMeServices as it is for 2-legged OAuth

I think that should work with both Spring Security OAuth implementations (OAuth1 and OAuth2)

david_syer on Fri, 21 Dec 2012 06:44:16 +0000

Alex,

Thanks for the long analysis. The only thing I'm not clear on is how you would prevent the token being stored longer than necessary. If you have a proposal for how to do it then a pull request would be more than welcome.

I don't think it has anything to do with OAuth2 though. The OAuth2 side was heavily refactored away from the patterns still found in OAuth1 - it uses the session for storage too by default but only through the Spring SessionAttributeStore abstraction, and temporary data (auth codes in this case) are removed as soon as they are used.

rauar on Fri, 21 Dec 2012 18:15:44 +0000

Dave,

I would not try removing tokens at some point when they were used. I would just not store access tokens at all (that's what the code posted with this issue does and this could be done depending on a boolean property so that it's configurable). The access token is not required in the session and even if it would be required in some special case it could be turned on by the new property.

Regarding OAuth2: good to hear - I assumed the same mechanism would be used as the rememberme services implementations do not carry special OAuth1 or OAuth2 prefixes in their class names.

One more thing. I think an additional javadoc comment in NoOpOAuthRememberMeServices would be good which says it can't be used in 3-legged OAuth. The title of this issue needs probably a better name now as well.

Below is enhanced HttpSessionOAuthRememberMeServices from Ryan including the proposed access token filter:

package org.springframework.security.oauth.consumer.rememberme;

import java.util.HashMap;
import java.util.Map;

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;

import org.springframework.security.oauth.consumer.OAuthConsumerToken;
import org.springframework.security.oauth.consumer.rememberme.OAuthRememberMeServices;

/**
 * Default implementation of the OAuth2 rememberme services. Stores request tokens and optionally
 * access tokens.
 * 
 * @author Ryan Heaton
 * @author Alex Rau
 */
public class HttpSessionOAuthRememberMeServices implements OAuthRememberMeServices {

  public static final String REMEMBERED_TOKENS_KEY = HttpSessionOAuthRememberMeServices.class.getName()
          + "#REMEMBERED_TOKENS";

  private boolean storeAccessTokens = true;

  @SuppressWarnings("unchecked")
  public Map<String, OAuthConsumerToken> loadRememberedTokens(HttpServletRequest request, HttpServletResponse response) {

      HttpSession session = request.getSession(false);

      Map<String, OAuthConsumerToken> rememberedTokens = null;

      if (session != null) {
          rememberedTokens = (Map<String, OAuthConsumerToken>) session.getAttribute(REMEMBERED_TOKENS_KEY);
      }

      return rememberedTokens;
  }

  public void rememberTokens(Map<String, OAuthConsumerToken> tokens, HttpServletRequest request,
          HttpServletResponse response) {

      HttpSession session = request.getSession(false);

      if (session == null) {
          return;
      }

      Map<String, OAuthConsumerToken> requestTokensOnly = new HashMap<String, OAuthConsumerToken>();

      for (Map.Entry<String, OAuthConsumerToken> token : tokens.entrySet()) {
          if (!storeAccessTokens && !token.getValue().isAccessToken())
              requestTokensOnly.put(token.getKey(), token.getValue());

      }

      session.setAttribute(REMEMBERED_TOKENS_KEY, requestTokensOnly);

  }

  /**
   * Defines if access tokens should be stores in the HTTP session beside request tokens.
   * 
   * @param storeAccessTokens
   */
  public void setStoreAccessTokens(boolean storeAccessTokens) {
      this.storeAccessTokens = storeAccessTokens;
  }

}

david_syer on Sun, 23 Dec 2012 09:15:29 +0000

Great. Any chance of a pull request with a test case?

rauar on Mon, 24 Dec 2012 17:49:17 +0000

Pull request created: "Fix for SECOAUTH-142"