Skip to content

Commit 35e8d8e

Browse files
itslennyitslenny
committed
fix: tenant s3 credentials fixes and refactor
- migration to use correct cache key in pg_notify cache invalidation - fix s3 credentials count, was always returning 0 (and allowing creation of > 50 credentials) - check for valid uuid on delete credential endpoint (used to throw 500 on insert) - add test coverage - refactor s3 credentials into manager pattern similar to jwks
1 parent d880b92 commit 35e8d8e

File tree

23 files changed

+882
-223
lines changed

23 files changed

+882
-223
lines changed

‎migrations/multitenant/0017-tenants-s3-credentials-fix-notify-key.sql

+21
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
CREATE OR REPLACE FUNCTION tenants_s3_credentials_update_notify_trigger ()
2+
RETURNS TRIGGER
3+
AS $$
4+
BEGIN
5+
PERFORM
6+
pg_notify('tenants_s3_credentials_update', '"' || NEW.tenant_id || ':' || NEW.access_key || '"');
7+
RETURN NULL;
8+
END;
9+
$$
10+
LANGUAGE plpgsql;
11+
12+
CREATE OR REPLACE FUNCTION tenants_s3_credentials_delete_notify_trigger ()
13+
RETURNS TRIGGER
14+
AS $$
15+
BEGIN
16+
PERFORM
17+
pg_notify('tenants_s3_credentials_update', '"' || OLD.tenant_id || ':' || OLD.access_key || '"');
18+
RETURN NULL;
19+
END;
20+
$$
21+
LANGUAGE plpgsql;

