From d79099946a8524c6995cf81f96978176466dcec0 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Wed, 25 Sep 2024 16:05:50 +0400 Subject: [PATCH 1/9] feat(secret-sharing): server-side encryption --- .../20240925100349_managed-secret-sharing.ts | 27 +++++++ backend/src/db/schemas/secret-sharing.ts | 11 ++- backend/src/server/routes/index.ts | 3 +- .../server/routes/v1/secret-sharing-router.ts | 23 +++--- .../secret-sharing/secret-sharing-service.ts | 76 +++++++++++++------ .../secret-sharing/secret-sharing-types.ts | 5 +- .../src/hooks/api/secretSharing/mutations.ts | 14 +++- frontend/src/hooks/api/secretSharing/types.ts | 12 +-- .../components/ShareSecretForm.tsx | 23 +----- .../ViewSecretPublicPage.tsx | 37 +++++++-- .../components/SecretContainer.tsx | 6 +- 11 files changed, 155 insertions(+), 82 deletions(-) create mode 100644 backend/src/db/migrations/20240925100349_managed-secret-sharing.ts diff --git a/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts b/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts new file mode 100644 index 0000000000..25871455a8 --- /dev/null +++ b/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts @@ -0,0 +1,27 @@ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +export async function up(knex: Knex): Promise { + if (await knex.schema.hasTable(TableName.SecretSharing)) { + await knex.schema.alterTable(TableName.SecretSharing, (t) => { + t.string("iv").nullable().alter(); + t.string("tag").nullable().alter(); + t.string("encryptedValue").nullable().alter(); + + t.binary("encryptedSecret").nullable(); + }); + } +} + +export async function down(knex: Knex): Promise { + if (await knex.schema.hasTable(TableName.SecretSharing)) { + await knex.schema.alterTable(TableName.SecretSharing, (t) => { + t.string("iv").notNullable().alter(); + t.string("tag").notNullable().alter(); + t.string("encryptedValue").notNullable().alter(); + + t.dropColumn("encryptedSecret"); + }); + } +} diff --git a/backend/src/db/schemas/secret-sharing.ts b/backend/src/db/schemas/secret-sharing.ts index 2d0fc5eb5f..d7597af6e3 100644 --- a/backend/src/db/schemas/secret-sharing.ts +++ b/backend/src/db/schemas/secret-sharing.ts @@ -5,13 +5,15 @@ import { z } from "zod"; +import { zodBuffer } from "@app/lib/zod"; + import { TImmutableDBKeys } from "./models"; export const SecretSharingSchema = z.object({ id: z.string().uuid(), - encryptedValue: z.string(), - iv: z.string(), - tag: z.string(), + encryptedValue: z.string().nullable().optional(), + iv: z.string().nullable().optional(), + tag: z.string().nullable().optional(), hashedHex: z.string(), expiresAt: z.date(), userId: z.string().uuid().nullable().optional(), @@ -22,7 +24,8 @@ export const SecretSharingSchema = z.object({ accessType: z.string().default("anyone"), name: z.string().nullable().optional(), lastViewedAt: z.date().nullable().optional(), - password: z.string().nullable().optional() + password: z.string().nullable().optional(), + encryptedSecret: zodBuffer.nullable().optional() }); export type TSecretSharing = z.infer; diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index 5d9632215a..c392cb24e0 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -917,7 +917,8 @@ export const registerRoutes = async ( const secretSharingService = secretSharingServiceFactory({ permissionService, secretSharingDAL, - orgDAL + orgDAL, + kmsService }); const accessApprovalPolicyService = accessApprovalPolicyServiceFactory({ diff --git a/backend/src/server/routes/v1/secret-sharing-router.ts b/backend/src/server/routes/v1/secret-sharing-router.ts index 7a909cae43..0c021fc534 100644 --- a/backend/src/server/routes/v1/secret-sharing-router.ts +++ b/backend/src/server/routes/v1/secret-sharing-router.ts @@ -73,7 +73,8 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => accessType: true }) .extend({ - orgName: z.string().optional() + orgName: z.string().optional(), + secretValue: z.string().optional() }) .optional() }) @@ -99,17 +100,15 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => }, schema: { body: z.object({ - encryptedValue: z.string(), + secretValue: z.string(), password: z.string().optional(), - hashedHex: z.string(), - iv: z.string(), - tag: z.string(), expiresAt: z.string(), expiresAfterViews: z.number().min(1).optional() }), response: { 200: z.object({ - id: z.string().uuid() + id: z.string().uuid(), + hashedHex: z.string() }) } }, @@ -118,7 +117,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => ...req.body, accessType: SecretSharingAccessType.Anyone }); - return { id: sharedSecret.id }; + return { id: sharedSecret.id, hashedHex: sharedSecret.hashedHex }; } }); @@ -132,17 +131,15 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => body: z.object({ name: z.string().max(50).optional(), password: z.string().optional(), - encryptedValue: z.string(), - hashedHex: z.string(), - iv: z.string(), - tag: z.string(), + secretValue: z.string(), expiresAt: z.string(), expiresAfterViews: z.number().min(1).optional(), accessType: z.nativeEnum(SecretSharingAccessType).default(SecretSharingAccessType.Organization) }), response: { 200: z.object({ - id: z.string().uuid() + id: z.string().uuid(), + hashedHex: z.string() }) } }, @@ -156,7 +153,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => actorOrgId: req.permission.orgId, ...req.body }); - return { id: sharedSecret.id }; + return { id: sharedSecret.id, hashedHex: sharedSecret.hashedHex }; } }); diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 47eefcf6ee..3cb75ad759 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -1,3 +1,5 @@ +import crypto from "node:crypto"; + import bcrypt from "bcrypt"; import { TSecretSharing } from "@app/db/schemas"; @@ -5,6 +7,7 @@ import { TPermissionServiceFactory } from "@app/ee/services/permission/permissio import { BadRequestError, ForbiddenRequestError, NotFoundError, UnauthorizedError } from "@app/lib/errors"; import { SecretSharingAccessType } from "@app/lib/types"; +import { TKmsServiceFactory } from "../kms/kms-service"; import { TOrgDALFactory } from "../org/org-dal"; import { TSecretSharingDALFactory } from "./secret-sharing-dal"; import { @@ -19,6 +22,7 @@ type TSecretSharingServiceFactoryDep = { permissionService: Pick; secretSharingDAL: TSecretSharingDALFactory; orgDAL: TOrgDALFactory; + kmsService: TKmsServiceFactory; }; export type TSecretSharingServiceFactory = ReturnType; @@ -26,7 +30,8 @@ export type TSecretSharingServiceFactory = ReturnType { const createSharedSecret = async ({ actor, @@ -34,10 +39,7 @@ export const secretSharingServiceFactory = ({ orgId, actorAuthMethod, actorOrgId, - encryptedValue, - hashedHex, - iv, - tag, + secretValue, name, password, accessType, @@ -59,19 +61,28 @@ export const secretSharingServiceFactory = ({ throw new BadRequestError({ message: "Expiration date cannot be more than 30 days" }); } - // Limit Input ciphertext length to 13000 (equivalent to 10,000 characters of Plaintext) - if (encryptedValue.length > 13000) { + if (secretValue.length > 10_000) { throw new BadRequestError({ message: "Shared secret value too long" }); } + const encryptWithRoot = await kmsService.encryptWithRootKey(); + + const encryptedSecret = await encryptWithRoot({ + plainText: Buffer.from(secretValue) + }); + const hashedHex = crypto.createHash("sha256").update(secretValue).digest("hex").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; + const newSharedSecret = await secretSharingDAL.create({ + iv: null, + tag: null, + encryptedValue: null, + + encryptedSecret: encryptedSecret.cipherTextBlob, + hashedHex, + name, password: hashedPassword, - encryptedValue, - hashedHex, - iv, - tag, expiresAt: new Date(expiresAt), expiresAfterViews, userId: actorId, @@ -79,15 +90,12 @@ export const secretSharingServiceFactory = ({ accessType }); - return { id: newSharedSecret.id }; + return { id: newSharedSecret.id, hashedHex: newSharedSecret.hashedHex }; }; const createPublicSharedSecret = async ({ password, - encryptedValue, - hashedHex, - iv, - tag, + secretValue, expiresAt, expiresAfterViews, accessType @@ -104,24 +112,33 @@ export const secretSharingServiceFactory = ({ throw new BadRequestError({ message: "Expiration date cannot exceed more than 30 days" }); } - // Limit Input ciphertext length to 13000 (equivalent to 10,000 characters of Plaintext) - if (encryptedValue.length > 13000) { + // Limit Input ciphertext length to 13000 (equivalent to 10,000 characters of Plaintext)n + if (secretValue.length > 10_000) { throw new BadRequestError({ message: "Shared secret value too long" }); } + const encryptWithRoot = await kmsService.encryptWithRootKey(); + const encrypted = await encryptWithRoot({ + plainText: Buffer.from(secretValue) + }); + + const hashedHex = crypto.createHash("sha256").update(secretValue).digest("hex").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; + const newSharedSecret = await secretSharingDAL.create({ - password: hashedPassword, - encryptedValue, + encryptedValue: null, + iv: null, + tag: null, hashedHex, - iv, - tag, + encryptedSecret: encrypted.cipherTextBlob, + + password: hashedPassword, expiresAt: new Date(expiresAt), expiresAfterViews, accessType }); - return { id: newSharedSecret.id }; + return { id: newSharedSecret.id, hashedHex: newSharedSecret.hashedHex }; }; const getSharedSecrets = async ({ @@ -222,6 +239,16 @@ export const secretSharingServiceFactory = ({ } } + // If encryptedSecret is set, we know that this secret has been encrypted using KMS, and we can therefore do server-side decryption. + let decryptedSecretValue: Buffer | undefined; + if (sharedSecret.encryptedSecret) { + const decrypt = await kmsService.decryptWithRootKey(); + + decryptedSecretValue = await decrypt({ + cipherTextBlob: sharedSecret.encryptedSecret + }); + } + // decrement when we are sure the user will view secret. await $decrementSecretViewCount(sharedSecret, sharedSecretId); @@ -229,6 +256,9 @@ export const secretSharingServiceFactory = ({ isPasswordProtected, secret: { ...sharedSecret, + ...(decryptedSecretValue && { + secretValue: Buffer.from(decryptedSecretValue).toString() + }), orgName: sharedSecret.accessType === SecretSharingAccessType.Organization && orgId === sharedSecret.orgId ? orgName diff --git a/backend/src/services/secret-sharing/secret-sharing-types.ts b/backend/src/services/secret-sharing/secret-sharing-types.ts index 794d99a339..9469e8de5c 100644 --- a/backend/src/services/secret-sharing/secret-sharing-types.ts +++ b/backend/src/services/secret-sharing/secret-sharing-types.ts @@ -19,10 +19,7 @@ export type TSharedSecretPermission = { }; export type TCreatePublicSharedSecretDTO = { - encryptedValue: string; - hashedHex: string; - iv: string; - tag: string; + secretValue: string; expiresAt: string; expiresAfterViews?: number; password?: string; diff --git a/frontend/src/hooks/api/secretSharing/mutations.ts b/frontend/src/hooks/api/secretSharing/mutations.ts index 7dec0bd5e5..e805abfd29 100644 --- a/frontend/src/hooks/api/secretSharing/mutations.ts +++ b/frontend/src/hooks/api/secretSharing/mutations.ts @@ -3,13 +3,21 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import { apiRequest } from "@app/config/request"; import { secretSharingKeys } from "./queries"; -import { TCreateSharedSecretRequest, TDeleteSharedSecretRequest, TSharedSecret } from "./types"; +import { + TCreatedSharedSecret, + TCreateSharedSecretRequest, + TDeleteSharedSecretRequest, + TSharedSecret +} from "./types"; export const useCreateSharedSecret = () => { const queryClient = useQueryClient(); return useMutation({ mutationFn: async (inputData: TCreateSharedSecretRequest) => { - const { data } = await apiRequest.post("/api/v1/secret-sharing", inputData); + const { data } = await apiRequest.post( + "/api/v1/secret-sharing", + inputData + ); return data; }, onSuccess: () => queryClient.invalidateQueries(secretSharingKeys.allSharedSecrets()) @@ -20,7 +28,7 @@ export const useCreatePublicSharedSecret = () => { const queryClient = useQueryClient(); return useMutation({ mutationFn: async (inputData: TCreateSharedSecretRequest) => { - const { data } = await apiRequest.post( + const { data } = await apiRequest.post( "/api/v1/secret-sharing/public", inputData ); diff --git a/frontend/src/hooks/api/secretSharing/types.ts b/frontend/src/hooks/api/secretSharing/types.ts index 0dd4a95555..4f0b27d959 100644 --- a/frontend/src/hooks/api/secretSharing/types.ts +++ b/frontend/src/hooks/api/secretSharing/types.ts @@ -13,13 +13,15 @@ export type TSharedSecret = { tag: string; }; +export type TCreatedSharedSecret = { + id: string; + hashedHex: string; +}; + export type TCreateSharedSecretRequest = { name?: string; password?: string; - encryptedValue: string; - hashedHex: string; - iv: string; - tag: string; + secretValue: string; expiresAt: Date; expiresAfterViews?: number; accessType?: SecretSharingAccessType; @@ -28,6 +30,7 @@ export type TCreateSharedSecretRequest = { export type TViewSharedSecretResponse = { isPasswordProtected: boolean; secret: { + secretValue?: string; encryptedValue: string; iv: string; tag: string; @@ -44,4 +47,3 @@ export enum SecretSharingAccessType { Anyone = "anyone", Organization = "organization" } - diff --git a/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx b/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx index ea39e12651..91cd362573 100644 --- a/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx +++ b/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx @@ -1,5 +1,3 @@ -import crypto from "crypto"; - import { useState } from "react"; import { Controller, useForm } from "react-hook-form"; import { faCheck, faCopy, faRedo } from "@fortawesome/free-solid-svg-icons"; @@ -8,7 +6,6 @@ import { zodResolver } from "@hookform/resolvers/zod"; import { z } from "zod"; import { createNotification } from "@app/components/notifications"; -import { encryptSymmetric } from "@app/components/utilities/cryptography/crypto"; import { Button, FormControl, IconButton, Input, Select, SelectItem } from "@app/components/v2"; import { useTimedReset } from "@app/hooks"; import { useCreatePublicSharedSecret, useCreateSharedSecret } from "@app/hooks/api"; @@ -79,30 +76,16 @@ export const ShareSecretForm = ({ isPublic, value }: Props) => { try { const expiresAt = new Date(new Date().getTime() + Number(expiresIn)); - const key = crypto.randomBytes(16).toString("hex"); - const hashedHex = crypto.createHash("sha256").update(key).digest("hex"); - const { ciphertext, iv, tag } = encryptSymmetric({ - plaintext: secret, - key - }); - - const { id } = await createSharedSecret.mutateAsync({ + const { id, hashedHex } = await createSharedSecret.mutateAsync({ name, password, - encryptedValue: ciphertext, - hashedHex, - iv, - tag, + secretValue: secret, expiresAt, expiresAfterViews: viewLimit === "-1" ? undefined : Number(viewLimit), accessType }); - setSecretLink( - `${window.location.origin}/shared/secret/${id}?key=${encodeURIComponent( - hashedHex - )}-${encodeURIComponent(key)}` - ); + setSecretLink(`${window.location.origin}/shared/secret/${id}-${hashedHex}`); reset(); setCopyTextSecret("secret"); diff --git a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx index 999f2d3423..6d75ca7823 100644 --- a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx +++ b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx @@ -1,23 +1,44 @@ import { useState } from "react"; import Image from "next/image"; import Link from "next/link"; -import { useRouter } from "next/router"; +import { NextRouter, useRouter } from "next/router"; import { faArrowRight } from "@fortawesome/free-solid-svg-icons"; import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; import { AxiosError } from "axios"; import { useGetActiveSharedSecretById } from "@app/hooks/api/secretSharing"; -import { PasswordContainer,SecretContainer, SecretErrorContainer } from "./components"; +import { PasswordContainer, SecretContainer, SecretErrorContainer } from "./components"; + +const extractDetailsFromUrl = (router: NextRouter) => { + const { id, key: urlEncodedKey } = router.query; + + if (urlEncodedKey) { + const [hashedHex, key] = urlEncodedKey ? urlEncodedKey.toString().split("-") : ["", ""]; + + return { + id: id as string, + hashedHex, + key + }; + } + + // its like this {uuid}-{hex} so example: idpart1-idpart2-idpart3-idpart4-hex + const extractedId = id?.toString().split("-").slice(0, 5).join("-"); + const extractedHex = id?.toString().split("-").slice(5).join("-"); + + return { + id: extractedId || "", + hashedHex: extractedHex || "", + key: null + }; +}; export const ViewSecretPublicPage = () => { const router = useRouter(); const [password, setPassword] = useState(); - const { id, key: urlEncodedPublicKey } = router.query; - const [hashedHex, key] = urlEncodedPublicKey - ? urlEncodedPublicKey.toString().split("-") - : ["", ""]; + const { hashedHex, key, id } = extractDetailsFromUrl(router); const { data: fetchSecret, @@ -25,7 +46,7 @@ export const ViewSecretPublicPage = () => { isLoading, isFetching } = useGetActiveSharedSecretById({ - sharedSecretId: id as string, + sharedSecretId: id, hashedHex, password }); @@ -80,7 +101,7 @@ export const ViewSecretPublicPage = () => { )} {!isLoading && ( <> - {!error && fetchSecret?.secret && key && ( + {!error && fetchSecret?.secret && ( )} {error && !isInvalidCredential && } diff --git a/frontend/src/views/ViewSecretPublicPage/components/SecretContainer.tsx b/frontend/src/views/ViewSecretPublicPage/components/SecretContainer.tsx index a920c9d933..f07ebfafd5 100644 --- a/frontend/src/views/ViewSecretPublicPage/components/SecretContainer.tsx +++ b/frontend/src/views/ViewSecretPublicPage/components/SecretContainer.tsx @@ -15,7 +15,7 @@ import { TViewSharedSecretResponse } from "@app/hooks/api/secretSharing"; type Props = { secret: TViewSharedSecretResponse["secret"]; - secretKey: string; + secretKey: string | null; }; export const SecretContainer = ({ secret, secretKey: key }: Props) => { @@ -25,6 +25,10 @@ export const SecretContainer = ({ secret, secretKey: key }: Props) => { }); const decryptedSecret = useMemo(() => { + if (secret.secretValue) { + return secret.secretValue; + } + if (secret && secret.encryptedValue && key) { const res = decryptSymmetric({ ciphertext: secret.encryptedValue, From 1a2495a95cf63c2cd6a87e397e2996ed0b9d5b35 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Wed, 25 Sep 2024 16:26:01 +0400 Subject: [PATCH 2/9] fix: improved root kms encryption methods --- backend/src/services/kms/kms-service.ts | 16 ++++++------ .../secret-sharing/secret-sharing-service.ts | 25 ++++++------------- backend/src/services/slack/slack-service.ts | 8 +++--- .../super-admin/super-admin-service.ts | 16 +++++------- 4 files changed, 25 insertions(+), 40 deletions(-) diff --git a/backend/src/services/kms/kms-service.ts b/backend/src/services/kms/kms-service.ts index ad06906dfc..aeef1678ec 100644 --- a/backend/src/services/kms/kms-service.ts +++ b/backend/src/services/kms/kms-service.ts @@ -208,20 +208,20 @@ export const kmsServiceFactory = ({ return org.kmsDefaultKeyId; }; - const encryptWithRootKey = async () => { + const encryptWithRootKey = () => { const cipher = symmetricCipherService(SymmetricEncryption.AES_GCM_256); - return ({ plainText }: { plainText: Buffer }) => { - const encryptedPlainTextBlob = cipher.encrypt(plainText, ROOT_ENCRYPTION_KEY); - return Promise.resolve({ cipherTextBlob: encryptedPlainTextBlob }); + return (plainTextBuffer: Buffer) => { + const encryptedBuffer = cipher.encrypt(plainTextBuffer, ROOT_ENCRYPTION_KEY); + return encryptedBuffer; }; }; - const decryptWithRootKey = async () => { + const decryptWithRootKey = () => { const cipher = symmetricCipherService(SymmetricEncryption.AES_GCM_256); - return ({ cipherTextBlob }: { cipherTextBlob: Buffer }) => { - const decryptedBlob = cipher.decrypt(cipherTextBlob, ROOT_ENCRYPTION_KEY); - return Promise.resolve(decryptedBlob); + + return (cipherTextBuffer: Buffer) => { + return cipher.decrypt(cipherTextBuffer, ROOT_ENCRYPTION_KEY); }; }; diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 3cb75ad759..4ad6124fb8 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -65,11 +65,9 @@ export const secretSharingServiceFactory = ({ throw new BadRequestError({ message: "Shared secret value too long" }); } - const encryptWithRoot = await kmsService.encryptWithRootKey(); + const encryptWithRoot = kmsService.encryptWithRootKey(); - const encryptedSecret = await encryptWithRoot({ - plainText: Buffer.from(secretValue) - }); + const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); const hashedHex = crypto.createHash("sha256").update(secretValue).digest("hex").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; @@ -77,10 +75,8 @@ export const secretSharingServiceFactory = ({ iv: null, tag: null, encryptedValue: null, - - encryptedSecret: encryptedSecret.cipherTextBlob, + encryptedSecret, hashedHex, - name, password: hashedPassword, expiresAt: new Date(expiresAt), @@ -117,10 +113,8 @@ export const secretSharingServiceFactory = ({ throw new BadRequestError({ message: "Shared secret value too long" }); } - const encryptWithRoot = await kmsService.encryptWithRootKey(); - const encrypted = await encryptWithRoot({ - plainText: Buffer.from(secretValue) - }); + const encryptWithRoot = kmsService.encryptWithRootKey(); + const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); const hashedHex = crypto.createHash("sha256").update(secretValue).digest("hex").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; @@ -130,7 +124,7 @@ export const secretSharingServiceFactory = ({ iv: null, tag: null, hashedHex, - encryptedSecret: encrypted.cipherTextBlob, + encryptedSecret, password: hashedPassword, expiresAt: new Date(expiresAt), @@ -242,11 +236,8 @@ export const secretSharingServiceFactory = ({ // If encryptedSecret is set, we know that this secret has been encrypted using KMS, and we can therefore do server-side decryption. let decryptedSecretValue: Buffer | undefined; if (sharedSecret.encryptedSecret) { - const decrypt = await kmsService.decryptWithRootKey(); - - decryptedSecretValue = await decrypt({ - cipherTextBlob: sharedSecret.encryptedSecret - }); + const decryptWithRoot = kmsService.decryptWithRootKey(); + decryptedSecretValue = decryptWithRoot(sharedSecret.encryptedSecret); } // decrement when we are sure the user will view secret. diff --git a/backend/src/services/slack/slack-service.ts b/backend/src/services/slack/slack-service.ts index d4a3ebe4d5..43d7bb1768 100644 --- a/backend/src/services/slack/slack-service.ts +++ b/backend/src/services/slack/slack-service.ts @@ -141,16 +141,14 @@ export const slackServiceFactory = ({ let slackClientId = appCfg.WORKFLOW_SLACK_CLIENT_ID as string; let slackClientSecret = appCfg.WORKFLOW_SLACK_CLIENT_SECRET as string; - const decrypt = await kmsService.decryptWithRootKey(); + const decrypt = kmsService.decryptWithRootKey(); if (serverCfg.encryptedSlackClientId) { - slackClientId = (await decrypt({ cipherTextBlob: Buffer.from(serverCfg.encryptedSlackClientId) })).toString(); + slackClientId = decrypt(Buffer.from(serverCfg.encryptedSlackClientId)).toString(); } if (serverCfg.encryptedSlackClientSecret) { - slackClientSecret = ( - await decrypt({ cipherTextBlob: Buffer.from(serverCfg.encryptedSlackClientSecret) }) - ).toString(); + slackClientSecret = decrypt(Buffer.from(serverCfg.encryptedSlackClientSecret)).toString(); } if (!slackClientId || !slackClientSecret) { diff --git a/backend/src/services/super-admin/super-admin-service.ts b/backend/src/services/super-admin/super-admin-service.ts index b4f77df053..a3acd6751b 100644 --- a/backend/src/services/super-admin/super-admin-service.ts +++ b/backend/src/services/super-admin/super-admin-service.ts @@ -122,20 +122,16 @@ export const superAdminServiceFactory = ({ } } - const encryptWithRoot = await kmsService.encryptWithRootKey(); + const encryptWithRoot = kmsService.encryptWithRootKey(); if (data.slackClientId) { - const { cipherTextBlob: encryptedClientId } = await encryptWithRoot({ - plainText: Buffer.from(data.slackClientId) - }); + const encryptedClientId = encryptWithRoot(Buffer.from(data.slackClientId)); updatedData.encryptedSlackClientId = encryptedClientId; updatedData.slackClientId = undefined; } if (data.slackClientSecret) { - const { cipherTextBlob: encryptedClientSecret } = await encryptWithRoot({ - plainText: Buffer.from(data.slackClientSecret) - }); + const encryptedClientSecret = encryptWithRoot(Buffer.from(data.slackClientSecret)); updatedData.encryptedSlackClientSecret = encryptedClientSecret; updatedData.slackClientSecret = undefined; @@ -270,14 +266,14 @@ export const superAdminServiceFactory = ({ let clientId = ""; let clientSecret = ""; - const decrypt = await kmsService.decryptWithRootKey(); + const decrypt = kmsService.decryptWithRootKey(); if (serverCfg.encryptedSlackClientId) { - clientId = (await decrypt({ cipherTextBlob: serverCfg.encryptedSlackClientId })).toString(); + clientId = decrypt(serverCfg.encryptedSlackClientId).toString(); } if (serverCfg.encryptedSlackClientSecret) { - clientSecret = (await decrypt({ cipherTextBlob: serverCfg.encryptedSlackClientSecret })).toString(); + clientSecret = decrypt(serverCfg.encryptedSlackClientSecret).toString(); } return { From fa63c150dd448fbf488071e35ca4a416ef11b629 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Mon, 30 Sep 2024 18:40:17 +0400 Subject: [PATCH 3/9] requested changes --- ...20240930134623_secret-sharing-string-id.ts | 68 +++++++++++++++++++ backend/src/db/schemas/secret-sharing.ts | 4 +- .../server/routes/v1/secret-sharing-router.ts | 14 ++-- .../secret-sharing/secret-sharing-dal.ts | 12 +++- .../secret-sharing/secret-sharing-service.ts | 15 ++-- frontend/src/hooks/api/secretSharing/types.ts | 1 - .../components/ShareSecretForm.tsx | 4 +- .../ViewSecretPublicPage.tsx | 22 ++++-- 8 files changed, 116 insertions(+), 24 deletions(-) create mode 100644 backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts diff --git a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts new file mode 100644 index 0000000000..6cd1979151 --- /dev/null +++ b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts @@ -0,0 +1,68 @@ +/* eslint-disable @typescript-eslint/ban-ts-comment */ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +export async function up(knex: Knex): Promise { + if (await knex.schema.hasTable(TableName.SecretSharing)) { + await knex.schema.alterTable(TableName.SecretSharing, (t) => { + // Add a new column + t.string("new_id", 36).nullable(); + }); + + // Copy data from old column to new column + await knex(TableName.SecretSharing).update({ + // @ts-ignore + new_id: knex.raw("id::text") + }); + + await knex.schema.alterTable(TableName.SecretSharing, (t) => { + // Make the new column not nullable + t.string("new_id", 36).notNullable().alter(); + + // Drop the old primary key + t.dropPrimary(); + + // Drop the old id column + t.dropColumn("id"); + + // Rename the new column to 'id' + t.renameColumn("new_id", "id"); + + // Set the new column as primary key + t.primary(["id"]); + }); + } +} + +export async function down(knex: Knex): Promise { + if (await knex.schema.hasTable(TableName.SecretSharing)) { + await knex.schema.alterTable(TableName.SecretSharing, (t) => { + // Add a new UUID column + t.uuid("new_id").nullable(); + }); + + // Copy data from string id to UUID, ensuring valid UUID format + await knex(TableName.SecretSharing).update({ + // @ts-ignore + new_id: knex.raw("id::uuid") + }); + + await knex.schema.alterTable(TableName.SecretSharing, (t) => { + // Make the new column not nullable + t.uuid("new_id").notNullable().alter(); + + // Drop the old primary key + t.dropPrimary(); + + // Drop the old id column + t.dropColumn("id"); + + // Rename the new column to 'id' + t.renameColumn("new_id", "id"); + + // Set the new column as primary key + t.primary(["id"]); + }); + } +} diff --git a/backend/src/db/schemas/secret-sharing.ts b/backend/src/db/schemas/secret-sharing.ts index d7597af6e3..7490269d79 100644 --- a/backend/src/db/schemas/secret-sharing.ts +++ b/backend/src/db/schemas/secret-sharing.ts @@ -10,7 +10,6 @@ import { zodBuffer } from "@app/lib/zod"; import { TImmutableDBKeys } from "./models"; export const SecretSharingSchema = z.object({ - id: z.string().uuid(), encryptedValue: z.string().nullable().optional(), iv: z.string().nullable().optional(), tag: z.string().nullable().optional(), @@ -25,7 +24,8 @@ export const SecretSharingSchema = z.object({ name: z.string().nullable().optional(), lastViewedAt: z.date().nullable().optional(), password: z.string().nullable().optional(), - encryptedSecret: zodBuffer.nullable().optional() + encryptedSecret: zodBuffer.nullable().optional(), + id: z.string() }); export type TSecretSharing = z.infer; diff --git a/backend/src/server/routes/v1/secret-sharing-router.ts b/backend/src/server/routes/v1/secret-sharing-router.ts index 0c021fc534..2149931385 100644 --- a/backend/src/server/routes/v1/secret-sharing-router.ts +++ b/backend/src/server/routes/v1/secret-sharing-router.ts @@ -55,7 +55,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => }, schema: { params: z.object({ - id: z.string().uuid() + id: z.string() }), body: z.object({ hashedHex: z.string().min(1), @@ -107,8 +107,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => }), response: { 200: z.object({ - id: z.string().uuid(), - hashedHex: z.string() + id: z.string() }) } }, @@ -117,7 +116,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => ...req.body, accessType: SecretSharingAccessType.Anyone }); - return { id: sharedSecret.id, hashedHex: sharedSecret.hashedHex }; + return { id: sharedSecret.id }; } }); @@ -138,8 +137,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => }), response: { 200: z.object({ - id: z.string().uuid(), - hashedHex: z.string() + id: z.string() }) } }, @@ -153,7 +151,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => actorOrgId: req.permission.orgId, ...req.body }); - return { id: sharedSecret.id, hashedHex: sharedSecret.hashedHex }; + return { id: sharedSecret.id }; } }); @@ -165,7 +163,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => }, schema: { params: z.object({ - sharedSecretId: z.string().uuid() + sharedSecretId: z.string() }), response: { 200: SecretSharingSchema diff --git a/backend/src/services/secret-sharing/secret-sharing-dal.ts b/backend/src/services/secret-sharing/secret-sharing-dal.ts index 5c690b2667..f6108b3ff1 100644 --- a/backend/src/services/secret-sharing/secret-sharing-dal.ts +++ b/backend/src/services/secret-sharing/secret-sharing-dal.ts @@ -82,11 +82,21 @@ export const secretSharingDALFactory = (db: TDbClient) => { } }; + const create = async (data: Omit, tx?: Knex) => { + try { + const [res] = await (tx || db)(TableName.SecretSharing).insert(data).returning("*"); + return res; + } catch (error) { + throw new DatabaseError({ error, name: "Create Shared Secret" }); + } + }; + return { ...sharedSecretOrm, countAllUserOrgSharedSecrets, pruneExpiredSharedSecrets, softDeleteById, - findActiveSharedSecrets + findActiveSharedSecrets, + create }; }; diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 4ad6124fb8..0aa89dda1e 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -68,10 +68,15 @@ export const secretSharingServiceFactory = ({ const encryptWithRoot = kmsService.encryptWithRootKey(); const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); - const hashedHex = crypto.createHash("sha256").update(secretValue).digest("hex").substring(0, 13); + + // This will be 36 characters long, due to encoding it to base64. + const id = crypto.randomBytes(27).toString("base64url"); + + const hashedHex = crypto.createHash("sha256").update(id).digest("base64").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ + id, iv: null, tag: null, encryptedValue: null, @@ -86,7 +91,7 @@ export const secretSharingServiceFactory = ({ accessType }); - return { id: newSharedSecret.id, hashedHex: newSharedSecret.hashedHex }; + return { id: `${newSharedSecret.id}${hashedHex}` }; }; const createPublicSharedSecret = async ({ @@ -116,10 +121,12 @@ export const secretSharingServiceFactory = ({ const encryptWithRoot = kmsService.encryptWithRootKey(); const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); - const hashedHex = crypto.createHash("sha256").update(secretValue).digest("hex").substring(0, 13); + const id = crypto.randomBytes(27).toString("base64url"); + const hashedHex = crypto.createHash("sha256").update(id).digest("hex").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ + id, encryptedValue: null, iv: null, tag: null, @@ -132,7 +139,7 @@ export const secretSharingServiceFactory = ({ accessType }); - return { id: newSharedSecret.id, hashedHex: newSharedSecret.hashedHex }; + return { id: `${newSharedSecret.id}${hashedHex}` }; }; const getSharedSecrets = async ({ diff --git a/frontend/src/hooks/api/secretSharing/types.ts b/frontend/src/hooks/api/secretSharing/types.ts index 4f0b27d959..b9843a711c 100644 --- a/frontend/src/hooks/api/secretSharing/types.ts +++ b/frontend/src/hooks/api/secretSharing/types.ts @@ -15,7 +15,6 @@ export type TSharedSecret = { export type TCreatedSharedSecret = { id: string; - hashedHex: string; }; export type TCreateSharedSecretRequest = { diff --git a/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx b/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx index 91cd362573..6aa44f65df 100644 --- a/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx +++ b/frontend/src/views/ShareSecretPublicPage/components/ShareSecretForm.tsx @@ -76,7 +76,7 @@ export const ShareSecretForm = ({ isPublic, value }: Props) => { try { const expiresAt = new Date(new Date().getTime() + Number(expiresIn)); - const { id, hashedHex } = await createSharedSecret.mutateAsync({ + const { id } = await createSharedSecret.mutateAsync({ name, password, secretValue: secret, @@ -85,7 +85,7 @@ export const ShareSecretForm = ({ isPublic, value }: Props) => { accessType }); - setSecretLink(`${window.location.origin}/shared/secret/${id}-${hashedHex}`); + setSecretLink(`${window.location.origin}/shared/secret/${id}`); reset(); setCopyTextSecret("secret"); diff --git a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx index 6d75ca7823..f2309860bd 100644 --- a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx +++ b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx @@ -13,23 +13,33 @@ import { PasswordContainer, SecretContainer, SecretErrorContainer } from "./comp const extractDetailsFromUrl = (router: NextRouter) => { const { id, key: urlEncodedKey } = router.query; + const idString = id as string; + + if (!idString) { + return { + id: "", + hashedHex: "", + key: null + }; + } + if (urlEncodedKey) { const [hashedHex, key] = urlEncodedKey ? urlEncodedKey.toString().split("-") : ["", ""]; return { - id: id as string, + id: idString, hashedHex, key }; } - // its like this {uuid}-{hex} so example: idpart1-idpart2-idpart3-idpart4-hex - const extractedId = id?.toString().split("-").slice(0, 5).join("-"); - const extractedHex = id?.toString().split("-").slice(5).join("-"); + // get the first 36 characters as id and the rest as hex + const idPart = idString.substring(0, 36); + const hexPart = idString.substring(36); return { - id: extractedId || "", - hashedHex: extractedHex || "", + id: idPart || "", + hashedHex: hexPart || "", key: null }; }; From 7e820745a4b1446f85f3b5077593d95f50fdd4c8 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Mon, 30 Sep 2024 18:43:48 +0400 Subject: [PATCH 4/9] Update 20240930134623_secret-sharing-string-id.ts --- .../db/migrations/20240930134623_secret-sharing-string-id.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts index 6cd1979151..69b8f7a83a 100644 --- a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts +++ b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts @@ -63,6 +63,9 @@ export async function down(knex: Knex): Promise { // Set the new column as primary key t.primary(["id"]); + + // Set the id column to use a UUID default + t.uuid("id").defaultTo(knex.raw("gen_random_uuid()")).notNullable().alter(); }); } } From 1a0896475cf9d2af5cd03b85159ccbb2fe0a3d52 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Mon, 30 Sep 2024 19:03:21 +0400 Subject: [PATCH 5/9] fix: added new identifier field for non-uuid IDs --- ...20240930134623_secret-sharing-string-id.ts | 58 ++---------------- backend/src/db/schemas/secret-sharing.ts | 3 +- .../secret-sharing/secret-sharing-dal.ts | 12 +--- .../secret-sharing/secret-sharing-service.ts | 60 +++++++++++++++---- 4 files changed, 55 insertions(+), 78 deletions(-) diff --git a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts index 69b8f7a83a..10d0da6607 100644 --- a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts +++ b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/ban-ts-comment */ import { Knex } from "knex"; import { TableName } from "../schemas"; @@ -6,31 +5,10 @@ import { TableName } from "../schemas"; export async function up(knex: Knex): Promise { if (await knex.schema.hasTable(TableName.SecretSharing)) { await knex.schema.alterTable(TableName.SecretSharing, (t) => { - // Add a new column - t.string("new_id", 36).nullable(); - }); + t.string("identifier", 36).nullable(); - // Copy data from old column to new column - await knex(TableName.SecretSharing).update({ - // @ts-ignore - new_id: knex.raw("id::text") - }); - - await knex.schema.alterTable(TableName.SecretSharing, (t) => { - // Make the new column not nullable - t.string("new_id", 36).notNullable().alter(); - - // Drop the old primary key - t.dropPrimary(); - - // Drop the old id column - t.dropColumn("id"); - - // Rename the new column to 'id' - t.renameColumn("new_id", "id"); - - // Set the new column as primary key - t.primary(["id"]); + t.unique("identifier"); + t.index("identifier"); }); } } @@ -38,34 +16,8 @@ export async function up(knex: Knex): Promise { export async function down(knex: Knex): Promise { if (await knex.schema.hasTable(TableName.SecretSharing)) { await knex.schema.alterTable(TableName.SecretSharing, (t) => { - // Add a new UUID column - t.uuid("new_id").nullable(); - }); - - // Copy data from string id to UUID, ensuring valid UUID format - await knex(TableName.SecretSharing).update({ - // @ts-ignore - new_id: knex.raw("id::uuid") - }); - - await knex.schema.alterTable(TableName.SecretSharing, (t) => { - // Make the new column not nullable - t.uuid("new_id").notNullable().alter(); - - // Drop the old primary key - t.dropPrimary(); - - // Drop the old id column - t.dropColumn("id"); - - // Rename the new column to 'id' - t.renameColumn("new_id", "id"); - - // Set the new column as primary key - t.primary(["id"]); - - // Set the id column to use a UUID default - t.uuid("id").defaultTo(knex.raw("gen_random_uuid()")).notNullable().alter(); + // If rolled back, all secrets created with this new structure will stop working. + t.dropColumn("identifier"); }); } } diff --git a/backend/src/db/schemas/secret-sharing.ts b/backend/src/db/schemas/secret-sharing.ts index 7490269d79..0b78b036f0 100644 --- a/backend/src/db/schemas/secret-sharing.ts +++ b/backend/src/db/schemas/secret-sharing.ts @@ -10,6 +10,7 @@ import { zodBuffer } from "@app/lib/zod"; import { TImmutableDBKeys } from "./models"; export const SecretSharingSchema = z.object({ + id: z.string().uuid(), encryptedValue: z.string().nullable().optional(), iv: z.string().nullable().optional(), tag: z.string().nullable().optional(), @@ -25,7 +26,7 @@ export const SecretSharingSchema = z.object({ lastViewedAt: z.date().nullable().optional(), password: z.string().nullable().optional(), encryptedSecret: zodBuffer.nullable().optional(), - id: z.string() + identifier: z.string().nullable().optional() }); export type TSecretSharing = z.infer; diff --git a/backend/src/services/secret-sharing/secret-sharing-dal.ts b/backend/src/services/secret-sharing/secret-sharing-dal.ts index f6108b3ff1..5c690b2667 100644 --- a/backend/src/services/secret-sharing/secret-sharing-dal.ts +++ b/backend/src/services/secret-sharing/secret-sharing-dal.ts @@ -82,21 +82,11 @@ export const secretSharingDALFactory = (db: TDbClient) => { } }; - const create = async (data: Omit, tx?: Knex) => { - try { - const [res] = await (tx || db)(TableName.SecretSharing).insert(data).returning("*"); - return res; - } catch (error) { - throw new DatabaseError({ error, name: "Create Shared Secret" }); - } - }; - return { ...sharedSecretOrm, countAllUserOrgSharedSecrets, pruneExpiredSharedSecrets, softDeleteById, - findActiveSharedSecrets, - create + findActiveSharedSecrets }; }; diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 0aa89dda1e..3509fc87fd 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -1,6 +1,7 @@ import crypto from "node:crypto"; import bcrypt from "bcrypt"; +import { z } from "zod"; import { TSecretSharing } from "@app/db/schemas"; import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service"; @@ -27,6 +28,8 @@ type TSecretSharingServiceFactoryDep = { export type TSecretSharingServiceFactory = ReturnType; +const isUuidV4 = (uuid: string) => z.string().uuid().safeParse(uuid).success; + export const secretSharingServiceFactory = ({ permissionService, secretSharingDAL, @@ -76,7 +79,7 @@ export const secretSharingServiceFactory = ({ const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ - id, + identifier: id, iv: null, tag: null, encryptedValue: null, @@ -91,7 +94,7 @@ export const secretSharingServiceFactory = ({ accessType }); - return { id: `${newSharedSecret.id}${hashedHex}` }; + return { id: `${newSharedSecret.identifier}${hashedHex}` }; }; const createPublicSharedSecret = async ({ @@ -126,7 +129,7 @@ export const secretSharingServiceFactory = ({ const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ - id, + identifier: id, encryptedValue: null, iv: null, tag: null, @@ -139,7 +142,7 @@ export const secretSharingServiceFactory = ({ accessType }); - return { id: `${newSharedSecret.id}${hashedHex}` }; + return { id: `${newSharedSecret.identifier}${hashedHex}` }; }; const getSharedSecrets = async ({ @@ -183,22 +186,44 @@ export const secretSharingServiceFactory = ({ const $decrementSecretViewCount = async (sharedSecret: TSecretSharing, sharedSecretId: string) => { const { expiresAfterViews } = sharedSecret; + const isUuid = isUuidV4(sharedSecretId); + if (expiresAfterViews) { // decrement view count if view count expiry set - await secretSharingDAL.updateById(sharedSecretId, { $decr: { expiresAfterViews: 1 } }); + + if (isUuid) { + await secretSharingDAL.updateById(sharedSecretId, { $decr: { expiresAfterViews: 1 } }); + } else { + await secretSharingDAL.update({ identifier: sharedSecretId }, { $decr: { expiresAfterViews: 1 } }); + } } - await secretSharingDAL.updateById(sharedSecretId, { - lastViewedAt: new Date() - }); + if (isUuid) { + await secretSharingDAL.updateById(sharedSecretId, { + lastViewedAt: new Date() + }); + } else { + await secretSharingDAL.update( + { identifier: sharedSecretId }, + { + lastViewedAt: new Date() + } + ); + } }; - /** Get's passwordless secret. validates all secret's requested (must be fresh). */ + /** Get's password-less secret. validates all secret's requested (must be fresh). */ const getSharedSecretById = async ({ sharedSecretId, hashedHex, orgId, password }: TGetActiveSharedSecretByIdDTO) => { - const sharedSecret = await secretSharingDAL.findOne({ - id: sharedSecretId, - hashedHex - }); + const sharedSecret = isUuidV4(sharedSecretId) + ? await secretSharingDAL.findOne({ + id: sharedSecretId, + hashedHex + }) + : await secretSharingDAL.findOne({ + hashedHex, + identifier: sharedSecretId + }); + if (!sharedSecret) throw new NotFoundError({ message: "Shared secret not found" @@ -269,7 +294,16 @@ export const secretSharingServiceFactory = ({ const { actor, actorId, orgId, actorAuthMethod, actorOrgId, sharedSecretId } = deleteSharedSecretInput; const { permission } = await permissionService.getOrgPermission(actor, actorId, orgId, actorAuthMethod, actorOrgId); if (!permission) throw new ForbiddenRequestError({ name: "User does not belong to the specified organization" }); + const deletedSharedSecret = await secretSharingDAL.deleteById(sharedSecretId); + + const sharedSecret = isUuidV4(sharedSecretId) + ? await secretSharingDAL.findById(sharedSecretId) + : await secretSharingDAL.findOne({ identifier: sharedSecretId }); + + if (sharedSecret.orgId && sharedSecret.orgId !== orgId) + throw new ForbiddenRequestError({ message: "User does not have permission to delete shared secret" }); + return deletedSharedSecret; }; From 9c33251c444c3e536bce396f3da9886ed575ad96 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Mon, 30 Sep 2024 22:27:36 +0400 Subject: [PATCH 6/9] Update secret-sharing-service.ts --- backend/src/services/secret-sharing/secret-sharing-service.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 3509fc87fd..59908f97ec 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -75,7 +75,7 @@ export const secretSharingServiceFactory = ({ // This will be 36 characters long, due to encoding it to base64. const id = crypto.randomBytes(27).toString("base64url"); - const hashedHex = crypto.createHash("sha256").update(id).digest("base64").substring(0, 13); + const hashedHex = crypto.createHash("sha256").update(id).digest("base64url").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ @@ -125,7 +125,7 @@ export const secretSharingServiceFactory = ({ const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); const id = crypto.randomBytes(27).toString("base64url"); - const hashedHex = crypto.createHash("sha256").update(id).digest("hex").substring(0, 13); + const hashedHex = crypto.createHash("sha256").update(id).digest("base64url").substring(0, 13); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ From b65842f5c1068cbfeaa410cd7cb4cb646c912543 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Tue, 1 Oct 2024 00:16:18 +0400 Subject: [PATCH 7/9] fix: requested changes --- .../20240925100349_managed-secret-sharing.ts | 11 +++++---- ...20240930134623_secret-sharing-string-id.ts | 23 ------------------ backend/src/db/schemas/secret-sharing.ts | 2 +- .../server/routes/v1/secret-sharing-router.ts | 2 +- .../secret-sharing/secret-sharing-service.ts | 24 +++++++------------ .../secret-sharing/secret-sharing-types.ts | 2 +- .../src/hooks/api/secretSharing/queries.ts | 8 +++---- .../ViewSecretPublicPage.tsx | 19 ++++----------- 8 files changed, 28 insertions(+), 63 deletions(-) delete mode 100644 backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts diff --git a/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts b/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts index 25871455a8..56784d314e 100644 --- a/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts +++ b/backend/src/db/migrations/20240925100349_managed-secret-sharing.ts @@ -10,6 +10,11 @@ export async function up(knex: Knex): Promise { t.string("encryptedValue").nullable().alter(); t.binary("encryptedSecret").nullable(); + t.string("hashedHex").nullable().alter(); + + t.string("identifier", 64).nullable(); + t.unique("identifier"); + t.index("identifier"); }); } } @@ -17,11 +22,9 @@ export async function up(knex: Knex): Promise { export async function down(knex: Knex): Promise { if (await knex.schema.hasTable(TableName.SecretSharing)) { await knex.schema.alterTable(TableName.SecretSharing, (t) => { - t.string("iv").notNullable().alter(); - t.string("tag").notNullable().alter(); - t.string("encryptedValue").notNullable().alter(); - t.dropColumn("encryptedSecret"); + + t.dropColumn("identifier"); }); } } diff --git a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts b/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts deleted file mode 100644 index 10d0da6607..0000000000 --- a/backend/src/db/migrations/20240930134623_secret-sharing-string-id.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { Knex } from "knex"; - -import { TableName } from "../schemas"; - -export async function up(knex: Knex): Promise { - if (await knex.schema.hasTable(TableName.SecretSharing)) { - await knex.schema.alterTable(TableName.SecretSharing, (t) => { - t.string("identifier", 36).nullable(); - - t.unique("identifier"); - t.index("identifier"); - }); - } -} - -export async function down(knex: Knex): Promise { - if (await knex.schema.hasTable(TableName.SecretSharing)) { - await knex.schema.alterTable(TableName.SecretSharing, (t) => { - // If rolled back, all secrets created with this new structure will stop working. - t.dropColumn("identifier"); - }); - } -} diff --git a/backend/src/db/schemas/secret-sharing.ts b/backend/src/db/schemas/secret-sharing.ts index 0b78b036f0..d47f288b22 100644 --- a/backend/src/db/schemas/secret-sharing.ts +++ b/backend/src/db/schemas/secret-sharing.ts @@ -14,7 +14,7 @@ export const SecretSharingSchema = z.object({ encryptedValue: z.string().nullable().optional(), iv: z.string().nullable().optional(), tag: z.string().nullable().optional(), - hashedHex: z.string(), + hashedHex: z.string().nullable().optional(), expiresAt: z.date(), userId: z.string().uuid().nullable().optional(), orgId: z.string().uuid().nullable().optional(), diff --git a/backend/src/server/routes/v1/secret-sharing-router.ts b/backend/src/server/routes/v1/secret-sharing-router.ts index 2149931385..fa4e9bf6ad 100644 --- a/backend/src/server/routes/v1/secret-sharing-router.ts +++ b/backend/src/server/routes/v1/secret-sharing-router.ts @@ -58,7 +58,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => id: z.string() }), body: z.object({ - hashedHex: z.string().min(1), + hashedHex: z.string().min(1).optional(), password: z.string().optional() }), response: { diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 59908f97ec..9f24043706 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -72,10 +72,7 @@ export const secretSharingServiceFactory = ({ const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); - // This will be 36 characters long, due to encoding it to base64. - const id = crypto.randomBytes(27).toString("base64url"); - - const hashedHex = crypto.createHash("sha256").update(id).digest("base64url").substring(0, 13); + const id = crypto.randomBytes(32).toString("hex"); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ @@ -84,7 +81,6 @@ export const secretSharingServiceFactory = ({ tag: null, encryptedValue: null, encryptedSecret, - hashedHex, name, password: hashedPassword, expiresAt: new Date(expiresAt), @@ -94,7 +90,9 @@ export const secretSharingServiceFactory = ({ accessType }); - return { id: `${newSharedSecret.identifier}${hashedHex}` }; + const idToReturn = `${Buffer.from(newSharedSecret.identifier!, "hex").toString("base64url")}`; + + return { id: idToReturn }; }; const createPublicSharedSecret = async ({ @@ -124,8 +122,7 @@ export const secretSharingServiceFactory = ({ const encryptWithRoot = kmsService.encryptWithRootKey(); const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); - const id = crypto.randomBytes(27).toString("base64url"); - const hashedHex = crypto.createHash("sha256").update(id).digest("base64url").substring(0, 13); + const id = crypto.randomBytes(32).toString("hex"); const hashedPassword = password ? await bcrypt.hash(password, 10) : null; const newSharedSecret = await secretSharingDAL.create({ @@ -133,16 +130,14 @@ export const secretSharingServiceFactory = ({ encryptedValue: null, iv: null, tag: null, - hashedHex, encryptedSecret, - password: hashedPassword, expiresAt: new Date(expiresAt), expiresAfterViews, accessType }); - return { id: `${newSharedSecret.identifier}${hashedHex}` }; + return { id: `${Buffer.from(newSharedSecret.identifier!, "hex").toString("base64url")}` }; }; const getSharedSecrets = async ({ @@ -220,8 +215,7 @@ export const secretSharingServiceFactory = ({ hashedHex }) : await secretSharingDAL.findOne({ - hashedHex, - identifier: sharedSecretId + identifier: Buffer.from(sharedSecretId, "base64url").toString("hex") }); if (!sharedSecret) @@ -295,12 +289,12 @@ export const secretSharingServiceFactory = ({ const { permission } = await permissionService.getOrgPermission(actor, actorId, orgId, actorAuthMethod, actorOrgId); if (!permission) throw new ForbiddenRequestError({ name: "User does not belong to the specified organization" }); - const deletedSharedSecret = await secretSharingDAL.deleteById(sharedSecretId); - const sharedSecret = isUuidV4(sharedSecretId) ? await secretSharingDAL.findById(sharedSecretId) : await secretSharingDAL.findOne({ identifier: sharedSecretId }); + const deletedSharedSecret = await secretSharingDAL.deleteById(sharedSecretId); + if (sharedSecret.orgId && sharedSecret.orgId !== orgId) throw new ForbiddenRequestError({ message: "User does not have permission to delete shared secret" }); diff --git a/backend/src/services/secret-sharing/secret-sharing-types.ts b/backend/src/services/secret-sharing/secret-sharing-types.ts index 9469e8de5c..1d9efa1e3a 100644 --- a/backend/src/services/secret-sharing/secret-sharing-types.ts +++ b/backend/src/services/secret-sharing/secret-sharing-types.ts @@ -28,7 +28,7 @@ export type TCreatePublicSharedSecretDTO = { export type TGetActiveSharedSecretByIdDTO = { sharedSecretId: string; - hashedHex: string; + hashedHex?: string; orgId?: string; password?: string; }; diff --git a/frontend/src/hooks/api/secretSharing/queries.ts b/frontend/src/hooks/api/secretSharing/queries.ts index c349d39f56..479f3ec165 100644 --- a/frontend/src/hooks/api/secretSharing/queries.ts +++ b/frontend/src/hooks/api/secretSharing/queries.ts @@ -8,7 +8,7 @@ export const secretSharingKeys = { allSharedSecrets: () => ["sharedSecrets"] as const, specificSharedSecrets: ({ offset, limit }: { offset: number; limit: number }) => [...secretSharingKeys.allSharedSecrets(), { offset, limit }] as const, - getSecretById: (arg: { id: string; hashedHex: string; password?: string }) => [ + getSecretById: (arg: { id: string; hashedHex: string | null; password?: string }) => [ "shared-secret", arg ] @@ -46,7 +46,7 @@ export const useGetActiveSharedSecretById = ({ password }: { sharedSecretId: string; - hashedHex: string; + hashedHex: string | null; password?: string; }) => { return useQuery( @@ -55,7 +55,7 @@ export const useGetActiveSharedSecretById = ({ const { data } = await apiRequest.post( `/api/v1/secret-sharing/public/${sharedSecretId}`, { - hashedHex, + ...(hashedHex && { hashedHex }), password } ); @@ -63,7 +63,7 @@ export const useGetActiveSharedSecretById = ({ return data; }, { - enabled: Boolean(sharedSecretId) && Boolean(hashedHex) + enabled: Boolean(sharedSecretId) } ); }; diff --git a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx index f2309860bd..95475ad4a5 100644 --- a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx +++ b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx @@ -15,14 +15,6 @@ const extractDetailsFromUrl = (router: NextRouter) => { const idString = id as string; - if (!idString) { - return { - id: "", - hashedHex: "", - key: null - }; - } - if (urlEncodedKey) { const [hashedHex, key] = urlEncodedKey ? urlEncodedKey.toString().split("-") : ["", ""]; @@ -33,13 +25,9 @@ const extractDetailsFromUrl = (router: NextRouter) => { }; } - // get the first 36 characters as id and the rest as hex - const idPart = idString.substring(0, 36); - const hexPart = idString.substring(36); - return { - id: idPart || "", - hashedHex: hexPart || "", + id: idString, + hashedHex: null, key: null }; }; @@ -65,6 +53,9 @@ export const ViewSecretPublicPage = () => { ((error as AxiosError)?.response?.data as { message: string })?.message === "Invalid credentials"; + console.log("data", fetchSecret); + console.log("err", error); + const shouldShowPasswordPrompt = isInvalidCredential || (fetchSecret?.isPasswordProtected && !fetchSecret.secret); const isValidatingPassword = Boolean(password) && isFetching; From 97c2b15e298eb422ea45720700526b2b1c68b301 Mon Sep 17 00:00:00 2001 From: Daniel Hougaard Date: Wed, 2 Oct 2024 15:20:06 +0400 Subject: [PATCH 8/9] fix: secret sharing view count --- .../secret-sharing/secret-sharing-service.ts | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index 9f24043706..b427844d0d 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -178,33 +178,17 @@ export const secretSharingServiceFactory = ({ }; }; - const $decrementSecretViewCount = async (sharedSecret: TSecretSharing, sharedSecretId: string) => { + const $decrementSecretViewCount = async (sharedSecret: TSecretSharing) => { const { expiresAfterViews } = sharedSecret; - const isUuid = isUuidV4(sharedSecretId); - if (expiresAfterViews) { // decrement view count if view count expiry set - - if (isUuid) { - await secretSharingDAL.updateById(sharedSecretId, { $decr: { expiresAfterViews: 1 } }); - } else { - await secretSharingDAL.update({ identifier: sharedSecretId }, { $decr: { expiresAfterViews: 1 } }); - } + await secretSharingDAL.updateById(sharedSecret.id, { $decr: { expiresAfterViews: 1 } }); } - if (isUuid) { - await secretSharingDAL.updateById(sharedSecretId, { - lastViewedAt: new Date() - }); - } else { - await secretSharingDAL.update( - { identifier: sharedSecretId }, - { - lastViewedAt: new Date() - } - ); - } + await secretSharingDAL.updateById(sharedSecret.id, { + lastViewedAt: new Date() + }); }; /** Get's password-less secret. validates all secret's requested (must be fresh). */ @@ -267,7 +251,7 @@ export const secretSharingServiceFactory = ({ } // decrement when we are sure the user will view secret. - await $decrementSecretViewCount(sharedSecret, sharedSecretId); + await $decrementSecretViewCount(sharedSecret); return { isPasswordProtected, From 2273c21eb26c49a3e782378bf8c455e5eaa4c0aa Mon Sep 17 00:00:00 2001 From: Tuan Dang Date: Wed, 2 Oct 2024 09:10:22 -0400 Subject: [PATCH 9/9] Clean PR --- backend/src/server/routes/v1/secret-sharing-router.ts | 2 +- .../src/services/secret-sharing/secret-sharing-service.ts | 5 ----- .../src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx | 3 --- 3 files changed, 1 insertion(+), 9 deletions(-) diff --git a/backend/src/server/routes/v1/secret-sharing-router.ts b/backend/src/server/routes/v1/secret-sharing-router.ts index fa4e9bf6ad..3363cc6c0a 100644 --- a/backend/src/server/routes/v1/secret-sharing-router.ts +++ b/backend/src/server/routes/v1/secret-sharing-router.ts @@ -100,7 +100,7 @@ export const registerSecretSharingRouter = async (server: FastifyZodProvider) => }, schema: { body: z.object({ - secretValue: z.string(), + secretValue: z.string().max(10_000), password: z.string().optional(), expiresAt: z.string(), expiresAfterViews: z.number().min(1).optional() diff --git a/backend/src/services/secret-sharing/secret-sharing-service.ts b/backend/src/services/secret-sharing/secret-sharing-service.ts index b427844d0d..0f7ee20b40 100644 --- a/backend/src/services/secret-sharing/secret-sharing-service.ts +++ b/backend/src/services/secret-sharing/secret-sharing-service.ts @@ -114,11 +114,6 @@ export const secretSharingServiceFactory = ({ throw new BadRequestError({ message: "Expiration date cannot exceed more than 30 days" }); } - // Limit Input ciphertext length to 13000 (equivalent to 10,000 characters of Plaintext)n - if (secretValue.length > 10_000) { - throw new BadRequestError({ message: "Shared secret value too long" }); - } - const encryptWithRoot = kmsService.encryptWithRootKey(); const encryptedSecret = encryptWithRoot(Buffer.from(secretValue)); diff --git a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx index 95475ad4a5..cc2cc16bab 100644 --- a/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx +++ b/frontend/src/views/ViewSecretPublicPage/ViewSecretPublicPage.tsx @@ -53,9 +53,6 @@ export const ViewSecretPublicPage = () => { ((error as AxiosError)?.response?.data as { message: string })?.message === "Invalid credentials"; - console.log("data", fetchSecret); - console.log("err", error); - const shouldShowPasswordPrompt = isInvalidCredential || (fetchSecret?.isPasswordProtected && !fetchSecret.secret); const isValidatingPassword = Boolean(password) && isFetching;