fix: allow access to devices only by public ID

Device ID should never be allowed to use as access key,
because it can be guessed.

Author: coderbyheart

cdk/BackendStack.ts

@@ -71,6 +71,11 @@ export class BackendStack extends Stack {
 			description: 'name of the public devices table',
 			value: publicDevices.publicDevicesTable.tableName,
 		})
+		new CfnOutput(this, 'publicDevicesTableIdIndexName', {
+			exportName: `${this.stackName}:publicDevicesTableIdIndexName`,
+			description: 'name of the public devices table id index',
+			value: publicDevices.idIndex,
+		})
 
 		const api = new API(this)
 		api.addRoute(
@@ -192,6 +197,7 @@ export type StackOutputs = {
 	 */
 	gatewayDomainName?: string
 	publicDevicesTableName: string
+	publicDevicesTableIdIndexName: string
 	/**
 	 * Role ARN to use in the deploy GitHub Actions Workflow
 	 */

cdk/resources/CustomDevicesAPI.ts

@@ -66,6 +66,7 @@ export class CustomDevicesAPI extends Construct {
 				BACKEND_STACK_NAME: STACK_NAME,
 				OPENSSL_LAMBDA_FUNCTION_NAME: openSSLFn.functionName,
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 			},
 			...new LambdaLogGroup(this, 'createCredentialsFnLogs'),
 			initialPolicy: [SettingsPermissions(Stack.of(this))],

cdk/resources/DevicesAPI.ts

@@ -34,6 +34,7 @@ export class DevicesAPI extends Construct {
 			environment: {
 				VERSION: this.node.getContext('version'),
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 				PUBLIC_DEVICES_TABLE_MODEL_OWNER_CONFIRMED_INDEX_NAME:
 					publicDevices.publicDevicesTableModelOwnerConfirmedIndex,
 				NODE_NO_WARNINGS: '1',

cdk/resources/LwM2MShadow.ts

@@ -43,6 +43,7 @@ export class LwM2MShadow extends Construct {
 			environment: {
 				VERSION: this.node.getContext('version'),
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 			},
 			initialPolicy: [
 				new IAM.PolicyStatement({

cdk/resources/SenMLMessage.ts

@@ -61,6 +61,7 @@ export class SenMLMessages extends Construct {
 			environment: {
 				VERSION: this.node.getContext('version'),
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 				IMPORT_LOGS_TABLE_NAME: importLogs.tableName,
 			},
 			initialPolicy: [

cdk/resources/ShareAPI.ts

@@ -45,6 +45,7 @@ export class ShareAPI extends Construct {
 			environment: {
 				VERSION: this.node.getContext('version'),
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 				FROM_EMAIL: `notification@${domain}`,
 				NODE_NO_WARNINGS: '1',
 				IS_TEST: this.node.getContext('isTest') === true ? '1' : '0',
@@ -83,6 +84,7 @@ export class ShareAPI extends Construct {
 			environment: {
 				VERSION: this.node.getContext('version'),
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 				NODE_NO_WARNINGS: '1',
 			},
 			...new LambdaLogGroup(this, 'confirmOwnershipFnLogs'),
@@ -101,6 +103,7 @@ export class ShareAPI extends Construct {
 			environment: {
 				VERSION: this.node.getContext('version'),
 				PUBLIC_DEVICES_TABLE_NAME: publicDevices.publicDevicesTable.tableName,
+				PUBLIC_DEVICES_ID_INDEX_NAME: publicDevices.idIndex,
 				NODE_NO_WARNINGS: '1',
 			},
 			...new LambdaLogGroup(this, 'sharingStatusFnLogs'),

cli/cli.ts

@@ -64,18 +64,20 @@ const CLI = async ({ isCI }: { isCI: boolean }) => {
 		console.error('Running on CI...')
 	} else {
 		try {
-			const mapOutputs = await stackOutput(cf)<StackOutputs>(STACK_NAME)
+			const backendOutputs = await stackOutput(cf)<StackOutputs>(STACK_NAME)
 			commands.push(
 				registerCustomMapDevice({
 					db,
-					publicDevicesTableName: mapOutputs.publicDevicesTableName,
+					publicDevicesTableName: backendOutputs.publicDevicesTableName,
+					idIndex: backendOutputs.publicDevicesTableIdIndexName,
 					ssm,
 					env: accountEnv,
 					stackName: STACK_NAME,
 				}),
 				shareDevice({
 					db,
-					publicDevicesTableName: mapOutputs.publicDevicesTableName,
+					publicDevicesTableName: backendOutputs.publicDevicesTableName,
+					idIndex: backendOutputs.publicDevicesTableIdIndexName,
 				}),
 			)
 		} catch (error) {

cli/commands/register-custom-device.ts

@@ -6,7 +6,7 @@ import chalk from 'chalk'
 import { randomUUID } from 'node:crypto'
 import fs from 'node:fs/promises'
 import path from 'node:path'
-import { publicDevicesRepo } from '../../sharing/publicDevicesRepo.js'
+import { publicDevicesRepo } from '../../devices/publicDevicesRepo.js'
 import { createCA } from '@hello.nrfcloud.com/certificate-helpers/ca'
 import { createDeviceCertificate } from '@hello.nrfcloud.com/certificate-helpers/device'
 import type { CommandDefinition } from './CommandDefinition.js'
@@ -21,12 +21,14 @@ export const registerCustomMapDevice = ({
 	stackName,
 	db,
 	publicDevicesTableName,
+	idIndex,
 	env,
 }: {
 	ssm: SSMClient
 	stackName: string
 	db: DynamoDBClient
 	publicDevicesTableName: string
+	idIndex: string
 	env: Required<Environment>
 }): CommandDefinition => ({
 	command: 'register-custom-map-device <model> <email>',
@@ -43,6 +45,7 @@ export const registerCustomMapDevice = ({
 		const publicDevice = publicDevicesRepo({
 			db,
 			TableName: publicDevicesTableName,
+			idIndex,
 		})
 		const maybePublished = await publicDevice.share({
 			deviceId,

cli/commands/share-device.ts

@@ -1,17 +1,19 @@
 import type { DynamoDBClient } from '@aws-sdk/client-dynamodb'
 import { models } from '@hello.nrfcloud.com/proto-map'
 import chalk from 'chalk'
-import { publicDevicesRepo } from '../../sharing/publicDevicesRepo.js'
+import { publicDevicesRepo } from '../../devices/publicDevicesRepo.js'
 import type { CommandDefinition } from './CommandDefinition.js'
 
 const modelIDs = Object.keys(models)
 
 export const shareDevice = ({
 	db,
 	publicDevicesTableName,
+	idIndex,
 }: {
 	db: DynamoDBClient
 	publicDevicesTableName: string
+	idIndex: string
 }): CommandDefinition => ({
 	command: `share-device <deviceId> <model> <email>`,
 	action: async (deviceId, model, email) => {
@@ -27,6 +29,7 @@ export const shareDevice = ({
 		const publicDevice = publicDevicesRepo({
 			db,
 			TableName: publicDevicesTableName,
+			idIndex,
 		})
 		const maybePublished = await publicDevice.share({
 			deviceId,

sharing/consentDuration.ts renamed to devices/consentDuration.ts

@@ -1,6 +1,6 @@
 import { describe, it, mock } from 'node:test'
 import assert from 'node:assert/strict'
-import { getDeviceById, publicDevicesRepo } from './publicDevicesRepo.js'
+import { publicDevicesRepo, toPublic } from './publicDevicesRepo.js'
 import { marshall } from '@aws-sdk/util-dynamodb'
 import { assertCall } from '../util/test/assertCall.js'
 import { randomUUID } from 'node:crypto'
@@ -10,18 +10,20 @@ import {
 	alphabet,
 	numbers,
 } from '@hello.nrfcloud.com/proto/fingerprint'
+import { ModelID } from '@hello.nrfcloud.com/proto-map'
 
 void describe('publicDevicesRepo()', () => {
 	void describe('getByDeviceId()', () => {
 		void it('should fetch device data', async () => {
+			const ownerConfirmed = new Date()
 			const id = randomUUID()
 			const send = mock.fn(async () =>
 				Promise.resolve({
 					Item: marshall({
 						id,
 						secret__deviceId: 'some-device',
 						model: 'asset_tracker_v2+AWS',
-						ownerConfirmed: new Date().toISOString(),
+						ownerConfirmed: ownerConfirmed.toISOString(),
 					}),
 				}),
 			)
@@ -31,11 +33,14 @@ void describe('publicDevicesRepo()', () => {
 						send,
 					} as any,
 					TableName: 'some-table',
+					idIndex: 'idIndex',
 				}).getByDeviceId('some-device'),
 				{
-					publicDevice: {
+					device: {
 						id,
 						model: 'asset_tracker_v2+AWS',
+						secret__deviceId: 'some-device',
+						ownerConfirmed: ownerConfirmed.toISOString(),
 					},
 				},
 			)
@@ -54,6 +59,7 @@ void describe('publicDevicesRepo()', () => {
 						send: async () => Promise.resolve({}),
 					} as any,
 					TableName: 'some-table',
+					idIndex: 'idIndex',
 				}).getByDeviceId('some-device'),
 				{ error: 'not_found' },
 			))
@@ -70,13 +76,14 @@ void describe('publicDevicesRepo()', () => {
 				} as any,
 				TableName: 'some-table',
 				now,
+				idIndex: 'idIndex',
 			}).share({
 				deviceId: 'some-device',
 				model: 'asset_tracker_v2+AWS',
 				email: '[email protected]',
 			})
 
-			const id = ('publicDevice' in res && res.publicDevice.id) as string
+			const id = ('device' in res && res.device.id) as string
 
 			assert.match(id, /^[a-z0-9]{8}-[a-z0-9]{8}-[a-z0-9]{8}$/) // e.g. mistrist-manicate-lunation
 
@@ -120,13 +127,14 @@ void describe('publicDevicesRepo()', () => {
 				} as any,
 				TableName: 'some-table',
 				now,
+				idIndex: 'idIndex',
 			}).confirmOwnership({
 				deviceId: id,
 				ownershipConfirmationToken,
 			})
 
 			assert.deepEqual(res, {
-				publicDevice: {
+				device: {
 					id,
 				},
 			})
@@ -154,36 +162,41 @@ void describe('publicDevicesRepo()', () => {
 	})
 })
 
-void describe('getDeviceById()', () => {
+void describe('getById()', () => {
 	void it(`it should return a device by it's public ID`, async () => {
 		const id = randomUUID()
-		const send = mock.fn(async () =>
-			Promise.resolve({
-				Items: [
-					marshall({
+		const send = mock.fn()
+		send.mock.mockImplementationOnce(
+			async () =>
+				Promise.resolve({
+					Items: [
+						marshall({
+							id,
+							secret__deviceId: 'some-device',
+						}),
+					],
+				}),
+			0,
+		)
+		send.mock.mockImplementationOnce(
+			async () =>
+				Promise.resolve({
+					Item: marshall({
 						id,
 						secret__deviceId: 'some-device',
+						model: 'asset_tracker_v2+AWS',
+						ownerConfirmed: new Date().toISOString(),
 					}),
-				],
-			}),
-		)
-		const getByDeviceId = mock.fn(async () =>
-			Promise.resolve({
-				publicDevice: {
-					id,
-					model: 'asset_tracker_v2+AWS',
-				},
-			}),
+				}),
+			1,
 		)
-
-		const res = await getDeviceById({
+		const res = await publicDevicesRepo({
 			db: {
 				send,
 			} as any,
 			TableName: 'some-table',
 			idIndex: 'id-index',
-			getByDeviceId: getByDeviceId as any,
-		})(id)
+		}).getById(id)
 
 		assertCall(send, {
 			input: {
@@ -203,13 +216,44 @@ void describe('getDeviceById()', () => {
 			},
 		})
 
-		assert.deepEqual(getByDeviceId.mock.calls[0]?.arguments, ['some-device'])
-
-		assert.deepEqual(res, {
-			publicDevice: {
-				id,
-				model: 'asset_tracker_v2+AWS',
+		assertCall(
+			send,
+			{
+				input: {
+					TableName: 'some-table',
+					Key: { secret__deviceId: { S: 'some-device' } },
+				},
 			},
+			1,
+		)
+
+		assert.deepEqual('device' in res && toPublic(res.device), {
+			id,
+			model: 'asset_tracker_v2+AWS',
 		})
 	})
 })
+
+void describe('toPublic()', () => {
+	void it('should convert a record to a publicly shareable record', () => {
+		const id = randomUUID()
+		const record = toPublic({
+			id,
+			secret__deviceId: 'some-device',
+			model: ModelID.PCA20035_solar,
+			ownerConfirmed: new Date(),
+			ownerEmail: '[email protected]',
+			ownershipConfirmationToken: '123456',
+			ownershipConfirmationTokenCreated: new Date(),
+			ttl: Date.now(),
+		})
+		assert.deepEqual(record, {
+			id,
+			model: ModelID.PCA20035_solar,
+		})
+		assert.equal(
+			Object.values(record as Record<string, any>).includes('some-device'),
+			false,
+		)
+	})
+})