SAP / cloud-sdk-js

Use the SAP Cloud SDK for JavaScript / TypeScript to reduce development effort when building applications on SAP Business Technology Platform that communicate with SAP solutions and services such as SAP S/4HANA Cloud, SAP SuccessFactors, and many others.
Apache License 2.0
167 stars 57 forks source link

Expired destinations through connectivity proxy remain in cache #5131

Open carlonnheim opened 2 weeks ago

carlonnheim commented 2 weeks ago

Describe the bug Destinations which use Authorization and Proxy-Authorization only consider the expiry time of the Authorization token when determining cache expiry.

To Reproduce Use a destination which has for example OAuth2JWTBearer with a service going through the SAP Cloud Connector via the connectivity service. Configure it with useCache = true.

Make a request with one authenticated user. The framework obtains a Proxy-Authentication token to use with the connectivity service and the application token to use with the service being connected too. Both of these have an expiry time but only the application token is considered when determining the cache expiry. If the application token has a shorter expiry than the proxy token, the cached destination will start failing once the proxy token expires. Once the application token expires, the cache will be discarded and the next request will work again.

Make another request with a different user. The framework reuses the Proxy-Authentication token obtained during the first call, which can now have an arbitrary time-to-live left. A new application token for the other user is fetched. These are stored in the cache together and expires based on the expiry date of the application token. Once the proxy-auth expires, the destination fails until the application token eventually expires and the cache entry gets discarded.

Expected behavior The cached destination must expire based on the shortest lived token (application or connectivity proxy). Alternatively, the proxy-auth needs to be refreshed on cached destinations when it expires.

Screenshots We have worked around the issue with the following patch, which also further clarifies the issue. This includes excessive logging and is also not properly protecting for authorization tokens which are not jwt's (decodeJwt throws in that case), but I assume the proper solution to the problem might be different altogether anyway - this is just to clarify the issue.

diff --git a/node_modules/@sap-cloud-sdk/connectivity/dist/scp-cf/destination/destination-cache.js b/node_modules/@sap-cloud-sdk/connectivity/dist/scp-cf/destination/destination-cache.js
index 884f610..47972fc 100644
--- a/node_modules/@sap-cloud-sdk/connectivity/dist/scp-cf/destination/destination-cache.js
+++ b/node_modules/@sap-cloud-sdk/connectivity/dist/scp-cf/destination/destination-cache.js
@@ -105,9 +105,24 @@ async function cacheRetrievedDestination(token, destination, isolation, cache) {
     }
     const key = getDestinationCacheKey(token, destination.name, isolation);
     const expiresIn = (0, util_1.first)(destination.authTokens || [])?.expiresIn;
-    const expirationTime = expiresIn
+    let expirationTime = expiresIn
         ? Date.now() + parseInt(expiresIn) * 1000
         : undefined;
+
+    const proxyAuthExpirationTime = (0, jwt_1).decodeJwt(destination.proxyConfiguration?.headers?.['Proxy-Authorization'].match(/^bearer (.+)/i)?.[1])?.exp * 1000; // Check exipry of any Proxy-Authorization
+    const user_name = (0, jwt_1).decodeJwt((0, util_1.first)(destination.authTokens || [])?.value)?.user_name; // Temporary for trace logging
+    if (expirationTime && proxyAuthExpirationTime) {
+        if (proxyAuthExpirationTime < expirationTime) {
+            logger.info(`Destination cache ${key} (${user_name}) proxy-auth expires ${(new Date(proxyAuthExpirationTime)).toISOString()} before user-auth ${(new Date(expirationTime)).toISOString()}`);
+            expirationTime = proxyAuthExpirationTime - 2000; // Use a small expiration margin
+        } else {
+            logger.info(`Destination cache ${key} (${user_name}) proxy-auth expires ${(new Date(proxyAuthExpirationTime)).toISOString()} after user-auth ${(new Date(expirationTime)).toISOString()}`);
+        }
+    } else if (expirationTime) {
+        logger.info(`Destination cache ${key} (${user_name}) no proxy-auth, user-auth expries ${(new Date(expirationTime)).toISOString()}`);
+    } else {
+        logger.info(`Destination cache ${key} (${user_name}) no expiration time`);        
+    }
     cache.set(key, { entry: destination, expires: expirationTime });
 }
 /**

Used Versions:

Code Examples See patch above

Log file With the patch active (including its excessive logging), output looks like this (in this example both the proxy-auth and the application token have a 12h lifetime and the user connections are half a minute apart. The cached destination for User2 would have kept being used and fail the proxy-authentication after it has expired without the patch.

Destination cache xyz:SomeService (User1) proxy-auth expires 2024-10-30T11:16:29.000Z after user-auth 2024-10-30T11:16:27.107Z
Destination cache abc:SomeService (User2) proxy-auth expires 2024-10-30T11:16:29.000Z before user-auth 2024-10-30T11:16:54.761Z

Impact / Priority Can't use cached destinations including a user JWT and on-premise connectivity in combination.

Affected development phase: Production but can work around

Impact: Impaired but can work around

Timeline: N/A

Additional context N/A

deekshas8 commented 1 day ago

Hi @carlonnheim ,

Thanks for bringing this to our attention and also sharing your workaround. You're right, the proxy-auth token should be refreshed if it is expired before the cached destination is returned. I'll create a ticket in our backlog.

I cannot promise a timeline as we're currently low on capacity, but we will try to address it soon.