fix(security): remove salt from database (#5412)
This commit is contained in:
1 parent
41a03ef4e0
commit
2454cac7be
24 files changed
+6198
-81
No files matched your search
@@ -1,6 +1,6 @@
|
||||
import { z } from "zod/v4";
|
||||
|
||||
import { createSaltAsync, hashPasswordAsync } from "@homarr/auth";
|
||||
import { hashPasswordAsync } from "@homarr/auth";
|
||||
import { createId } from "@homarr/common";
|
||||
import { generateSecureRandomToken } from "@homarr/common/server";
|
||||
import { db, eq } from "@homarr/db";
|
||||
@@ -14,7 +14,6 @@ export const apiKeysRouter = createTRPCRouter({
|
||||
columns: {
|
||||
id: true,
|
||||
apiKey: false,
|
||||
salt: false,
|
||||
},
|
||||
with: {
|
||||
user: {
|
||||
@@ -29,14 +28,12 @@ export const apiKeysRouter = createTRPCRouter({
|
||||
});
|
||||
}),
|
||||
create: permissionRequiredProcedure.requiresPermission("admin").mutation(async ({ ctx }) => {
|
||||
const salt = await createSaltAsync();
|
||||
const randomToken = generateSecureRandomToken(64);
|
||||
const hashedRandomToken = await hashPasswordAsync(randomToken, salt);
|
||||
const hashedRandomToken = await hashPasswordAsync(randomToken);
|
||||
const id = createId();
|
||||
await db.insert(apiKeys).values({
|
||||
id,
|
||||
apiKey: hashedRandomToken,
|
||||
salt,
|
||||
userId: ctx.session.user.id,
|
||||
});
|
||||
return {
|
||||
|
||||
@@ -3,7 +3,7 @@ import type { MySqlRawQueryResult } from "drizzle-orm/mysql2";
|
||||
import type { QueryResult } from "pg";
|
||||
import { z } from "zod/v4";
|
||||
|
||||
import { createSaltAsync, hashPasswordAsync } from "@homarr/auth";
|
||||
import { comparePasswordsAsync, hashPasswordAsync } from "@homarr/auth";
|
||||
import { createId } from "@homarr/common";
|
||||
import { createLogger } from "@homarr/core/infrastructure/logs";
|
||||
import type { Database } from "@homarr/db";
|
||||
@@ -11,8 +11,8 @@ import { and, eq, handleTransactionsAsync, like } from "@homarr/db";
|
||||
import { getMaxGroupPositionAsync } from "@homarr/db/queries";
|
||||
import { boards, groupMembers, groupPermissions, groups, invites, users } from "@homarr/db/schema";
|
||||
import { selectUserSchema } from "@homarr/db/validationSchemas";
|
||||
import { credentialsAdminGroup } from "@homarr/definitions";
|
||||
import type { SupportedAuthProvider } from "@homarr/definitions";
|
||||
import { credentialsAdminGroup } from "@homarr/definitions";
|
||||
import { byIdSchema } from "@homarr/validation/common";
|
||||
import type { userBaseCreateSchema } from "@homarr/validation/user";
|
||||
import {
|
||||
@@ -92,15 +92,13 @@ export const userRouter = createTRPCRouter({
|
||||
|
||||
await checkUsernameAlreadyTakenAndThrowAsync(ctx.db, "credentials", input.username);
|
||||
|
||||
const salt = await createSaltAsync();
|
||||
const hashedPassword = await hashPasswordAsync(input.password, salt);
|
||||
const hashedPassword = await hashPasswordAsync(input.password);
|
||||
|
||||
const userId = createId();
|
||||
const user = {
|
||||
id: userId,
|
||||
name: input.username,
|
||||
password: hashedPassword,
|
||||
salt,
|
||||
};
|
||||
|
||||
await handleTransactionsAsync(ctx.db, {
|
||||
@@ -393,7 +391,6 @@ export const userRouter = createTRPCRouter({
|
||||
columns: {
|
||||
id: true,
|
||||
password: true,
|
||||
salt: true,
|
||||
provider: true,
|
||||
},
|
||||
where: eq(users.id, input.userId),
|
||||
@@ -423,8 +420,7 @@ export const userRouter = createTRPCRouter({
|
||||
});
|
||||
|
||||
if (isPreviousPasswordRequired) {
|
||||
const previousPasswordHash = await hashPasswordAsync(input.previousPassword, dbUser.salt ?? "");
|
||||
const isValid = previousPasswordHash === dbUser.password;
|
||||
const isValid = await comparePasswordsAsync(input.previousPassword, dbUser.password ?? "");
|
||||
|
||||
if (!isValid) {
|
||||
throw new TRPCError({
|
||||
@@ -434,8 +430,7 @@ export const userRouter = createTRPCRouter({
|
||||
}
|
||||
}
|
||||
|
||||
const salt = await createSaltAsync();
|
||||
const hashedPassword = await hashPasswordAsync(input.password, salt);
|
||||
const hashedPassword = await hashPasswordAsync(input.password);
|
||||
await ctx.db
|
||||
.update(users)
|
||||
.set({
|
||||
@@ -583,8 +578,7 @@ export const userRouter = createTRPCRouter({
|
||||
});
|
||||
|
||||
const createUserAsync = async (db: Database, input: Omit<z.infer<typeof userBaseCreateSchema>, "groupIds">) => {
|
||||
const salt = await createSaltAsync();
|
||||
const hashedPassword = await hashPasswordAsync(input.password, salt);
|
||||
const hashedPassword = await hashPasswordAsync(input.password);
|
||||
|
||||
const userId = createId();
|
||||
await db.insert(users).values({
|
||||
@@ -592,7 +586,6 @@ const createUserAsync = async (db: Database, input: Omit<z.infer<typeof userBase
|
||||
name: input.username,
|
||||
email: input.email,
|
||||
password: hashedPassword,
|
||||
salt,
|
||||
});
|
||||
return userId;
|
||||
};
|
||||
|
||||
@@ -5,7 +5,7 @@ import type { Database } from "@homarr/db";
|
||||
import { eq } from "@homarr/db";
|
||||
import { apiKeys } from "@homarr/db/schema";
|
||||
|
||||
import { hashPasswordAsync } from "../security";
|
||||
import { comparePasswordsAsync } from "../security";
|
||||
import { createSessionAsync } from "../server";
|
||||
|
||||
const logger = createLogger({ module: "apiKeyAuth" });
|
||||
@@ -41,7 +41,6 @@ export const getSessionFromApiKeyAsync = async (
|
||||
columns: {
|
||||
id: true,
|
||||
apiKey: true,
|
||||
salt: true,
|
||||
},
|
||||
with: {
|
||||
user: {
|
||||
@@ -60,9 +59,9 @@ export const getSessionFromApiKeyAsync = async (
|
||||
return null;
|
||||
}
|
||||
|
||||
const hashedApiKey = await hashPasswordAsync(apiKey, apiKeyFromDb.salt);
|
||||
const isValid = await comparePasswordsAsync(apiKey, apiKeyFromDb.apiKey);
|
||||
|
||||
if (apiKeyFromDb.apiKey !== hashedApiKey) {
|
||||
if (!isValid) {
|
||||
logger.warn("Failed to authenticate with api-key", { ipAddress, userAgent, reason: "API_KEY_MISMATCH" });
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -5,7 +5,7 @@ import { createId } from "@homarr/common";
|
||||
import { apiKeys, users } from "@homarr/db/schema";
|
||||
import { createDb } from "@homarr/db/test";
|
||||
|
||||
import { createSaltAsync, hashPasswordAsync } from "../../security";
|
||||
import { hashPasswordAsync } from "../../security";
|
||||
import { getSessionFromApiKeyAsync } from "../get-api-key-session";
|
||||
|
||||
// Mock the logger to avoid console output during tests
|
||||
@@ -118,11 +118,9 @@ const setupAsync = async (options?: SetupOptions) => {
|
||||
});
|
||||
|
||||
if (options?.token) {
|
||||
const salt = await createSaltAsync();
|
||||
await db.insert(apiKeys).values({
|
||||
id: defaultApiKeyId,
|
||||
apiKey: await hashPasswordAsync(options.token, salt),
|
||||
salt,
|
||||
apiKey: await hashPasswordAsync(options.token),
|
||||
userId: defaultUserId,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
import bcrypt from "bcrypt";
|
||||
import type { z } from "zod/v4";
|
||||
|
||||
import { createLogger } from "@homarr/core/infrastructure/logs";
|
||||
@@ -7,6 +6,8 @@ import { and, eq } from "@homarr/db";
|
||||
import { users } from "@homarr/db/schema";
|
||||
import type { userSignInSchema } from "@homarr/validation/user";
|
||||
|
||||
import { comparePasswordsAsync } from "../../../security";
|
||||
|
||||
const logger = createLogger({ module: "basicAuthorization" });
|
||||
|
||||
export const authorizeWithBasicCredentialsAsync = async (
|
||||
@@ -23,7 +24,7 @@ export const authorizeWithBasicCredentialsAsync = async (
|
||||
}
|
||||
|
||||
logger.info("User is trying to log in. Checking password...", { userName: user.name });
|
||||
const isValidPassword = await bcrypt.compare(credentials.password, user.password);
|
||||
const isValidPassword = await comparePasswordsAsync(credentials.password, user.password);
|
||||
|
||||
if (!isValidPassword) {
|
||||
logger.warn("Password for user was incorrect", { userName: user.name });
|
||||
|
||||
@@ -4,7 +4,7 @@ import { createId } from "@homarr/common";
|
||||
import { users } from "@homarr/db/schema";
|
||||
import { createDb } from "@homarr/db/test";
|
||||
|
||||
import { createSaltAsync, hashPasswordAsync } from "../../security";
|
||||
import { hashPasswordAsync } from "../../security";
|
||||
import { authorizeWithBasicCredentialsAsync } from "../credentials/authorization/basic-authorization";
|
||||
|
||||
const defaultUserId = createId();
|
||||
@@ -13,12 +13,10 @@ describe("authorizeWithBasicCredentials", () => {
|
||||
test("should authorize user with correct credentials", async () => {
|
||||
// Arrange
|
||||
const db = createDb();
|
||||
const salt = await createSaltAsync();
|
||||
await db.insert(users).values({
|
||||
id: defaultUserId,
|
||||
name: "test",
|
||||
salt,
|
||||
password: await hashPasswordAsync("test", salt),
|
||||
password: await hashPasswordAsync("test"),
|
||||
});
|
||||
|
||||
// Act
|
||||
@@ -34,12 +32,10 @@ describe("authorizeWithBasicCredentials", () => {
|
||||
test("should not authorize user with incorrect credentials", async () => {
|
||||
// Arrange
|
||||
const db = createDb();
|
||||
const salt = await createSaltAsync();
|
||||
await db.insert(users).values({
|
||||
id: defaultUserId,
|
||||
name: "test",
|
||||
salt,
|
||||
password: await hashPasswordAsync("test", salt),
|
||||
password: await hashPasswordAsync("test"),
|
||||
});
|
||||
|
||||
// Act
|
||||
@@ -55,12 +51,10 @@ describe("authorizeWithBasicCredentials", () => {
|
||||
test("should not authorize user with incorrect username", async () => {
|
||||
// Arrange
|
||||
const db = createDb();
|
||||
const salt = await createSaltAsync();
|
||||
await db.insert(users).values({
|
||||
id: defaultUserId,
|
||||
name: "test",
|
||||
salt,
|
||||
password: await hashPasswordAsync("test", salt),
|
||||
password: await hashPasswordAsync("test"),
|
||||
});
|
||||
|
||||
// Act
|
||||
|
||||
@@ -1,9 +1,15 @@
|
||||
import bcrypt from "bcrypt";
|
||||
|
||||
export const createSaltAsync = async () => {
|
||||
return bcrypt.genSalt(10);
|
||||
export const hashPasswordAsync = async (password: string) => {
|
||||
return bcrypt.hash(password, 10);
|
||||
};
|
||||
|
||||
export const hashPasswordAsync = async (password: string, salt: string) => {
|
||||
return bcrypt.hash(password, salt);
|
||||
/**
|
||||
* Compares a plaintext password with a hashed password and returns whether they match.
|
||||
* @param password plaintext password
|
||||
* @param hash hashed password
|
||||
* @returns wheter it is the same
|
||||
*/
|
||||
export const comparePasswordsAsync = async (password: string, hash: string) => {
|
||||
return await bcrypt.compare(password, hash);
|
||||
};
|
||||
@@ -1,25 +1,11 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import { createSaltAsync, hashPasswordAsync } from "../security";
|
||||
|
||||
describe("createSalt should return a salt", () => {
|
||||
it("should return a salt", async () => {
|
||||
const result = await createSaltAsync();
|
||||
expect(result).toBeDefined();
|
||||
expect(result.length).toBeGreaterThan(25);
|
||||
});
|
||||
it("should return a different salt each time", async () => {
|
||||
const result1 = await createSaltAsync();
|
||||
const result2 = await createSaltAsync();
|
||||
expect(result1).not.toEqual(result2);
|
||||
});
|
||||
});
|
||||
import { comparePasswordsAsync, hashPasswordAsync } from "../security";
|
||||
|
||||
describe("hashPassword should return a hash", () => {
|
||||
it("should return a hash", async () => {
|
||||
const password = "password";
|
||||
const salt = await createSaltAsync();
|
||||
const result = await hashPasswordAsync(password, salt);
|
||||
const result = await hashPasswordAsync(password);
|
||||
expect(result).toBeDefined();
|
||||
expect(result.length).toBeGreaterThan(55);
|
||||
expect(result).not.toEqual(password);
|
||||
@@ -27,21 +13,44 @@ describe("hashPassword should return a hash", () => {
|
||||
it("should return a different hash each time", async () => {
|
||||
const password = "password";
|
||||
const password2 = "another password";
|
||||
const salt = await createSaltAsync();
|
||||
|
||||
const result1 = await hashPasswordAsync(password, salt);
|
||||
const result2 = await hashPasswordAsync(password2, salt);
|
||||
const result1 = await hashPasswordAsync(password);
|
||||
const result2 = await hashPasswordAsync(password2);
|
||||
|
||||
expect(result1).not.toEqual(result2);
|
||||
});
|
||||
it("should return a different hash for the same password with different salts", async () => {
|
||||
const password = "password";
|
||||
const salt1 = await createSaltAsync();
|
||||
const salt2 = await createSaltAsync();
|
||||
|
||||
const result1 = await hashPasswordAsync(password, salt1);
|
||||
const result2 = await hashPasswordAsync(password, salt2);
|
||||
const result1 = await hashPasswordAsync(password);
|
||||
const result2 = await hashPasswordAsync(password);
|
||||
|
||||
expect(result1).not.toEqual(result2);
|
||||
});
|
||||
});
|
||||
|
||||
describe("comparePasswordsAsync should compare passwords correctly", () => {
|
||||
it("should return true for matching passwords", async () => {
|
||||
// Arrange
|
||||
const password = "password";
|
||||
const hash = await hashPasswordAsync(password);
|
||||
|
||||
// Act
|
||||
const result = await comparePasswordsAsync(password, hash);
|
||||
|
||||
// Assert
|
||||
expect(result).toBe(true);
|
||||
});
|
||||
it("should return false for non-matching passwords", async () => {
|
||||
// Arrange
|
||||
const password = "password";
|
||||
const wrongPassword = "wrongpassword";
|
||||
const hash = await hashPasswordAsync(password);
|
||||
|
||||
// Act
|
||||
const result = await comparePasswordsAsync(wrongPassword, hash);
|
||||
|
||||
// Assert
|
||||
expect(result).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,6 @@
|
||||
import { command, string } from "@drizzle-team/brocli";
|
||||
|
||||
import { createSaltAsync, hashPasswordAsync } from "@homarr/auth";
|
||||
import { hashPasswordAsync } from "@homarr/auth";
|
||||
import { createId } from "@homarr/common";
|
||||
import { generateSecureRandomToken } from "@homarr/common/server";
|
||||
import { and, count, db, eq } from "@homarr/db";
|
||||
@@ -67,9 +67,8 @@ export const recreateAdmin = command({
|
||||
permission: "admin",
|
||||
});
|
||||
|
||||
const salt = await createSaltAsync();
|
||||
const password = generateSecureRandomToken(24);
|
||||
const hashedPassword = await hashPasswordAsync(password, salt);
|
||||
const hashedPassword = await hashPasswordAsync(password);
|
||||
|
||||
const userId = createId();
|
||||
await db.insert(users).values({
|
||||
@@ -77,7 +76,6 @@ export const recreateAdmin = command({
|
||||
name: result.data,
|
||||
provider: "credentials",
|
||||
password: hashedPassword,
|
||||
salt,
|
||||
});
|
||||
|
||||
await db.insert(groupMembers).values({
|
||||
|
||||
@@ -22,7 +22,7 @@ export const resetPassword = command({
|
||||
where: and(eq(users.name, options.username), eq(users.provider, "credentials")),
|
||||
});
|
||||
|
||||
if (!user?.salt) {
|
||||
if (!user?.password) {
|
||||
console.error(`User ${options.username} not found`);
|
||||
return;
|
||||
}
|
||||
@@ -33,7 +33,7 @@ export const resetPassword = command({
|
||||
await db
|
||||
.update(users)
|
||||
.set({
|
||||
password: await hashPasswordAsync(newPassword, user.salt),
|
||||
password: await hashPasswordAsync(newPassword),
|
||||
})
|
||||
.where(eq(users.id, user.id));
|
||||
|
||||
|
||||
@@ -29,7 +29,7 @@ export const usersUpdatePassword = command({
|
||||
where: options.id ? eq(users.id, options.id) : eq(users.name, options.username!),
|
||||
});
|
||||
|
||||
if (!user?.salt) {
|
||||
if (!user?.password) {
|
||||
console.error("User not found or has no credentials record");
|
||||
return;
|
||||
}
|
||||
@@ -37,7 +37,7 @@ export const usersUpdatePassword = command({
|
||||
await db
|
||||
.update(users)
|
||||
.set({
|
||||
password: await hashPasswordAsync(options.password, user.salt),
|
||||
password: await hashPasswordAsync(options.password),
|
||||
})
|
||||
.where(eq(users.id, user.id));
|
||||
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
ALTER TABLE `apiKey` DROP COLUMN `salt`;--> statement-breakpoint
|
||||
ALTER TABLE `user` DROP COLUMN `salt`;
|
||||
File diff suppressed because it is too large.
Load diff
@@ -260,6 +260,13 @@
|
||||
"when": 1760968518445,
|
||||
"tag": "0036_add_app_reference_to_integration",
|
||||
"breakpoints": true
|
||||
},
|
||||
{
|
||||
"idx": 37,
|
||||
"version": "5",
|
||||
"when": 1775399941430,
|
||||
"tag": "0037_remove_salt",
|
||||
"breakpoints": true
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,2 @@
|
||||
ALTER TABLE "apiKey" DROP COLUMN "salt";--> statement-breakpoint
|
||||
ALTER TABLE "user" DROP COLUMN "salt";
|
||||
File diff suppressed because it is too large.
Load diff
@@ -22,6 +22,13 @@
|
||||
"when": 1760968530084,
|
||||
"tag": "0002_add_app_reference_to_integration",
|
||||
"breakpoints": true
|
||||
},
|
||||
{
|
||||
"idx": 3,
|
||||
"version": "7",
|
||||
"when": 1775399963041,
|
||||
"tag": "0003_remove_salt",
|
||||
"breakpoints": true
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -0,0 +1,2 @@
|
||||
ALTER TABLE `apiKey` DROP COLUMN `salt`;--> statement-breakpoint
|
||||
ALTER TABLE `user` DROP COLUMN `salt`;
|
||||
File diff suppressed because it is too large.
Load diff
@@ -246,6 +246,13 @@
|
||||
"when": 1760968503571,
|
||||
"tag": "0034_add_app_reference_to_integration",
|
||||
"breakpoints": true
|
||||
},
|
||||
{
|
||||
"idx": 35,
|
||||
"version": "6",
|
||||
"when": 1775399918831,
|
||||
"tag": "0035_remove_salt",
|
||||
"breakpoints": true
|
||||
}
|
||||
]
|
||||
}
|
||||
@@ -51,7 +51,6 @@ export * from "@homarr/core/infrastructure/certificates/hostnames/db/mysql";
|
||||
export const apiKeys = mysqlTable("apiKey", {
|
||||
id: varchar({ length: 64 }).notNull().primaryKey(),
|
||||
apiKey: text().notNull(),
|
||||
salt: text().notNull(),
|
||||
userId: varchar({ length: 64 })
|
||||
.notNull()
|
||||
.references((): AnyMySqlColumn => users.id, {
|
||||
@@ -66,7 +65,6 @@ export const users = mysqlTable("user", {
|
||||
emailVerified: timestamp(),
|
||||
image: text(),
|
||||
password: text(),
|
||||
salt: text(),
|
||||
provider: varchar({ length: 64 }).$type<SupportedAuthProvider>().default("credentials").notNull(),
|
||||
homeBoardId: varchar({ length: 64 }).references((): AnyMySqlColumn => boards.id, {
|
||||
onDelete: "set null",
|
||||
|
||||
@@ -50,7 +50,6 @@ export * from "@homarr/core/infrastructure/certificates/hostnames/db/postgresql"
|
||||
export const apiKeys = pgTable("apiKey", {
|
||||
id: varchar({ length: 64 }).notNull().primaryKey(),
|
||||
apiKey: text().notNull(),
|
||||
salt: text().notNull(),
|
||||
userId: varchar({ length: 64 })
|
||||
.notNull()
|
||||
.references((): AnyPgColumn => users.id, {
|
||||
@@ -65,7 +64,6 @@ export const users = pgTable("user", {
|
||||
emailVerified: timestamp(),
|
||||
image: text(),
|
||||
password: text(),
|
||||
salt: text(),
|
||||
provider: varchar({ length: 64 }).$type<SupportedAuthProvider>().default("credentials").notNull(),
|
||||
homeBoardId: varchar({ length: 64 }).references((): AnyPgColumn => boards.id, {
|
||||
onDelete: "set null",
|
||||
|
||||
@@ -33,7 +33,6 @@ export * from "@homarr/core/infrastructure/certificates/hostnames/db/sqlite";
|
||||
export const apiKeys = sqliteTable("apiKey", {
|
||||
id: text().notNull().primaryKey(),
|
||||
apiKey: text().notNull(),
|
||||
salt: text().notNull(),
|
||||
userId: text()
|
||||
.notNull()
|
||||
.references((): AnySQLiteColumn => users.id, {
|
||||
@@ -48,7 +47,6 @@ export const users = sqliteTable("user", {
|
||||
emailVerified: int({ mode: "timestamp_ms" }),
|
||||
image: text(),
|
||||
password: text(),
|
||||
salt: text(),
|
||||
provider: text().$type<SupportedAuthProvider>().default("credentials").notNull(),
|
||||
homeBoardId: text().references((): AnySQLiteColumn => boards.id, {
|
||||
onDelete: "set null",
|
||||
|
||||
@@ -16,7 +16,7 @@ export const mapAndDecryptUsers = (importUsers: OldmarrImportUser[], encryptionT
|
||||
({
|
||||
id,
|
||||
password,
|
||||
salt,
|
||||
salt: _saltIsNotNeededAsPartOfPassword,
|
||||
settings,
|
||||
...user
|
||||
}): InferInsertModel<typeof users> & { oldId: string; isAdmin: boolean } => ({
|
||||
@@ -30,7 +30,6 @@ export const mapAndDecryptUsers = (importUsers: OldmarrImportUser[], encryptionT
|
||||
pingIconsEnabled: settings?.replacePingWithIcons,
|
||||
isAdmin: user.isAdmin || user.isOwner,
|
||||
password: decryptSecretWithKey(password, key),
|
||||
salt: decryptSecretWithKey(salt, key),
|
||||
}),
|
||||
);
|
||||
};
|
||||
Reference in new issue
Block a user