Fix: destination cache vulnerablity #1770 (#1779)

* cherry pick + remove deprecated functions * missing changelog * fix tests * fix tests 2 Co-authored-by: d068544 <frank.essenberger@sap.com>
SAP · Nov 8, 2021 · 6f0fcb9 · 6f0fcb9
1 parent 478f1d9
commit 6f0fcb9
Show file tree

Hide file tree

Showing 8 changed files with 168 additions and 28 deletions.
diff --git a/CHANGELOG.md b/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,8 +26,8 @@
 
 ## 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.
+- [core] Use provider token to retrieve destinations from cache.
 
 # 1.51.0
 

diff --git a/packages/connectivity/src/scp-cf/cache.spec.ts b/packages/connectivity/src/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();
+  });
 });
diff --git a/packages/connectivity/src/scp-cf/cache.ts b/packages/connectivity/src/scp-cf/cache.ts
@@ -84,8 +84,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;
   }
@@ -96,11 +96,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 };
+    }
   }
 }
 

diff --git a/packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts b/packages/connectivity/src/scp-cf/destination/destination-cache.spec.ts
@@ -1,4 +1,5 @@
 import nock from 'nock';
+import { createLogger } from '@sap-cloud-sdk/util';
 import {
   providerJwtBearerToken,
   providerServiceToken,
@@ -173,12 +174,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 () => {
@@ -503,13 +506,13 @@ describe('caching destination integration tests', () => {
       destinationCache.cacheRetrievedDestination(
         decodeJwt(providerServiceToken),
         parsedDestination,
-        IsolationStrategy.User
+        IsolationStrategy.Tenant
       );
 
       const actual = await getDestination('ProviderDest', {
         userJwt: providerUserJwt,
         useCache: true,
-        isolationStrategy: IsolationStrategy.User,
+        isolationStrategy: IsolationStrategy.Tenant,
         selectionStrategy: alwaysProvider,
         cacheVerificationKeys: false,
         iasToXsuaaTokenExchange: false
@@ -558,6 +561,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(
@@ -591,6 +598,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' };
@@ -611,3 +684,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');
+
+    getDestinationCacheKey({ zid: 'tenant' }, 'dest');
+    expect(warn).toBeCalledWith(
+      'Cannot get cache key. Isolation strategy TenantUser is used, but tenant id or user id is undefined.'
+    );
+  });
+});
diff --git a/packages/connectivity/src/scp-cf/destination/destination-cache.ts b/packages/connectivity/src/scp-cf/destination/destination-cache.ts
@@ -1,9 +1,15 @@
+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>,
@@ -49,17 +55,42 @@ const DestinationCache = (cache: Cache<Destination>) => ({
 export function getDestinationCacheKey(
   decodedJwt: Record<string, any>,
   destinationName: string,
-  isolationStrategy: IsolationStrategy
-): 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_User:
-      return `${tenantId(decodedJwt)}:${userId(decodedJwt)}:${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:
-      return `:${userId(decodedJwt)}:${destinationName}`;
+      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:
-      return `${tenantId(decodedJwt)}::${destinationName}`;
+      logger.warn(
+        `Cannot get cache key. Isolation strategy ${isolationStrategy} is not supported.`
+      );
+      return;
   }
 }
 

diff --git a/packages/connectivity/src/scp-cf/destination/destination-from-service.ts b/packages/connectivity/src/scp-cf/destination/destination-from-service.ts
@@ -214,7 +214,7 @@ class DestinationFromServiceRetriever {
     readonly providerClientCredentialsToken: JwtPair
   ) {
     const defaultOptions = {
-      isolationStrategy: IsolationStrategy.Tenant,
+      isolationStrategy: IsolationStrategy.Tenant_User,
       selectionStrategy: subscriberFirst,
       useCache: false,
       ...options

diff --git a/packages/connectivity/src/scp-cf/destination/destination-service-cache.spec.ts b/packages/connectivity/src/scp-cf/destination/destination-service-cache.spec.ts
@@ -3,7 +3,8 @@ import { mockServiceBindings } from '../../../../core/test/test-util/environment
 import { mockServiceToken } from '../../../../core/test/test-util/token-accessor-mocks';
 import {
   providerServiceToken,
-  subscriberServiceToken
+  subscriberServiceToken,
+  subscriberUserJwt
 } from '../../../../core/test/test-util/mocked-access-tokens';
 import {
   mockSingleDestinationCall,
@@ -59,6 +60,13 @@ describe('DestinationServiceCache', () => {
       subscriberServiceToken,
       destinationServiceUrl
     );
+    mockSubaccountDestinationsCall(
+      nock,
+      [subscriberDest, subscriberDest2],
+      200,
+      subscriberUserJwt,
+      destinationServiceUrl
+    );
     mockSubaccountDestinationsCall(
       nock,
       [providerDest],
@@ -74,6 +82,14 @@ describe('DestinationServiceCache', () => {
       wrapJwtInHeader(subscriberServiceToken).headers,
       destinationServiceUrl
     );
+    mockSingleDestinationCall(
+      nock,
+      singleDest,
+      200,
+      singleDest.Name,
+      wrapJwtInHeader(subscriberUserJwt).headers,
+      destinationServiceUrl
+    );
   });
   afterEach(() => {
     destinationServiceCache.clear();
@@ -104,7 +120,7 @@ describe('DestinationServiceCache', () => {
     );
     expect(cache).toEqual(directCall);
     const cacheUndefined = getDestinationFromCache(
-      subscriberServiceToken,
+      subscriberUserJwt,
       singleDest.Name,
       IsolationStrategy.Tenant_User
     );
@@ -114,13 +130,13 @@ describe('DestinationServiceCache', () => {
   it('should cache single destination with tenant_user isolation.', async () => {
     const directCall = await fetchDestination(
       destinationServiceUrl,
-      subscriberServiceToken,
+      subscriberUserJwt,
       singleDest.Name,
       { useCache: true, isolationStrategy: IsolationStrategy.Tenant_User }
     );
 
     const cache = getDestinationFromCache(
-      subscriberServiceToken,
+      subscriberUserJwt,
       singleDest.Name,
       IsolationStrategy.Tenant_User
     );
@@ -162,12 +178,12 @@ describe('DestinationServiceCache', () => {
   it('should cache multiple destinations with tenant_user isolation.', async () => {
     const directCall = await fetchSubaccountDestinations(
       destinationServiceUrl,
-      subscriberServiceToken,
+      subscriberUserJwt,
       { useCache: true, isolationStrategy: IsolationStrategy.Tenant_User }
     );
 
     const cache = getDestinationsFromCache(
-      subscriberServiceToken,
+      subscriberUserJwt,
       IsolationStrategy.Tenant_User
     );
     expect(cache).toEqual(directCall);

diff --git a/packages/connectivity/src/scp-cf/destination/destination-service-cache.ts b/packages/connectivity/src/scp-cf/destination/destination-service-cache.ts
@@ -37,7 +37,7 @@ function getDestinationCacheKeyService(
   destinationServiceUri: string,
   decodedJwt: JwtPayload,
   isolationStrategy?: IsolationStrategy
-): string {
+): string | undefined {
   const usedIsolationStrategy =
     isolationStrategy === IsolationStrategy.Tenant ||
     isolationStrategy === IsolationStrategy.Tenant_User