Fix issue with broken authentication of users in REST module

src/python/WMCore/REST/Auth.py

@@ -41,23 +41,47 @@ def user_info_from_headers(key, verbose=False):
     # for HMAC validation while processing headers.
     prefix = suffix = ""
     hkeys = sorted(headers.keys())
+    suffixDict = {}
     for hk in hkeys:
+        # hk here represents CMS HTTP header, e.g. cms-authz-user
         hk = hk.lower()
         if hk[0:9] in ("cms-authn", "cms-authz") and hk != "cms-authn-hmac":
             prefix += "h%xv%x" % (len(hk), len(headers[hk]))
+            # old implementation of suffix does not account for proper ordering
             suffix += "%s%s" % (hk, headers[hk])
+            # keep all header key-vaules pair in dict and later sort values
+            suffixDict[hk] = headers[hk]
+            # hkname represents last element of header, e.g.
+            # if HTTP header is cms-authz-user then hkname would be user
             hkname = hk.split('-', 2)[-1]
             if hk.startswith("cms-authn"):
                 val = headers[hk]
                 if hk in ("cms-authn-name", "cms-authn-dn"):
                     val = str(val) if PY3 else str(val, "utf-8")
                 user[hkname] = val
             if hk.startswith("cms-authz"):
-                user['roles'][hkname] = {'site': set(), 'group': set()}
+                user['roles'][hkname] = {}
                 for r in headers[hk].split():
-                    site_or_group, name = r.split(':')
-                    user['roles'][hkname][site_or_group].add(name)
-
+                    # r hear represent HTTP header value, e.g. group:admin
+                    cricRoleKey, cricRoleValue = r.split(':')
+                    # check if cric_role_key exists in user's roles, if not create a set
+                    if cricRoleKey not in user['roles'][hkname].keys():
+                        user['roles'][hkname][cricRoleKey] = set()
+                    # add new value to a key set
+                    setValues = set(user['roles'][hkname][cricRoleKey])
+                    setValues.add(cricRoleValue)
+                    user['roles'][hkname][cricRoleKey] = set(sorted(setValues))
+
+    # take into account that groups can come in different order since they
+    # are supplied as dictionary, e.g.
+    # {... {'user': {'group': {'users', 'admin'}}}, ...}
+    # we should always persists the order of groups to properly calculate new checksum
+    skeys = sorted([h for h in suffixDict.keys()])
+    newSuffix = ""
+    for key in skeys:
+        vals = sorted(suffixDict[key].split(' '))
+        newSuffix += "%s%s" % (key, ' '.join(vals))
+    suffix = newSuffix
     # Check HMAC over authn/z headers with server key. If differs, reject.
     msg = prefix + "#" + suffix
     if PY3:
@@ -180,7 +204,6 @@ def __init__(self):
 
     def _setup(self):
         """Hook this tool into cherrypy request."""
-        log = cherrypy.log
         hooks = cherrypy.request.hooks
         conf = self._merged_args()
 

src/python/WMCore/REST/Test.py

