Disable API keys if used over insecure transport (#15457)

This commit adds a new property (secure_transport) to
ConnectionOrigin to indicate whether the websocket session
has secure transport. This flag is set in the following
scenarios:

1) AF_UNIX
2) localhost
3) https / wss
4) ha connection

New behavior based on security team feedback is introduced
in this commit such that API keys will be automatically
revoked when used over insecure transport. This is to prevent
them being used in replay attacks.

src/middlewared/middlewared/api/v25_04_0/auth.py

@@ -167,6 +167,7 @@ class AuthSessionEntry(BaseModel):
     ]
     credentials_data: BaseCredentialData | UserCredentialData | APIKeyCredentialData | TokenCredentialData
     created_at: datetime
+    secure_transport: bool
 
 
 class AuthTerminateSessionArgs(BaseModel):

src/middlewared/middlewared/plugins/api_key.py

@@ -359,6 +359,15 @@ async def authenticate(self, key: str) -> dict | None:
             'name': entry['name'],
         })
 
+    @private
+    async def revoke(self, key_id):
+        """ Revoke the specified API key in the DB, deactivate in the pam_tdb file, and
+        generate a middleware alert that it has been revoked. This is a private method
+        that is called when API key passed as plain-text over insecure transport."""
+        await self.middleware.call('datastore.update', self._config.datastore, key_id, {'expiry': -1})
+        await self.middleware.call('etc.generate', 'pam_middleware')
+        await self.check_status()
+
     @api_method(ApiKeyMyKeysArgs, ApiKeyMyKeysResult, roles=['READONLY_ADMIN', 'API_KEY_READ'])
     @pass_app(require=True)
     async def my_keys(self, app) -> list:

src/middlewared/middlewared/plugins/auth.py

@@ -181,6 +181,7 @@ def dump(self):
             "origin": str(self.app.origin),
             **dump_credentials(self.credentials),
             "created_at": utc_now() - timedelta(seconds=time.monotonic() - self.created_at),
+            "secure_transport": self.app.origin.secure_transport,
         }
 
 
@@ -771,6 +772,21 @@ async def login_ex(self, app, data):
                     resp['pam_response']['reason'] = 'Api key is expired.'
 
                 if resp['pam_response']['code'] == pam.PAM_SUCCESS:
+                    if not app.origin.secure_transport:
+                        # Per NEP if plain API key auth occurs over insecure transport
+                        # the key should be automatically revoked.
+                        await self.middleware.call('api_key.revoke', key_id)
+                        await self.middleware.log_audit_message(app, 'AUTHENTICATION', {
+                            'credentials': {
+                                'credentials': 'API_KEY',
+                                'credentials_data': {'username': data['username']},
+                            },
+                            'error': 'API key revoked due to insecure transport.'
+                        }, False)
+
+                        response['response_type'] = AuthResp.EXPIRED.name
+                        return response
+
                     if thehash.startswith('$pbkdf2-sha256'):
                         # Legacy API key with insufficient iterations. Since we
                         # know that the plain-text we have here is correct, we can

src/middlewared/middlewared/test/integration/utils/client.py

@@ -116,7 +116,7 @@ def client(self) -> Client:
             raise RuntimeError('IP is not set')
 
         uri = host_websocket_uri(addr)
