Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expired destinations through connectivity proxy remain in cache #5131

Open
carlonnheim opened this issue Oct 29, 2024 · 1 comment
Open

Expired destinations through connectivity proxy remain in cache #5131

carlonnheim opened this issue Oct 29, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@carlonnheim
Copy link

carlonnheim commented Oct 29, 2024

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:

  • node version v18.20.4
  • npm version 10.7.0
  • SAP Cloud SDK version 3.15.0 (also present in higher 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

@carlonnheim carlonnheim added the bug Something isn't working label Oct 29, 2024
@deekshas8
Copy link
Contributor

deekshas8 commented Nov 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants