diff --git a/docs/content/dev/2.0.md b/docs/content/dev/2.0.md index 797dd67ad..863b0b5a4 100644 --- a/docs/content/dev/2.0.md +++ b/docs/content/dev/2.0.md @@ -39,3 +39,28 @@ Because we need to have empty constructors in our entity classes for TypeORM to - Should either return a complete and fully useable instance or return a Pick/Omit type. - Exceptions to these rules are allowed, if they are mentioned in the method documentation +## Auth tokens for the public API +The public API uses bearer tokens for authentication. + +When a new token is requested via the private API, the backend generates a 64 bytes-long secret of +cryptographically secure data and returns it as a base64url-encoded string, along with an identifier. +That string can then be used by clients as a bearer token. + +A SHA-512 hash of the secret is stored in the database. To validate tokens, the backend computes the hash of the provided +secret and checks it against the stored hash for the provided identifier. + +### Choosing a hash function +Unfortunately, there does not seem to be any explicit documentation about our exact use-case. +Most docs describe classic password-saving scenarios and recommend bcrypt, scrypt or argon2. +These hashing functions are slow to stop brute-force or dictionary attacks, which would expose the original, +user-provided password, that may have been reused across multiple services. + +We have a very different scenario: +Our API tokens are 64 bytes of cryptographically strong pseudorandom data. +Brute-force or dictionary attacks are therefore virtually impossible, and tokens are not reused across multiple services. +We therefore need to only guard against one scenario: +An attacker gains read-only access to the database. Saving only hashes in the database prevents the attacker +from authenticating themselves as a user. The hash-function does not need to be very slow, +as the randomness of the original token prevents inverting the hash. The function actually needs to be reasonably fast, +as the hash must be computed on every request to the public API. +SHA-512 (or alternatively SHA3) fits this use-case. diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 21a39d752..f9d5249bc 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -7,6 +7,7 @@ import { ConfigModule } from '@nestjs/config'; import { PassportModule } from '@nestjs/passport'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; +import crypto from 'crypto'; import { Repository } from 'typeorm'; import appConfigMock from '../config/mock/app.config.mock'; @@ -86,7 +87,10 @@ describe('AuthService', () => { describe('getAuthToken', () => { const token = 'testToken'; it('works', async () => { - const accessTokenHash = await hashPassword(token); + const accessTokenHash = crypto + .createHash('sha512') + .update(token) + .digest('hex'); jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce({ ...authToken, user: user, @@ -162,8 +166,12 @@ describe('AuthService', () => { describe('validateToken', () => { it('works', async () => { - const token = 'testToken'; - const accessTokenHash = await hashPassword(token); + const testSecret = + 'gNrv_NJ4FHZ0UFZJQu_q_3i3-GP_d6tELVtkYiMFLkLWNl_dxEmPVAsCNKxP3N3DB9aGBVFYE1iptvw7hFMJvA'; + const accessTokenHash = crypto + .createHash('sha512') + .update(testSecret) + .digest('hex'); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce({ ...user, authTokens: [authToken], @@ -179,7 +187,7 @@ describe('AuthService', () => { return authToken; }); const userByToken = await service.validateToken( - `${authToken.keyId}.${token}`, + `${authToken.keyId}.${testSecret}`, ); expect(userByToken).toEqual({ ...user, diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 789e9b7fb..88a982e67 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -6,7 +6,7 @@ import { Injectable } from '@nestjs/common'; import { Cron, Timeout } from '@nestjs/schedule'; import { InjectRepository } from '@nestjs/typeorm'; -import { randomBytes } from 'crypto'; +import crypto, { randomBytes } from 'crypto'; import { Repository } from 'typeorm'; import { @@ -17,11 +17,7 @@ import { import { ConsoleLoggerService } from '../logger/console-logger.service'; import { User } from '../users/user.entity'; import { UsersService } from '../users/users.service'; -import { - bufferToBase64Url, - checkPassword, - hashPassword, -} from '../utils/password'; +import { bufferToBase64Url } from '../utils/password'; import { TimestampMillis } from '../utils/timestamp'; import { AuthTokenWithSecretDto } from './auth-token-with-secret.dto'; import { AuthTokenDto } from './auth-token.dto'; @@ -43,13 +39,10 @@ export class AuthService { if (!secret) { throw new TokenNotValidError('Invalid AuthToken format'); } - if (secret.length > 72) { - // Only the first 72 characters of the tokens are considered by bcrypt - // This should prevent strange corner cases - // At the very least it won't hurt us - throw new TokenNotValidError( - `AuthToken '${secret}' is too long the be a proper token`, - ); + if (secret.length != 86) { + // We always expect 86 characters, as the secret is generated with 64 bytes + // and then converted to a base64url string + throw new TokenNotValidError(`AuthToken '${token}' has incorrect length`); } const accessToken = await this.getAuthTokenAndValidate(keyId, secret); await this.setLastUsedToken(keyId); @@ -70,9 +63,13 @@ export class AuthService { `User '${user.username}' has already 200 tokens and can't have anymore`, ); } - const secret = bufferToBase64Url(randomBytes(54)); + const secret = bufferToBase64Url(randomBytes(64)); const keyId = bufferToBase64Url(randomBytes(8)); - const accessToken = await hashPassword(secret); + // More about the choice of SHA-512 in the dev docs + const accessTokenHash = crypto + .createHash('sha512') + .update(secret) + .digest('hex'); let token; // Tokens can only be valid for a maximum of 2 years const maximumTokenValidity = @@ -82,7 +79,7 @@ export class AuthService { keyId, user, identifier, - accessToken, + accessTokenHash, new Date(maximumTokenValidity), ); } else { @@ -90,7 +87,7 @@ export class AuthService { keyId, user, identifier, - accessToken, + accessTokenHash, new Date(validUntil), ); } @@ -122,7 +119,17 @@ export class AuthService { if (accessToken === undefined) { throw new NotInDBError(`AuthToken '${token}' not found`); } - if (!(await checkPassword(token, accessToken.accessTokenHash))) { + // Hash the user-provided token + const userHash = Buffer.from( + crypto.createHash('sha512').update(token).digest('hex'), + ); + const dbHash = Buffer.from(accessToken.accessTokenHash); + if ( + // Normally, both hashes have the same length, as they are both SHA512 + // This is only defense-in-depth, as timingSafeEqual throws if the buffers are not of the same length + userHash.length !== dbHash.length || + !crypto.timingSafeEqual(userHash, dbHash) + ) { // hashes are not the same throw new TokenNotValidError(`AuthToken '${token}' is not valid.`); } diff --git a/test/private-api/tokens.e2e-spec.ts b/test/private-api/tokens.e2e-spec.ts index b49e769cf..d3b140932 100644 --- a/test/private-api/tokens.e2e-spec.ts +++ b/test/private-api/tokens.e2e-spec.ts @@ -48,7 +48,7 @@ describe('Tokens', () => { expect(response.body.label).toBe(tokenName); expect(response.body.validUntil).toBe(null); expect(response.body.lastUsed).toBe(null); - expect(response.body.secret.length).toBe(84); + expect(response.body.secret.length).toBe(98); }); it(`GET /tokens`, async () => {