-        cl = Client(uri, py_exceptions=True, log_py_exceptions=True)
+        cl = Client(uri, py_exceptions=True, log_py_exceptions=True, verify_ssl=False)
         try:
             resp = cl.call('auth.login_ex', {
                 'mechanism': 'PASSWORD_PLAIN',
@@ -160,13 +160,13 @@ def ha_ips(self) -> dict:
 
 
 @contextlib.contextmanager
-def client(*, auth=undefined, auth_required=True, py_exceptions=True, log_py_exceptions=True, host_ip=None):
+def client(*, auth=undefined, auth_required=True, py_exceptions=True, log_py_exceptions=True, host_ip=None, ssl=True):
     if auth is undefined:
         auth = ("root", password())
 
-    uri = host_websocket_uri(host_ip)
+    uri = host_websocket_uri(host_ip, ssl)
     try:
-        with Client(uri, py_exceptions=py_exceptions, log_py_exceptions=log_py_exceptions) as c:
+        with Client(uri, py_exceptions=py_exceptions, log_py_exceptions=log_py_exceptions, verify_ssl=False) as c:
             if auth is not None:
                 auth_req = {
                     "mechanism": "PASSWORD_PLAIN",
@@ -215,8 +215,9 @@ def host():
     return truenas_server
 
 
-def host_websocket_uri(host_ip=None):
-    return f"ws://{host_ip or host().ip}/api/current"
+def host_websocket_uri(host_ip=None, ssl=True):
+    prefix = 'wss://' if ssl else 'ws://'
+    return f"{prefix}{host_ip or host().ip}/api/current"
 
 
 def password():

src/middlewared/middlewared/utils/origin.py

@@ -1,4 +1,5 @@
 from dataclasses import dataclass
+from ipaddress import ip_address
 from socket import AF_INET, AF_INET6, AF_UNIX, SO_PEERCRED, SOL_SOCKET
 from struct import calcsize, unpack
 
@@ -42,6 +43,8 @@ class ConnectionOrigin:
     the login uid associated to the unix datagram connection.
     The login uid can be used to determine whether the connection was
     created by an interactive shell session"""
+    ssl: bool = False
+    """Nginx reports that https was used for connection"""
 
     @classmethod
     def create(cls, request):
@@ -55,16 +58,18 @@ def create(cls, request):
                     pid=pid,
                     uid=uid,
                     loginuid=login_uid,
-                    gid=gid
+                    gid=gid,
                 )
             elif sock.family in (AF_INET, AF_INET6):
-                la, lp, ra, rp = get_tcp_ip_info(sock, request)
+                la, lp, ra, rp, ssl = get_tcp_ip_info(sock, request)
+
                 return cls(
                     family=sock.family,
                     loc_addr=la,
                     loc_port=lp,
                     rem_addr=ra,
                     rem_port=rp,
+                    ssl=ssl
                 )
         except AttributeError:
             # request.transport can be None by the time this is
@@ -124,6 +129,25 @@ def is_ha_connection(self) -> bool:
             self.rem_addr and self.rem_addr in HA_HEARTBEAT_IPS
         )
 
+    @property
+    def secure_transport(self) -> bool:
+        """Indicates whether we should treat the connection as having
+        secure transport for purposes of invalidation of API keys.
+        Secure in this case means AF_UNIX, loopback connection, or
+        transport is encrypted."""
+
+        if self.ssl or self.is_unix_family or self.is_ha_connection:
+            return True
+
+        if self.is_tcp_ip_family:
+            try:
+                return ip_address(self.rem_addr).is_loopback
+            except Exception:
+                pass
+
+        # By default assume that transport is insecure
+        return False
+
 
 def get_tcp_ip_info(sock, request) -> tuple:
     # All API connections are terminated by nginx reverse
@@ -143,9 +167,11 @@ def get_tcp_ip_info(sock, request) -> tuple:
         # 0 (root) or 33 (www-data (nginx forks workers))
         ra = request.headers["X-Real-Remote-Addr"]
         rp = int(request.headers["X-Real-Remote-Port"])
+        ssl = request.headers.get("Origin", "").startswith("https:")
         check_uids = True
     except (KeyError, ValueError):
         ra, rp = sock.getpeername()
+        ssl = False
         check_uids = False
 
     with DiagSocket() as ds:
@@ -154,7 +180,7 @@ def get_tcp_ip_info(sock, request) -> tuple:
             if i['idiag_dst'] == ra and i['idiag_dport'] == rp:
                 if check_uids:
                     if i['idiag_uid'] in UIDS_TO_CHECK:
-                        return i['idiag_src'], i['idiag_sport'], i['idiag_dst'], i['idiag_dport']
+                        return i['idiag_src'], i['idiag_sport'], i['idiag_dst'], i['idiag_dport'], ssl
                 else:
-                    return i['idiag_src'], i['idiag_sport'], i['idiag_dst'], i['idiag_dport']
-    return (None, None, None, None)
+                    return i['idiag_src'], i['idiag_sport'], i['idiag_dst'], i['idiag_dport'], ssl
+    return (None, None, None, None, None)

tests/api2/test_api_key.py

@@ -256,3 +256,18 @@ def test_api_key_restrict_admin_other_keys_fail(sharing_admin_user):
             })
 
         assert ce.value.errno == errno.EACCES
+
+
+def test_api_key_revoke_insecure_transport(sharing_admin_user):
+    with api_key(sharing_admin_user.username) as key:
+        with client(auth=None, ssl=False) as c:
+            resp = c.call('auth.login_ex', {
+                'mechanism': 'API_KEY_PLAIN',
+                'username': sharing_admin_user.username,
+                'api_key': key
+            })
+            assert resp['response_type'] == 'EXPIRED'
+
+        # When the key is revoked due to use over insecure transport, it should
+        # automatically generate an alert that the key has been revoked.
+        assert check_revoked_alert()