Adds support for hidden private keys in server tenants #23379

dhr-verma · 2024-12-19T19:58:30Z

Description

This PR adds support for storing private keys in the tenants DB collection. These private keys will be used to sign tokens that are acquired for keyless access. These are never shared with the client and will be rotated based on their rotation schedule. It also adds a new claim in the fluid access token - fluidRelayKeylessAccess. This claim will be used to determine how to validate the access token - with the shared keys or the private keys. This allows to keep access tokens to be backward compatible while also supporting keyless access. It does introduce some new APIs and modifies existing APIs:

/tenants/:id/keys: Adds a new query parameter - getPrivateKeys. When set to true, it returns the privateKeys for the tenant. By default, this flag is set to false.
/tenants/:id/keylessAccess: Toggles the enableKeylessAccess flag for a tenant.
/tenants/:id/key: Refreshes the requested tenant key. When refreshPrivateKey is true it will refresh the private keys but does not return the refreshed keys. This is because key refreshes of the private keys will not be exposed to clients.
/tenants/:id?: Creates a fluid tenant. This request accepts a new parameter in the body - enableKeylessAccess. When this is set to true, a tenant with support for private keys will be created. By default, this is false.

To support these, the following changes are done:

Updated Riddler APIs to support a new interface ITenantPrivateKeys. The workflows essentially are the same as before but they include some new flags which helps riddler determine what keys to get/refresh/use for validation. The changes also include creating different caching keys for shared and private keys.
Added a new claim fluidRelayKeylessAccess which will be set to true when generating a keyless access token.
To test and ensure backward compatibility, I also created a test suite for tenantManager.ts. This ensures the existing APIs work as before. It also tests the new functionality. To further test these changes, I ran the server locally using Docker.

Breaking Changes

This adds a bunch of functionalities to support private hidden keys. However, these should not break existing changes.

Reviewer Guidance

General design of reusing the existing design of getTenantKeys rather than creating new interfaces.
Unit test clarity.

…to vermadhr/keylessAccessWork

server/routerlicious/packages/routerlicious-base/src/riddler/api.ts

manishgargd · 2024-12-19T20:54:25Z

server/routerlicious/packages/routerlicious-base/src/riddler/api.ts

