Have Quay always use an OAuth-specific CSRF token

This change ensures that we always store and then check the contents of the OAuth `state` argument against a session-stored CSRF token.

Fixes https://www.pivotaltracker.com/story/show/135803615

endpoints/api/__init__.py

@@ -32,7 +32,7 @@ logger = logging.getLogger(__name__)
 api_bp = Blueprint('api', __name__)
 api = Api()
 api.init_app(api_bp)
-api.decorators = [csrf_protect,
+api.decorators = [csrf_protect(),
                   crossdomain(origin='*', headers=['Authorization', 'Content-Type']),
                   process_oauth, time_decorator(api_bp.name, metric_queue)]
 

endpoints/api/user.py

@@ -26,6 +26,7 @@ from endpoints.api import (ApiResource, nickname, resource, validate_json_reques
 from endpoints.exception import NotFound, InvalidToken
 from endpoints.api.subscribe import subscribe
 from endpoints.common import common_login
+from endpoints.csrf import generate_csrf_token, OAUTH_CSRF_TOKEN_NAME
 from endpoints.decorators import anon_allowed
 from util.useremails import (send_confirmation_email, send_recovery_email, send_change_email,
                              send_password_changed, send_org_recovery_email)
@@ -673,6 +674,15 @@ class Signout(ApiResource):
     return {'success': True}
 
 
+@resource('/v1/externaltoken')
+@internal_only
+class GenerateExternalToken(ApiResource):
+  """ Resource for generating a token for external login. """
+  @nickname('generateExternalLoginToken')
+  def post(self):
+    """ Generates a CSRF token explicitly for OIDC/OAuth-associated login. """
+    return {'token': generate_csrf_token(OAUTH_CSRF_TOKEN_NAME)}
+
 
 @resource('/v1/detachexternal/<servicename>')
 @show_if(features.DIRECT_LOGIN)

endpoints/csrf.py

@@ -12,31 +12,46 @@ from util.http import abort
 
 logger = logging.getLogger(__name__)
 
+OAUTH_CSRF_TOKEN_NAME = '_oauth_csrf_token'
+_QUAY_CSRF_TOKEN_NAME = '_csrf_token'
 
-def generate_csrf_token():
+def generate_csrf_token(session_token_name=_QUAY_CSRF_TOKEN_NAME):
-  if '_csrf_token' not in session:
+  """ If not present in the session, generates a new CSRF token with the given name
-    session['_csrf_token'] = base64.b64encode(os.urandom(48))
+      and places it into the session. Returns the generated token.
+  """
+  if session_token_name not in session:
+    session[session_token_name] = base64.b64encode(os.urandom(48))
 
-  return session['_csrf_token']
+  return session[session_token_name]
 
-def verify_csrf():
-  token = session.get('_csrf_token', None)
-  found_token = request.values.get('_csrf_token', None)
 
+def verify_csrf(session_token_name=_QUAY_CSRF_TOKEN_NAME,
+                request_token_name=_QUAY_CSRF_TOKEN_NAME):
+  """ Verifies that the CSRF token with the given name is found in the session and
+      that the matching token is found in the request args or values.
+  """
+  token = session.get(session_token_name, None)
+  found_token = request.values.get(request_token_name, None)
   if not token or token != found_token:
-    msg = 'CSRF Failure. Session token was %s and request token was %s'
+    msg = 'CSRF Failure. Session token (%s) was %s and request token (%s) was %s'
-    logger.error(msg, token, found_token)
+    logger.error(msg, session_token_name, token, request_token_name, found_token)
     abort(403, message='CSRF token was invalid or missing.')
 
-def csrf_protect(func):
-  @wraps(func)
-  def wrapper(*args, **kwargs):
-    oauth_token = get_validated_oauth_token()
-    if oauth_token is None and request.method != "GET" and request.method != "HEAD":
-      verify_csrf()
 
-    return func(*args, **kwargs)
+def csrf_protect(session_token_name=_QUAY_CSRF_TOKEN_NAME,
-  return wrapper
+                 request_token_name=_QUAY_CSRF_TOKEN_NAME,
+                 all_methods=False):
+  def inner(func):
+    @wraps(func)
+    def wrapper(*args, **kwargs):
+      oauth_token = get_validated_oauth_token()
+      if oauth_token is None:
+        if all_methods or (request.method != "GET" and request.method != "HEAD"):
+          verify_csrf(session_token_name, request_token_name)
+
+      return func(*args, **kwargs)
+    return wrapper
+  return inner
 
 
 app.jinja_env.globals['csrf_token'] = generate_csrf_token

endpoints/oauthlogin.py

@@ -12,6 +12,7 @@ from auth.process import require_session_login
 from data import model
 from endpoints.common import common_login, route_show_if
 from endpoints.web import index
+from endpoints.csrf import csrf_protect, OAUTH_CSRF_TOKEN_NAME
 from util.security.jwtutil import decode, InvalidTokenError
 from util.validation import generate_valid_usernames
 
@@ -19,6 +20,7 @@ logger = logging.getLogger(__name__)
 client = app.config['HTTPCLIENT']
 oauthlogin = Blueprint('oauthlogin', __name__)
 
+oauthlogin_csrf_protect = csrf_protect(OAUTH_CSRF_TOKEN_NAME, 'state', all_methods=True)
 
 def render_ologin_error(service_name, error_message=None, register_redirect=False):
   user_creation = bool(features.USER_CREATION and features.DIRECT_LOGIN)
@@ -32,6 +34,7 @@ def render_ologin_error(service_name, error_message=None, register_redirect=Fals
   }
   return index('', error_info=error_info)
 
+
 def get_user(service, token):
   token_param = {
     'access_token': token,
@@ -44,7 +47,7 @@ def get_user(service, token):
   return got_user.json()
 
 
-def conduct_oauth_login(service, user_id, username, email, metadata={}):
+def conduct_oauth_login(service, user_id, username, email, metadata=None):
   service_name = service.service_name()
   to_login = model.user.verify_federated_login(service_name.lower(), user_id)
   if not to_login:
@@ -66,17 +69,12 @@ def conduct_oauth_login(service, user_id, username, email, metadata={}):
       prompts = model.user.get_default_user_prompts(features)
       to_login = model.user.create_federated_user(new_username, email, service_name.lower(),
                                                   user_id, set_password_notification=True,
-                                                  metadata=metadata,
+                                                  metadata=metadata or {},
                                                   prompts=prompts)
 
       # Success, tell analytics
       analytics.track(to_login.username, 'register', {'service': service_name.lower()})
 
-      state = request.args.get('state', None)
-      if state:
-        logger.debug('Aliasing with state: %s', state)
-        analytics.alias(to_login.username, state)
-
     except model.InvalidEmailAddressException:
       message = "The e-mail address %s is already associated " % (email, )
       message = message + "with an existing %s account." % (app.config['REGISTRY_TITLE_SHORT'], )
@@ -96,6 +94,7 @@ def conduct_oauth_login(service, user_id, username, email, metadata={}):
 
   return render_ologin_error(service_name)
 
+
 def get_email_username(user_data):
   username = user_data['email']
   at = username.find('@')
@@ -107,6 +106,7 @@ def get_email_username(user_data):
 
 @oauthlogin.route('/google/callback', methods=['GET'])
 @route_show_if(features.GOOGLE_LOGIN)
+@oauthlogin_csrf_protect
 def google_oauth_callback():
   error = request.args.get('error', None)
   if error:
@@ -136,6 +136,7 @@ def google_oauth_callback():
 
 @oauthlogin.route('/github/callback', methods=['GET'])
 @route_show_if(features.GITHUB_LOGIN)
+@oauthlogin_csrf_protect
 def github_oauth_callback():
   error = request.args.get('error', None)
   if error:
@@ -199,6 +200,7 @@ def github_oauth_callback():
 @oauthlogin.route('/google/callback/attach', methods=['GET'])
 @route_show_if(features.GOOGLE_LOGIN)
 @require_session_login
+@oauthlogin_csrf_protect
 def google_oauth_attach():
   code = request.args.get('code')
   token = google_login.exchange_code_for_token(app.config, client, code,
@@ -236,6 +238,7 @@ def google_oauth_attach():
 @oauthlogin.route('/github/callback/attach', methods=['GET'])
 @route_show_if(features.GITHUB_LOGIN)
 @require_session_login
+@oauthlogin_csrf_protect
 def github_oauth_attach():
   code = request.args.get('code')
   token = github_login.exchange_code_for_token(app.config, client, code)
@@ -276,6 +279,7 @@ def decode_user_jwt(token, oidc_provider):
 
 @oauthlogin.route('/dex/callback', methods=['GET', 'POST'])
 @route_show_if(features.DEX_LOGIN)
+@oauthlogin_csrf_protect
 def dex_oauth_callback():
   error = request.values.get('error', None)
   if error:
@@ -318,6 +322,7 @@ def dex_oauth_callback():
 @oauthlogin.route('/dex/callback/attach', methods=['GET', 'POST'])
 @route_show_if(features.DEX_LOGIN)
 @require_session_login
+@oauthlogin_csrf_protect
 def dex_oauth_attach():
   code = request.args.get('code')
   token = dex_login.exchange_code_for_token(app.config, client, code, redirect_suffix='/attach',

endpoints/web.py

@@ -494,7 +494,7 @@ def oauth_local_handler():
 
 
 @web.route('/oauth/denyapp', methods=['POST'])
-@csrf_protect
+@csrf_protect()
 def deny_application():
   if not current_user.is_authenticated:
     abort(401)

static/js/app.js

@@ -215,7 +215,8 @@ quayApp.config(['$routeProvider', '$locationProvider', 'pages', function($routeP
     // 404/403
     .route('/:catchall', 'error-view')
     .route('/:catch/:all', 'error-view')
-    .route('/:catch/:all/:things', 'error-view');
+    .route('/:catch/:all/:things', 'error-view')
+    .route('/:catch/:all/:things/:here', 'error-view');
 }]);
 
 // Configure compile provider to add additional URL prefixes to the sanitization list. We use

static/js/directives/ui/external-login-button.js

@@ -21,19 +21,21 @@ angular.module('quay').directive('externalLoginButton', function () {
 
       $scope.startSignin = function() {
         $scope.signInStarted({'service': $scope.provider});
+        ApiService.generateExternalLoginToken().then(function(data) {
+          var url = ExternalLoginService.getLoginUrl($scope.provider, $scope.action || 'login');
+          url = url + '&state=' + encodeURIComponent(data['token']);
 
-        var url = ExternalLoginService.getLoginUrl($scope.provider, $scope.action || 'login');
+          // Save the redirect URL in a cookie so that we can redirect back after the service returns to us.
+          var redirectURL = $scope.redirectUrl || window.location.toString();
+          CookieService.putPermanent('quay.redirectAfterLoad', redirectURL);
 
-        // Save the redirect URL in a cookie so that we can redirect back after the service returns to us.
+          // Needed to ensure that UI work done by the started callback is finished before the location
-        var redirectURL = $scope.redirectUrl || window.location.toString();
+          // changes.
-        CookieService.putPermanent('quay.redirectAfterLoad', redirectURL);
+          $scope.signingIn = true;
-
+          $timeout(function() {
-        // Needed to ensure that UI work done by the started callback is finished before the location
+            document.location = url;
-        // changes.
+          }, 250);
-        $scope.signingIn = true;
+        }, ApiService.errorDisplay('Could not perform sign in'));
-        $timeout(function() {
-          document.location = url;
-        }, 250);
       };
     }
   };

static/js/services/external-login-service.js

@@ -9,14 +9,6 @@ angular.module('quay').factory('ExternalLoginService', ['KeyService', 'Features'
     var serviceInfo = externalLoginService.getProvider(service);
     if (!serviceInfo) { return ''; }
 
-    var stateClause = '';
-
-    if (Config.MIXPANEL_KEY && window.mixpanel) {
-      if (mixpanel.get_distinct_id !== undefined) {
-        stateClause = "&state=" + encodeURIComponent(mixpanel.get_distinct_id());
-      }
-    }
-
     var loginUrl = KeyService.getConfiguration(serviceInfo.key, 'AUTHORIZE_ENDPOINT');
     var clientId = KeyService.getConfiguration(serviceInfo.key, 'CLIENT_ID');
 
@@ -28,8 +20,7 @@ angular.module('quay').factory('ExternalLoginService', ['KeyService', 'Features'
     }
 
     var url = loginUrl + 'client_id=' + clientId + '&scope=' + scope + '&redirect_uri=' +
-              redirectUri + stateClause;
+              redirectUri;
-
     return url;
   };