‎src/http/error-handler.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export const setErrorHandler = (app: FastifyInstance) => {
6464
// Fastify errors
6565
if ('statusCode' in error) {
6666
const err = error as FastifyError
67-
return reply.status((error as any).statusCode || 500).send({
67+
return reply.status(err.statusCode || 500).send({
6868
statusCode: `${err.statusCode}`,
6969
error: err.name,
7070
message: err.message,

‎src/http/plugins/db.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ export const migrations = fastifyPlugin(
171171
})
172172

173173
if (dbMigrationStrategy === MultitenantMigrationStrategy.ON_REQUEST) {
174-
const migrationsMutex = createMutexByKey()
174+
const migrationsMutex = createMutexByKey<void>()
175175

176176
fastify.addHook('preHandler', async (request) => {
177177
// migrations are handled via async migrations

‎src/http/plugins/jwt.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export const jwt = fastifyPlugin(
2525
fastify.decorateRequest('jwt', '')
2626
fastify.decorateRequest('jwtPayload', undefined)
2727

28-
fastify.addHook('preHandler', async (request, reply) => {
28+
fastify.addHook('preHandler', async (request) => {
2929
request.jwt = (request.headers.authorization || '').replace(BEARER, '')
3030

3131
if (!request.jwt && request.routeOptions.config.allowInvalidJwt) {
@@ -41,13 +41,14 @@ export const jwt = fastifyPlugin(
4141
request.jwtPayload = payload
4242
request.owner = payload.sub
4343
request.isAuthenticated = true
44-
} catch (err: any) {
44+
} catch (e) {
4545
request.jwtPayload = { role: 'anon' }
4646
request.isAuthenticated = false
4747

4848
if (request.routeOptions.config.allowInvalidJwt) {
4949
return
5050
}
51+
const err = e as Error
5152
throw ERRORS.AccessDenied(err.message, err)
5253
}
5354
})

‎src/http/plugins/signature-v4.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { FastifyInstance, FastifyRequest } from 'fastify'
22
import fastifyPlugin from 'fastify-plugin'
3-
import { getJwtSecret, getS3CredentialsByAccessKey, getTenantConfig } from '@internal/database'
3+
import { getJwtSecret, getTenantConfig, s3CredentialsManager } from '@internal/database'
44
import { ClientSignature, SignatureV4 } from '@storage/protocols/s3'
55
import { signJWT, verifyJWT } from '@internal/auth'
66
import { ERRORS } from '@internal/errors'
@@ -160,7 +160,7 @@ async function createServerSignature(tenantId: string, clientSignature: ClientSi
160160
}
161161

162162
if (isMultitenant) {
163-
const credential = await getS3CredentialsByAccessKey(
163+
const credential = await s3CredentialsManager.getS3CredentialsByAccessKey(
164164
tenantId,
165165
clientSignature.credentials.accessKey
166166
)

‎src/http/routes/admin/jwks.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ export default async function routes(fastify: FastifyInstance) {
110110
}
111111

112112
const result = await jwksManager.addJwk(params.tenantId, body.jwk, body.kind)
113-
return reply.send(result)
113+
return reply.status(201).send(result)
114114
}
115115
)
116116

‎src/http/routes/admin/s3.ts

+9-5
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { FastifyInstance, RequestGenericInterface } from 'fastify'
22
import apiKey from '../../plugins/apikey'
3-
import { createS3Credentials, deleteS3Credential, listS3Credentials } from '@internal/database'
3+
import { s3CredentialsManager } from '@internal/database'
44
import { FromSchema } from 'json-schema-to-ts'
5+
import { isUuid } from '@storage/limits'
6+
import { ERRORS } from '@internal/errors'
57

68
const createCredentialsSchema = {
79
description: 'Create S3 Credentials',
@@ -88,7 +90,7 @@ export default async function routes(fastify: FastifyInstance) {
8890
schema: createCredentialsSchema,
8991
},
9092
async (req, reply) => {
91-
const credentials = await createS3Credentials(req.params.tenantId, {
93+
const credentials = await s3CredentialsManager.createS3Credentials(req.params.tenantId, {
9294
description: req.body.description,
9395
claims: req.body.claims,
9496
})
@@ -106,8 +108,7 @@ export default async function routes(fastify: FastifyInstance) {
106108
'/:tenantId/credentials',
107109
{ schema: listCredentialsSchema },
108110
async (req, reply) => {
109-
const credentials = await listS3Credentials(req.params.tenantId)
110-
111+
const credentials = await s3CredentialsManager.listS3Credentials(req.params.tenantId)
111112
return reply.send(credentials)
112113
}
113114
)
@@ -116,7 +117,10 @@ export default async function routes(fastify: FastifyInstance) {
116117
'/:tenantId/credentials',
117118
{ schema: deleteCredentialsSchema },
118119
async (req, reply) => {
119-
await deleteS3Credential(req.params.tenantId, req.body.id)
120+
if (!isUuid(req.body.id)) {
121+
throw ERRORS.InvalidParameter('id not uuid')
122+
}
123+
await s3CredentialsManager.deleteS3Credential(req.params.tenantId, req.body.id)
120124

121125
return reply.code(204).send()
122126
}

‎src/internal/concurrency/mutex.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
import { Semaphore } from '@shopify/semaphore'
22

3-
export function createMutexByKey() {
3+
export function createMutexByKey<T>() {
44
const semaphoreMap = new Map<string, { semaphore: Semaphore; count: number }>()
55

6-
return async (key: string, fn: () => Promise<any>) => {
6+
return async (key: string, fn: () => Promise<T>) => {
77
let entry = semaphoreMap.get(key)
88
if (!entry) {
99
entry = { semaphore: new Semaphore(1), count: 0 }

‎src/internal/database/jwks-manager/manager.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const TENANTS_JWKS_UPDATE_CHANNEL = 'tenants_jwks_update'
99
const JWK_KIND_STORAGE_URL_SIGNING = 'storage-url-signing-key'
1010
const JWK_KID_SEPARATOR = '_'
1111

12-
const tenantJwksMutex = createMutexByKey()
12+
const tenantJwksMutex = createMutexByKey<JwksConfig>()
1313
const tenantJwksConfigCache = new Map<string, JwksConfig>()
1414

1515
function createJwkKid({ kind, id }: { id: string; kind: string }): string {

‎src/internal/database/s3-credentials-manager/index.ts

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export * from './manager'

‎src/internal/database/s3-credentials-manager/manager.ts

+126
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import crypto from 'node:crypto'
2+
import { LRUCache } from 'lru-cache'
3+
import objectSizeOf from 'object-sizeof'
4+
import { S3Credentials, S3CredentialsManagerStore, S3CredentialsRaw } from './store'
5+
import { createMutexByKey } from '@internal/concurrency'
6+
import { ERRORS } from '@internal/errors'
7+
import { getConfig } from '../../../config'
8+
import { decrypt, encrypt } from '@internal/auth'
9+
import { PubSubAdapter } from '@internal/pubsub'
10+
11+
const TENANTS_S3_CREDENTIALS_UPDATE_CHANNEL = 'tenants_s3_credentials_update'
12+
13+
const tenantS3CredentialsCache = new LRUCache<string, S3Credentials>({
14+
maxSize: 1024 * 1024 * 50, // 50MB
15+
ttl: 1000 * 60 * 60, // 1 hour
16+
sizeCalculation: (value) => objectSizeOf(value),
17+
updateAgeOnGet: true,
18+
allowStale: false,
19+
})
20+
21+
const s3CredentialsMutex = createMutexByKey<S3Credentials>()
22+
23+
export class S3CredentialsManager {
24+
private dbServiceRole: string
25+
26+
constructor(private storage: S3CredentialsManagerStore) {
27+
const { dbServiceRole } = getConfig()
28+
this.dbServiceRole = dbServiceRole
29+
}
30+
31+
/**
32+
* Keeps the in memory config cache up to date
33+
*/
34+
async listenForTenantUpdate(pubSub: PubSubAdapter): Promise<void> {
35+
await pubSub.subscribe(TENANTS_S3_CREDENTIALS_UPDATE_CHANNEL, (cacheKey) => {
36+
tenantS3CredentialsCache.delete(cacheKey)
37+
})
38+
}
39+
40+
/**
41+
* Create S3 Credential for a tenant
42+
* @param tenantId
43+
* @param data
44+
*/
45+
async createS3Credentials(
46+
tenantId: string,
47+
data: { description: string; claims?: S3Credentials['claims'] }
48+
) {
49+
const existingCount = await this.countS3Credentials(tenantId)
50+
51+
if (existingCount >= 50) {
52+
throw ERRORS.MaximumCredentialsLimit()
53+
}
54+
55+
const accessKey = crypto.randomBytes(32).toString('hex').slice(0, 32)
56+
const secretKey = crypto.randomBytes(64).toString('hex').slice(0, 64)
57+
58+
if (data.claims) {
59+
delete data.claims.iss
60+
delete data.claims.issuer
61+
delete data.claims.exp
62+
delete data.claims.iat
63+
}
64+
65+
const claims = {
66+
...(data.claims || {}),
67+
role: data.claims?.role ?? this.dbServiceRole,
68+
issuer: `supabase.storage.${tenantId}`,
69+
sub: data.claims?.sub,
70+
}
71+
72+
const id = await this.storage.insert(tenantId, {
73+
description: data.description,
74+
claims,
75+
accessKey,
76+
secretKey: encrypt(secretKey),
77+
})
78+
79+
return {
80+
id,
81+
access_key: accessKey,
82+
secret_key: secretKey,
83+
}
84+
}
85+
86+
async getS3CredentialsByAccessKey(tenantId: string, accessKey: string): Promise<S3Credentials> {
87+
const cacheKey = `${tenantId}:${accessKey}`
88+
const cachedCredentials = tenantS3CredentialsCache.get(cacheKey)
89+
90+
if (cachedCredentials) {
91+
return cachedCredentials
92+
}
93+
94+
return s3CredentialsMutex(cacheKey, async () => {
95+
const cachedCredentials = tenantS3CredentialsCache.get(cacheKey)
96+
97+
if (cachedCredentials) {
98+
return cachedCredentials
99+
}
100+
101+
const data = await this.storage.getOneByAccessKey(tenantId, accessKey)
102+
103+
if (!data) {
104+
throw ERRORS.MissingS3Credentials()
105+
}
106+
107+
data.secretKey = decrypt(data.secretKey)
108+
109+
tenantS3CredentialsCache.set(cacheKey, data)
110+
111+
return data
112+
})
113+
}
114+
115+
deleteS3Credential(tenantId: string, credentialId: string): Promise<number> {
116+
return this.storage.delete(tenantId, credentialId)
117+
}
118+
119+
listS3Credentials(tenantId: string): Promise<S3CredentialsRaw[]> {
120+
return this.storage.list(tenantId)
121+
}
122+
123+
async countS3Credentials(tenantId: string) {
124+
return this.storage.count(tenantId)
125+
}
126+
}

‎src/internal/database/s3-credentials-manager/store-knex.ts

+59
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import { Knex } from 'knex'
2+
import {
3+
S3Credentials,
4+
S3CredentialsManagerStore,
5+
S3CredentialsRaw,
6+
S3CredentialWithDescription,
7+
} from './store'
8+
9+
export class S3CredentialsManagerStoreKnex implements S3CredentialsManagerStore {
10+
constructor(private knex: Knex) {}
11+
12+
async insert(tenantId: string, credential: S3CredentialWithDescription): Promise<string> {
13+
const credentials = await this.knex
14+
.table('tenants_s3_credentials')
15+
.insert({
16+
tenant_id: tenantId,
17+
description: credential.description,
18+
access_key: credential.accessKey,
19+
secret_key: credential.secretKey,
20+
claims: JSON.stringify(credential.claims),
21+
})
22+
.returning('id')
23+
return credentials[0].id
24+
}
25+
26+
list(tenantId: string): Promise<S3CredentialsRaw[]> {
27+
return this.knex
28+
.table('tenants_s3_credentials')
29+
.select<S3CredentialsRaw[]>('id', 'description', 'access_key', 'created_at')
30+
.where('tenant_id', tenantId)
31+
.orderBy('created_at', 'asc')
32+
}
33+
34+
getOneByAccessKey(tenantId: string, accessKey: string): Promise<S3Credentials> {
35+
return this.knex
36+
.table('tenants_s3_credentials')
37+
.select({ accessKey: 'access_key', secretKey: 'secret_key', claims: 'claims' })
38+
.where('tenant_id', tenantId)
39+
.where('access_key', accessKey)
40+
.first()
41+
}
42+
43+
async count(tenantId: string): Promise<number> {
44+
const data = await this.knex
45+
.table('tenants_s3_credentials')
46+
.count<{ count: number }>('id')
47+
.where('tenant_id', tenantId)
48+
.first()
49+
return Number(data?.count || 0)
50+
}
51+
52+
delete(tenantId: string, credentialId: string): Promise<number> {
53+
return this.knex
54+
.table('tenants_s3_credentials')
55+
.where('tenant_id', tenantId)
56+
.where('id', credentialId)
57+
.delete()
58+
}
59+
}

‎src/internal/database/s3-credentials-manager/store.ts

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
export interface S3Credentials {
2+
accessKey: string
3+
secretKey: string
4+
claims: { role: string; sub?: string; [key: string]: unknown }
5+
}
6+
7+
export interface S3CredentialWithDescription extends S3Credentials {
8+
description: string
9+
}
10+
11+
export interface S3CredentialsRaw {
12+
id: string
13+
description: string
14+
access_key: string
15+
created_at: string
16+
}
17+
18+
export interface S3CredentialsManagerStore {
19+
/**
20+
* Inserts a new credential and returns the id
21+
*
22+
* @param tenantId
23+
*/
24+
insert(tenantId: string, credential: S3CredentialWithDescription): Promise<string>
25+
26+
/**
27+
* List all credentials for the specified tenant
28+
* Returns data in the database style (snake case) format because the endpoint is expected to return data in this format
29+
*
30+
* @param tenantId
31+
*/
32+
list(tenantId: string): Promise<S3CredentialsRaw[]>
33+
34+
/**
35+
* Get one credential for the specified tenant / access key
36+
*
37+
* @param tenantId
38+
* @param accessKey
39+
*/
40+
getOneByAccessKey(tenantId: string, accessKey: string): Promise<S3Credentials>
41+
42+
/**
43+
* Gets the count of credentials for the specified tenant
44+
*
45+
* @param tenantId
46+
*/
47+
count(tenantId: string): Promise<number>
48+
49+
/**
50+
* Deletes a credential and returns the count of items deleted
51+
*
52+
* @param tenantId
53+
* @param credentialId
54+
*/
55+
delete(tenantId: string, credentialId: string): Promise<number>
56+
}

0 commit comments

Comments
 (0)