Merge pull request #1882 from hedgedoc/fix/auth_token_hash

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.

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,

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.`);
     }

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 () => {