feat(backend): tenant signature validation for admin api #3164

njlie · 2024-12-06T18:35:25Z

Changes proposed in this pull request

Adds a utility to determine if a http signature was signed by a tenant, the operator, or neither and augment the context of the request to reflect that.

Context

Fixes #2928.

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

mkurapov

To clarify, is the idea that we can run Rafiki without an operator tenant?

mkurapov · 2024-12-09T20:06:41Z

packages/backend/src/shared/utils.ts

+  }
+
+  const tenantService = await ctx.container.use('tenantService')
+  const tenantId = headers['tenantid']


Suggested change

const tenantId = headers['tenantid']

const tenantId = headers['tenant-id']

nit, but maybe kebab case is better for http headers?

mkurapov · 2024-12-09T20:07:01Z

packages/backend/src/shared/utils.ts

+
+  const tenantService = await ctx.container.use('tenantService')
+  const tenantId = headers['tenantid']
+  const tenant = tenantId ? await tenantService.get(tenantId) : undefined


should we return false if we can't find the tenant?

The idea here is to account for non-tenanted requests (e.g. createTenant, listTenants, other pagination requests). But this is moot since our call where the tenant id is to be expected in any request.

mkurapov · 2024-12-09T20:15:27Z

packages/backend/src/shared/utils.ts

+  if (
+    verifyApiSignatureDigest(
+      signature as string,
+      ctx.request,
+      config,
+      config.adminApiSecret as string
+    )
+  ) {
+    ctx.tenant = tenant
+    ctx.isOperator = true
+    return true
+  }


(related to my PR)
If we think that apiSecret is required in the DB schema, and we are seeding the operator tenant, we could actually avoid this extra check, and just determine whether its the operator by doing tenant.apiSecret = config.adminApiSecret after verifyingApiSignatureDigest

mkurapov · 2024-12-10T16:28:23Z

packages/backend/src/app.ts

