Fix: destination cache vulnerablity (#1770)

CHANGELOG.md

@@ -14,7 +14,7 @@
 
 ## Compatibility Notes
 
--
+- [core] Switch the default isolation strategy from `IsolationStrategy.Tenant` to `IsolationStrategy.Tenant_User`, when setting `useCache` to true for destination lookup functions like `getDestination`.
 
 ## New Functionality
 
@@ -26,7 +26,7 @@
 
 ## Fixed Issues
 
--
+- [core] Disable destination cache, when the JWT does not contain necessary information. For example, when using `IsolationStrategy.Tenant_User`, the JWT has to contain both tenant id and user id.
 
 
 # 1.51.0

packages/core/src/connectivity/scp-cf/cache.spec.ts

@@ -86,4 +86,10 @@ describe('Cache', () => {
     expect(cacheTwo.get('expiredToken')).toBeUndefined();
     expect(cacheTwo.get('validToken')).toBe(dummyToken);
   });
+
+  it('should not hit cache for undefined key', () => {
+    cacheOne.set(undefined, {} as Destination);
+    const actual = cacheOne.get(undefined);
+    expect(actual).toBeUndefined();
+  });
 });

packages/core/src/connectivity/scp-cf/cache.ts

@@ -79,8 +79,8 @@ export class Cache<T> implements CacheInterface<T> {
    * @param key - The key of the entry to retrieve.
    * @returns The corresponding entry to the provided key if it is still valid, returns `undefined` otherwise.
    */
-  get(key: string): T | undefined {
-    return this.hasKey(key) && !isExpired(this.cache[key])
+  get(key: string | undefined): T | undefined {
+    return key && this.hasKey(key) && !isExpired(this.cache[key])
       ? this.cache[key].entry
       : undefined;
   }
@@ -91,11 +91,13 @@ export class Cache<T> implements CacheInterface<T> {
    * @param entry - The entry to cache
    * @param expirationTime - The time expressed in UTC in which the given entry expires
    */
-  set(key: string, entry: T, expirationTime?: number): void {
-    const expires = expirationTime
-      ? moment(expirationTime)
-      : inferExpirationTime(this.defaultValidityTime);
-    this.cache[key] = { entry, expires };
+  set(key: string | undefined, entry: T, expirationTime?: number): void {
+    if (key) {
+      const expires = expirationTime
+        ? moment(expirationTime)
+        : inferExpirationTime(this.defaultValidityTime);
+      this.cache[key] = { entry, expires };
+    }
   }
 }
 

packages/core/src/connectivity/scp-cf/destination/destination-cache.spec.ts

@@ -1,4 +1,5 @@
 import nock from 'nock';
+import { createLogger } from '@sap-cloud-sdk/util';
 import { IsolationStrategy } from '../cache';
 import { decodeJwt, wrapJwtInHeader } from '../jwt';
 import {
@@ -37,7 +38,10 @@ import {
   alwaysSubscriber,
   subscriberFirst
 } from './destination-selection-strategies';
-import { destinationCache } from './destination-cache';
+import {
+  destinationCache,
+  getDestinationCacheKeyStrict
+} from './destination-cache';
 import { AuthenticationType, Destination } from './destination-service-types';
 import { getDestinationFromDestinationService } from './destination-from-service';
 import { parseDestination } from './destination';
@@ -152,12 +156,14 @@ describe('caching destination integration tests', () => {
       const c3 = getSubscriberCache(IsolationStrategy.User);
       const c4 = getProviderCache(IsolationStrategy.No_Isolation);
       const c5 = getSubscriberCache(IsolationStrategy.Tenant_User);
+      const c6 = getProviderCache(IsolationStrategy.Tenant_User);
 
-      expect(c1!.url).toBe('https://subscriber.example');
+      expect(c1).toBeUndefined();
       expect(c2).toBeUndefined();
       expect(c3).toBeUndefined();
       expect(c4).toBeUndefined();
-      expect(c5).toBeUndefined();
+      expect(c5!.url).toBe('https://subscriber.example');
+      expect(c6).toBeUndefined();
     });
 
     it('retrieved  provider destinations are cached using only destination name in "NoIsolation" type', async () => {
@@ -513,6 +519,10 @@ describe('caching destination integration tests', () => {
 });
 
 describe('caching destination unit tests', () => {
+  beforeEach(() => {
+    destinationCache.clear();
+  });
+
   it('should cache the destination correctly', () => {
     const dummyJwt = { user_id: 'user', zid: 'tenant' };
     destinationCache.cacheRetrievedDestination(
@@ -546,6 +556,72 @@ describe('caching destination unit tests', () => {
     expect([actual1, actual2, actual3, actual4]).toEqual(expected);
   });
 
+  it('should not hit cache when Tenant_User is chosen but user id is missing', () => {
+    const dummyJwt = { zid: 'tenant' };
+    destinationCache.cacheRetrievedDestination(
+      dummyJwt,
+      destinationOne,
+      IsolationStrategy.Tenant_User
+    );
+    const actual1 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.User
+    );
+    const actual2 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.Tenant
+    );
+    const actual3 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.Tenant_User
+    );
+    const actual4 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.No_Isolation
+    );
+
+    const expected = [undefined, undefined, undefined, undefined];
+
+    expect([actual1, actual2, actual3, actual4]).toEqual(expected);
+  });
+
+  it('should not hit cache when Tenant is chosen but tenant id is missing', () => {
+    const dummyJwt = { user_id: 'user' };
+    destinationCache.cacheRetrievedDestination(
+      dummyJwt,
+      destinationOne,
+      IsolationStrategy.Tenant
+    );
+    const actual1 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.User
+    );
+    const actual2 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.Tenant
+    );
+    const actual3 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.Tenant_User
+    );
+    const actual4 = destinationCache.retrieveDestinationFromCache(
+      dummyJwt,
+      'destToCache1',
+      IsolationStrategy.No_Isolation
+    );
+
+    const expected = [undefined, undefined, undefined, undefined];
+
+    expect([actual1, actual2, actual3, actual4]).toEqual(expected);
+  });
+
   it('should return undefined when the destination is not valid', () => {
     jest.useFakeTimers('modern');
     const dummyJwt = { user_id: 'user', zid: 'tenant' };
@@ -566,3 +642,15 @@ describe('caching destination unit tests', () => {
     expect(actual).toBeUndefined();
   });
 });
+
+describe('get destination cache key', () => {
+  it('should shown warning, when Tenant_User is chosen, but user id is missing', () => {
+    const logger = createLogger('destination-cache');
+    const warn = jest.spyOn(logger, 'warn');
+
+    getDestinationCacheKeyStrict({ zid: 'tenant' }, 'dest');
+    expect(warn).toBeCalledWith(
+      'Cannot get cache key. Isolation strategy TenantUser is used, but tenant id or user id is undefined.'
+    );
+  });
+});

packages/core/src/connectivity/scp-cf/destination/destination-cache.ts

@@ -1,16 +1,22 @@
+import { createLogger } from '@sap-cloud-sdk/util';
 import { Cache, IsolationStrategy } from '../cache';
 import { tenantId } from '../tenant';
 import { userId } from '../user';
 import { Destination } from './destination-service-types';
 import { DestinationsByType } from './destination-accessor-types';
 
+const logger = createLogger({
+  package: 'core',
+  messageContext: 'destination-cache'
+});
+
 const DestinationCache = (cache: Cache<Destination>) => ({
   retrieveDestinationFromCache: (
     decodedJwt: Record<string, any>,
     name: string,
     isolation: IsolationStrategy
   ): Destination | undefined =>
-    cache.get(getDestinationCacheKey(decodedJwt, name, isolation)),
+    cache.get(getDestinationCacheKeyStrict(decodedJwt, name, isolation)),
   cacheRetrievedDestination: (
     decodedJwt: Record<string, any>,
     destination: Destination,
@@ -46,6 +52,59 @@ const DestinationCache = (cache: Cache<Destination>) => ({
  * @returns The cache key.
  * @hidden
  */
+export function getDestinationCacheKeyStrict(
+  decodedJwt: Record<string, any>,
+  destinationName: string,
+  isolationStrategy = IsolationStrategy.Tenant_User
+): string | undefined {
+  const tenant = tenantId(decodedJwt);
+  const user = userId(decodedJwt);
+  switch (isolationStrategy) {
+    case IsolationStrategy.No_Isolation:
+      return `::${destinationName}`;
+    case IsolationStrategy.Tenant:
+      if (tenant) {
+        return `${tenant}::${destinationName}`;
+      }
+      logger.warn(
+        `Cannot get cache key. Isolation strategy ${isolationStrategy} is used, but tenant id is undefined.`
+      );
+      return;
+    case IsolationStrategy.User:
+      if (user) {
+        return `:${user}:${destinationName}`;
+      }
+      logger.warn(
+        `Cannot get cache key. Isolation strategy ${isolationStrategy} is used, but user id is undefined.`
+      );
+      return;
+    case IsolationStrategy.Tenant_User:
+      if (tenant && user) {
+        return `${user}:${tenant}:${destinationName}`;
+      }
+      logger.warn(
+        `Cannot get cache key. Isolation strategy ${isolationStrategy} is used, but tenant id or user id is undefined.`
+      );
+      return;
+    default:
+      logger.warn(
+        `Cannot get cache key. Isolation strategy ${isolationStrategy} is not supported.`
+      );
+      return;
+  }
+}
+
+/**
+ * @deprecated Since v1.52.0. Use [[getDestinationCacheKeyStrict]] instead.
+ * Calculates a cache key based on the jwt and destination name for the given isolation strategy.
+ * Cache keys for strategies are non-overlapping, i.e. using a cache key for strategy [[IsolationStrategy.Tenant]]
+ * will not result in a cache hit for a destination that has been cached with strategy [[IsolationStrategy.Tenant_User]].
+ * @param decodedJwt - The decoded JWT of the current request.
+ * @param destinationName - The name of the destination.
+ * @param isolationStrategy - The strategy used to isolate cache entries.
+ * @returns The cache key.
+ * @hidden
+ */
 export function getDestinationCacheKey(
   decodedJwt: Record<string, any>,
   destinationName: string,
@@ -73,7 +132,11 @@ function cacheRetrievedDestination(
     throw new Error('The destination name is undefined.');
   }
 
-  const key = getDestinationCacheKey(decodedJwt, destination.name, isolation);
+  const key = getDestinationCacheKeyStrict(
+    decodedJwt,
+    destination.name,
+    isolation
+  );
   cache.set(key, destination);
 }
 

packages/core/src/connectivity/scp-cf/destination/destination-from-service.ts

@@ -205,7 +205,7 @@ class DestinationFromServiceRetriever {
     readonly providerClientCredentialsToken: JwtPair
   ) {
     const defaultOptions = {
-      isolationStrategy: IsolationStrategy.Tenant,
+      isolationStrategy: IsolationStrategy.Tenant_User,
       selectionStrategy: subscriberFirst,
       useCache: false,
       ...options

packages/core/src/connectivity/scp-cf/destination/destination-service-cache.ts

@@ -1,7 +1,7 @@
 import { JwtPayload } from 'jsonwebtoken';
 import { Cache, IsolationStrategy } from '../cache';
 import { Destination } from './destination-service-types';
-import { getDestinationCacheKey } from './destination-cache';
+import { getDestinationCacheKeyStrict } from './destination-cache';
 
 const DestinationServiceCache = (cache: Cache<Destination[]>) => ({
   retrieveDestinationsFromCache: (
@@ -37,14 +37,14 @@ function getDestinationCacheKeyService(
   destinationServiceUri: string,
   decodedJwt: JwtPayload,
   isolationStrategy?: IsolationStrategy
-): string {
+): string | undefined {
   const usedIsolationStrategy =
     isolationStrategy === IsolationStrategy.Tenant ||
     isolationStrategy === IsolationStrategy.Tenant_User
       ? isolationStrategy
       : IsolationStrategy.Tenant;
 
-  return getDestinationCacheKey(
+  return getDestinationCacheKeyStrict(
     decodedJwt,
     destinationServiceUri,
     usedIsolationStrategy