@@ -142,11 +161,15 @@ export function create(
 		const tenantCustomData: ITenantCustomData = request.body.customData
 			? request.body.customData
 			: {};
+		const enableKeylessAccess = request.body.enableKeylessAccess
+			? request.body.enableKeylessAccess
+			: null;
 		const tenantP = manager.createTenant(


This method should also have "enableKeyAcess". Both "enableKeylessAcess" and "enableKeyAcess" can be true, either of them can be true. If both of them are false, then it is an error condition.

I do not think we need another flag - enableKeyAccess. Access via keys will continue to work even if enableKeylessAccess is set to true.

server/routerlicious/packages/routerlicious-base/src/riddler/api.ts

server/routerlicious/packages/services-utils/src/auth.ts

manishgargd · 2024-12-19T21:14:31Z

server/routerlicious/packages/routerlicious-base/src/riddler/api.ts

+		const enableKeylessAccess = request.body.enableKeylessAccess
+			? request.body.enableKeylessAccess
+			: null;
+		const storageP = manager.updateKeylessAccessPolicy(tenantId, enableKeylessAccess);


We should be able to update both enableKeylessAccess and enableKeyAccess. Ensure that both cannot be set to false.

Ahh okay got it. So in case both are true, then we will have (key, secondaryKey) and privateKeys present for the tenant. In case only enableKeyAccess is true, then we would only have (key, secondaryKey) present. In case only enableKeylessAccess is true, then we would only have privateKeys present for the tenant. Is my understanding correct?

manishgargd · 2024-12-19T21:17:35Z

server/routerlicious/packages/routerlicious-base/src/riddler/api.ts

@@ -128,7 +146,8 @@ export function create(
 	router.put("/tenants/:id/key", (request, response) => {
 		const tenantId = request.params.id;
 		const keyName = request.body.keyName as string;
-		const refreshKeyP = manager.refreshTenantKey(tenantId, keyName);
+		const refreshPrivateKey = request.body.refreshPrivateKey as boolean;


Ensure that the tenant cannot send us a request to "refreshPrivateKey".
"refreshPrivateKey" should be exposed only to internal APIs and not exposed to the tenant.

Yes, the /tenants/:id/key API with refreshPrivateKey as true would only be called by internal APIs that will refresh the private tenant keys periodically.

manishgargd · 2024-12-19T21:30:57Z

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts

@@ -57,6 +60,9 @@ export interface ITenantDocument {
 	// Timestamp of when this tenant will be hard deleted.
 	// Only applicable if the tenant is disabled.
 	scheduledDeletionTime?: string;
+
+	// Optional private keys, used for keyless tenants


Even (key, secondaryKey) can be optional. However, both (key, secondaryKey) and ITenantPrivateKeys cannot be optional. Only one can be at a given time.

manishgargd · 2024-12-19T21:51:26Z

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts

@@ -261,6 +278,7 @@ export class TenantManager {
 			storage: tenant.storage,
 			customData: tenant.customData,
 			scheduledDeletionTime: tenant.scheduledDeletionTime,
+			enableKeylessAccess: tenant.privateKeys ? true : false,


I think it should have another property "enableKeyAccess".

manishgargd · 2024-12-19T21:52:27Z

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts

@@ -276,9 +294,55 @@ export class TenantManager {
 			storage: tenant.storage,
 			customData: tenant.customData,
 			scheduledDeletionTime: tenant.scheduledDeletionTime,
+			enableKeylessAccess: tenant.privateKeys ? true : false,


This should also have the property "enableKeyAccess"

manishgargd · 2024-12-19T23:48:04Z

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts

@@ -287,6 +351,7 @@ export class TenantManager {
 		storage: ITenantStorage,
 		orderer: ITenantOrderer,
 		customData: ITenantCustomData,
+		enableKeylessAccess = false,
 	): Promise<ITenantConfig & { key: string }> {
 		const latestKeyVersion = this.secretManager.getLatestKeyVersion();



The function above could be called "createTenantKeys" and it could be called twice. Once for non-private keys and once for private keys. Lines 358 to 376 would not be required.

manishgargd · 2024-12-20T00:36:16Z

server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts

+		const latestKeyVersion = this.secretManager.getLatestKeyVersion();
+		let privateKeys: ITenantPrivateKeys | undefined;
+		// If the tenant is being converted to a keyless tenant, generate new private keys, otherwise update them to be undefined
+		if (enableKeylessAccess) {


If enableKeylessAccess is false, should we remove it? Check Azure Store or Redis if they allow to reset keylessAcceess, once it has been enabled.

If enableKeylessAccess is true, and it was already true in tenantMetadata, should we just ignore and return success for this operation.

dhr-verma added 16 commits December 17, 2024 09:40

Added support for hidden private keys in Riddler

bd5c693

Added key ordering logic

76325b6

Added unit tests

518b48d

Combined the getKey methods

abca9ee

Fixed bug in sinon

b97ae1b

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

39810d2

…to vermadhr/keylessAccessWork

Fixed unit test bug

a4b24eb

Added more unit tests

ab12637

Added more unit tests for TenantManager

604d986

FIxed format

cf3bcd9

Fixed formatting

478b5fb

Added new prop to ITenantConfig

52cbe69

Lint fixes

367c841

Fixed bugs

053741c

Changed property name from isKeylessAccessEnabled to enableKeylessAccess

d6b4c3c

Added documentation

2332f10

Copilot bot review requested due to automatic review settings December 19, 2024 19:58

github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Dec 19, 2024

dhr-verma requested review from znewton, yunho-microsoft, pradeepvairamani, manishgargd, zhangxin511 and zhajie and removed request for Copilot December 19, 2024 19:58