@@ -384,12 +398,14 @@ export class App {
    )

    if (this.config.adminApiSecret) {


We can remove this check if this secret is required, right?

mkurapov · 2024-12-11T19:11:37Z

packages/backend/src/app.ts

-        if (!verifyApiSignature(ctx, this.config)) {
+    koa.use(
+      async (ctx: TenantedHttpSigContext, next: Koa.Next): Promise<void> => {
+        if (!verifyTenantOrOperatorApiSignature(ctx, this.config)) {


Suggested change

if (!verifyTenantOrOperatorApiSignature(ctx, this.config)) {

if (!(await verifyTenantOrOperatorApiSignature(ctx, this.config))) {

to mirror the PR just merged into main

mkurapov · 2024-12-12T13:30:32Z

packages/backend/src/shared/utils.test.ts

+
+      ctx.request.body = requestBody
+
+      const result = await verifyTenantOrOperatorApiSignature(ctx, config)


Might be good to check that the tenantService.get was in fact called

mkurapov · 2024-12-12T13:31:39Z

packages/backend/src/shared/utils.ts

+    signature as string,
+    ctx.request,
+    config,
+    config.adminApiSecret as string


Suggested change

config.adminApiSecret as string

config.adminApiSecret

Since it should already be typed

nit: but we can pass in either whole of config or adminApiSecret + adminApiSignatureVersion explicitly

njlie · 2024-12-12T21:14:41Z

Integration tests should start working again once #3177 is in

mkurapov · 2024-12-16T12:13:16Z

packages/backend/src/shared/utils.ts

+
+  if (!(await canApiSignatureBeProcessed(signature, ctx, config))) return false
+
+  // First, try validating with the tenant api secret


Suggested change

// First, try validating with the tenant api secret

mkurapov · 2024-12-16T12:14:05Z

packages/backend/src/shared/utils.ts

+  Verifies http signatures by first attempting to replicate it with a secret
+  associated with a tenant id in the headers, then with the configured admin secret.


Would be good to update this comment

mkurapov · 2024-12-16T12:51:37Z

packages/backend/src/app.ts

+type TenantedHttpSigHeaders = HttpSigHeaders & Record<'tenantId', string>
+
+type TenantedHttpSigRequest = Omit<HttpSigContext['request'], 'headers'> & {
+  headers: TenantedHttpSigHeaders
+}
+
+export type TenantedHttpSigContext = HttpSigContext & {
+  headers: TenantedHttpSigHeaders
+  request: TenantedHttpSigRequest
+  tenant?: Tenant
+  isOperator: boolean
+}


These types seem to be more suited for the Open Payments server. The main thing we need to do here is to add the the tenant object to the GraphQL context (ApolloContext & lines 409 -> 418), such that we can get the tenant information in the resolvers

mkurapov · 2024-12-17T10:55:35Z

packages/backend/src/shared/utils.ts

@@ -171,6 +173,53 @@ async function canApiSignatureBeProcessed(
  return true
 }

+export interface TenantApiSignatureResult {
+  tenant?: Tenant


Suggested change

tenant?: Tenant

tenant: Tenant

(same for TenantedApolloContext)

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Dec 6, 2024

njlie marked this pull request as ready for review December 6, 2024 18:38

njlie requested review from koekiebox, BlairCurrey, mkurapov, golobitch, sanducb and oana-lolea December 6, 2024 22:57

njlie force-pushed the nl/2928/multi-tenancy-signatures branch from f4d3fb6 to 22d48fe Compare December 6, 2024 23:02

github-actions bot added pkg: frontend Changes in the frontend package. pkg: auth Changes in the GNAP auth package. pkg: documentation Changes in the documentation package. labels Dec 6, 2024

Base automatically changed from nl/3123/backend-tenant-service to 2893/multi-tenancy-v1 December 9, 2024 17:32

njlie force-pushed the nl/2928/multi-tenancy-signatures branch from 22d48fe to 3805b10 Compare December 9, 2024 18:43

github-actions bot removed pkg: frontend Changes in the frontend package. pkg: documentation Changes in the documentation package. pkg: auth Changes in the GNAP auth package. labels Dec 9, 2024

mkurapov reviewed Dec 9, 2024

View reviewed changes

mkurapov linked an issue Dec 9, 2024 that may be closed by this pull request

[Multi-Tenant] Use http-signatures to determine tenant identity during requests #2928

Closed

2 tasks

mkurapov reviewed Dec 10, 2024

View reviewed changes

njlie requested a review from mkurapov December 10, 2024 23:43

mkurapov reviewed Dec 12, 2024

View reviewed changes

njlie requested a review from mkurapov December 13, 2024 19:40

mkurapov reviewed Dec 16, 2024

View reviewed changes

njlie added 2 commits December 16, 2024 10:59

feat(auth): tenants table v1

5a04f4d

feat(backend): tenant service

49e335e

njlie added 12 commits December 16, 2024 10:59

feat: use soft delete

12500cc

feat: add idp columns to tenant model

54e62ef

feat: pagination tests, push deletedAt to auth api call

10e2922

feat: add cache

56d51e4

feat(backend): tenant signature validation for admin api

f7fe54b

fix: rebase errors

11e91a6

fix: remove admin api secret check from app

496df5e

fix: always expect tenant id in request

a78e1ad

chore: remove some logs

f9437dc

feat: await signature verification, test improvements

1e6a93a

fix: better util parameters

6ae0cf6

fix: add tenant info to apollo context

23b121d

njlie force-pushed the nl/2928/multi-tenancy-signatures branch from 602aa6e to 23b121d Compare December 16, 2024 19:04

feat: fix integration tests

8994937

mkurapov approved these changes Dec 17, 2024

View reviewed changes

sanducb approved these changes Dec 17, 2024

View reviewed changes

fix: make tenant required on extended apollo context

39b663c

njlie merged commit a8b7ca4 into 2893/multi-tenancy-v1 Dec 17, 2024
36 checks passed

njlie deleted the nl/2928/multi-tenancy-signatures branch December 17, 2024 15:23

golobitch mentioned this pull request Mar 1, 2025

feature(wallet-address)!: possibility to specify wallet address range #3324

Closed

6 tasks

	const tenantId = headers['tenantid']
	const tenantId = headers['tenant-id']

		@@ -384,12 +398,14 @@ export class App {
		)

		if (this.config.adminApiSecret) {

	if (!verifyTenantOrOperatorApiSignature(ctx, this.config)) {
	if (!(await verifyTenantOrOperatorApiSignature(ctx, this.config))) {


		ctx.request.body = requestBody

		const result = await verifyTenantOrOperatorApiSignature(ctx, config)


		if (!(await canApiSignatureBeProcessed(signature, ctx, config))) return false

		// First, try validating with the tenant api secret

		Verifies http signatures by first attempting to replicate it with a secret
		associated with a tenant id in the headers, then with the configured admin secret.

feat(backend): tenant signature validation for admin api #3164

feat(backend): tenant signature validation for admin api #3164

Uh oh!

Conversation

njlie commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Context

Checklist

Uh oh!

mkurapov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njlie commented Dec 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

njlie commented Dec 6, 2024 •

edited

Loading