@@ -47,11 +47,25 @@ def fake_authz_headers(hmac_key, method = 'HNLogin',
 
     prefix = suffix = ""
     hkeys = list(headers)
+    suffixDict = {}
     for hk in sorted(hkeys):
         if hk != 'cms-auth-status':
             prefix += "h%xv%x" % (len(hk), len(headers[hk]))
+            # old implementation of suffix does not account for proper ordering
             suffix += "%s%s" % (hk, headers[hk])
-
+            # keep all header key-vaules pair in dict and later sort values
+            suffixDict[hk] = headers[hk]
+
+    # take into account that groups can come in different order since they
+    # are supplied as dictionary, e.g.
+    # {... {'user': {'group': {'users', 'admin'}}}, ...}
+    # we should always persists the order of groups to properly calculate new checksum
+    skeys = sorted([h for h in suffixDict.keys()])
+    newSuffix = ""
+    for key in skeys:
+        vals = sorted(suffixDict[key].split(' '))
+        newSuffix += "%s%s" % (key, ' '.join(vals))
+    suffix = newSuffix
     msg = prefix + "#" + suffix
     if PY3:
         hmac_key = encodeUnicodeToBytes(hmac_key)

test/python/WMCore_t/MicroService_t/MSCore_t/MSAuth_t.py

@@ -29,9 +29,9 @@ def setUp(self):
         # REST/Auth.py module calculates checksum based on provided hmac
         # for instnace for self.hmac we will expect self.chkSum
         # with all CMS headers we setup below
-        # d357b299194ec4bed4e4fc73fc9ceab10139c16f vs. 4311773080ea12f4ea31a096da49de105083bb9e
-        self.hmac = 'd357b299194ec4bed4e4fc73fc9ceab10139c16f'
-        self.chkSum = '4311773080ea12f4ea31a096da49de105083bb9e'
+        # ERROR: authz hmac mismatch, 169d02b96265caf05894b526f99a22549dcd38ed vs. d357b299194ec4bed4e4fc73fc9ceab10139c16f
+        self.hmac = '169d02b96265caf05894b526f99a22549dcd38ed'
+        self.chkSum = 'd357b299194ec4bed4e4fc73fc9ceab10139c16f'
         self.fname = '/tmp/ms-authz.json'
 
         # for testing purposes we need to setup cherrypy headers

test/python/WMCore_t/REST_t/RestAuth_t.py

@@ -0,0 +1,83 @@
+"""
+Unit tests for REST/Auth.py module
+Author: Valentin Kuznetsov <vkuznet [AT] gmail [DOT] com>
+"""
+
+# system modules
+import logging
+import unittest
+
+# third party modules
+import cherrypy
+
+# WMCore modules
+from WMCore.REST.Auth import user_info_from_headers
+
+
+class RESTAuthTest(unittest.TestCase):
+    "Unit test for RESTAuth module"
+
+    def setUp(self):
+        """
+        setup necessary data for unit test usage
+        """
+        self.logger = logging.getLogger('rest_auth')
+
+        # REST/Auth.py module calculates checksum based on provided hmac
+        # for instnace for self.hmac we will expect self.chkSum
+        # with all CMS headers
+
+        # let's create set of different conditions
+        self.user_groups = []
+        self.hmacs = []
+        self.chkSums = []
+
+        # for group:users group:admin
+        user_groups = 'group:users group:admin'
+        hmac = '169d02b96265caf05894b526f99a22549dcd38ed'
+        chkSum = 'd357b299194ec4bed4e4fc73fc9ceab10139c16f'
+        self.user_groups.append(user_groups)
+        self.hmacs.append(hmac)
+        self.chkSums.append(chkSum)
+
+        # let's reverse order of user groups
+        user_groups = 'group:admin group:users'
+        self.user_groups.append(user_groups)
+        self.hmacs.append(hmac)
+        self.chkSums.append(chkSum)
+
+        # for group:admin group:users iam_group:test site:T1_XX_YYYY
+        user_groups = 'group:admin group:users iam_group:test site:T1_XX_YYYY'
+        hmac = '57ea0f58134aa079972da30a8fc2bf81853c949b'
+        chkSum = 'd357b299194ec4bed4e4fc73fc9ceab1013'
+        self.user_groups.append(user_groups)
+        self.hmacs.append(hmac)
+        self.chkSums.append(chkSum)
+
+    def testRESTAuth(self):
+        "test RESTAuth methods"
+        for idx in range(len(self.user_groups)):
+            user_groups = self.user_groups[idx]
+            hmac = self.hmacs[idx]
+            chkSum = self.chkSums[idx]
+
+            # for testing purposes we need to setup cherrypy headers
+            # cms-auth-status, cms-authn, cms-authz, cms-authn-hmac, cms-authn-name, cms-authn-dn
+            cherrypy.request.headers = {
+                'cms-auth-status': 'OK',
+                'cms-authn': 'fake',
+                'cms-authz-user': user_groups,
+                'cms-authn-hmac': hmac,
+                'cms-authn-name': 'test',
+                'cms-authn-dn': 'dn'}
+
+            authzKey = bytes(chkSum, 'UTF-8')
+            user = user_info_from_headers(key=authzKey)
+            self.logger.info(f"user_groups {user_groups}")
+            self.logger.info(f"hmac {hmac}")
+            self.logger.info(f"chkSum {chkSum}")
+            self.logger.info(f"user {user}")
+
+
+if __name__ == '__main__':
+    unittest.main()