From e178c014f273a3982185eb371590fd40f9a3d12a Mon Sep 17 00:00:00 2001 From: Eric Camellini Date: Thu, 10 Oct 2024 11:59:34 +0200 Subject: [PATCH] IMN-879 - Refactoring headers parsing and improving auth (#1079) --- packages/api-gateway/src/app.ts | 2 +- packages/backend-for-frontend/src/app.ts | 5 +- .../src/auth/authenticationMiddleware.ts | 100 +++++------------- .../src/auth/authorizationMiddleware.ts | 35 ++---- packages/commons/src/auth/headers.ts | 85 ++++++--------- packages/commons/src/auth/jwt.ts | 6 +- packages/commons/src/context/context.ts | 49 ++++++--- packages/models/src/errors.ts | 13 ++- 8 files changed, 114 insertions(+), 181 deletions(-) diff --git a/packages/api-gateway/src/app.ts b/packages/api-gateway/src/app.ts index 6cc1be41d4..2a505bd121 100644 --- a/packages/api-gateway/src/app.ts +++ b/packages/api-gateway/src/app.ts @@ -31,7 +31,7 @@ const redisRateLimiter = await initRedisRateLimiter({ // See https://cheatsheetseries.owasp.org/cheatsheets/HTTP_Headers_Cheat_Sheet.html#recommendation_16 app.disable("x-powered-by"); -app.use(contextMiddleware(serviceName, true)); +app.use(contextMiddleware(serviceName, false)); app.use( `/api-gateway/${config.apiGatewayInterfaceVersion}`, diff --git a/packages/backend-for-frontend/src/app.ts b/packages/backend-for-frontend/src/app.ts index 6ede95dc95..0e9859b6eb 100644 --- a/packages/backend-for-frontend/src/app.ts +++ b/packages/backend-for-frontend/src/app.ts @@ -53,8 +53,6 @@ app.disable("x-powered-by"); app.disable("etag"); -app.use(loggerMiddleware(serviceName)); - // parse files from multipart/form-data and put them in req.body app.use(multerMiddleware); app.use(fromFilesToBodyMiddleware); @@ -62,7 +60,8 @@ app.use(fromFilesToBodyMiddleware); // parse application/x-www-form-urlencoded and put it in req.body app.use(express.urlencoded({ extended: true })); -app.use(contextMiddleware(serviceName, true)); +app.use(contextMiddleware(serviceName, false)); +app.use(loggerMiddleware(serviceName)); app.use( `/backend-for-frontend/${config.backendForFrontendInterfaceVersion}`, diff --git a/packages/commons/src/auth/authenticationMiddleware.ts b/packages/commons/src/auth/authenticationMiddleware.ts index 20607fdae7..d59a80e06c 100644 --- a/packages/commons/src/auth/authenticationMiddleware.ts +++ b/packages/commons/src/auth/authenticationMiddleware.ts @@ -2,15 +2,17 @@ import { ZodiosRouterContextRequestHandler } from "@zodios/express"; import { makeApiProblemBuilder, - missingBearer, - missingHeader, unauthorizedError, } from "pagopa-interop-models"; -import { match, P } from "ts-pattern"; -import { ExpressContext, getJwksClients, JWTConfig } from "../index.js"; -import { Logger, logger } from "../logging/index.js"; +import { match } from "ts-pattern"; +import { + ExpressContext, + fromAppContext, + getJwksClients, + JWTConfig, + jwtFromAuthHeader, +} from "../index.js"; import { AuthData } from "./authData.js"; -import { Headers } from "./headers.js"; import { readAuthDataFromJwtToken, verifyJwtToken } from "./jwt.js"; const makeApiProblem = makeApiProblemBuilder({}); @@ -20,80 +22,28 @@ export const authenticationMiddleware: ( ) => ZodiosRouterContextRequestHandler = (config: JWTConfig) => async (req, res, next): Promise => { - const jwksClients = getJwksClients(config); - - const addCtxAuthData = async ( - authHeader: string, - logger: Logger - ): Promise => { - const authorizationHeader = authHeader.split(" "); - if ( - authorizationHeader.length !== 2 || - authorizationHeader[0] !== "Bearer" - ) { - logger.warn( - `No authentication has been provided for this call ${req.method} ${req.url}` - ); - throw missingBearer; - } + // We assume that: + // - contextMiddleware already set ctx.serviceName and ctx.correlationId + const ctx = fromAppContext(req.ctx); - const jwtToken = authorizationHeader[1]; - const valid = await verifyJwtToken(jwtToken, jwksClients, config, logger); + try { + const jwksClients = getJwksClients(config); + + const jwtToken = jwtFromAuthHeader(req, ctx.logger); + const valid = await verifyJwtToken( + jwtToken, + jwksClients, + config, + ctx.logger + ); if (!valid) { throw unauthorizedError("Invalid token"); } - const authData: AuthData = readAuthDataFromJwtToken(jwtToken, logger); + const authData: AuthData = readAuthDataFromJwtToken(jwtToken, ctx.logger); // eslint-disable-next-line functional/immutable-data req.ctx.authData = authData; - next(); - }; - - const loggerInstance = logger({ - serviceName: req.ctx.serviceName, - correlationId: req.ctx.correlationId, - }); - - try { - const headers = Headers.safeParse(req.headers); - if (!headers.success) { - throw missingHeader(); - } - - return await match(headers.data) - .with( - { - authorization: P.string, - "x-correlation-id": P.string, - }, - async (headers) => - await addCtxAuthData(headers.authorization, loggerInstance) - ) - .with( - { - authorization: P.nullish, - "x-correlation-id": P._, - }, - () => { - loggerInstance.warn( - `No authentication has been provided for this call ${req.method} ${req.url}` - ); - - throw missingBearer; - } - ) - .with( - { - authorization: P.string, - "x-correlation-id": P.nullish, - }, - () => { - throw missingHeader("x-correlation-id"); - } - ) - .otherwise(() => { - throw missingHeader(); - }); + return next(); } catch (error) { const problem = makeApiProblem( error, @@ -101,9 +51,9 @@ export const authenticationMiddleware: ( match(err.code) .with("unauthorizedError", () => 401) .with("operationForbidden", () => 403) - .with("missingHeader", () => 400) + .with("missingHeader", "badBearerToken", () => 400) .otherwise(() => 500), - loggerInstance + ctx.logger ); return res.status(problem.status).send(problem); } diff --git a/packages/commons/src/auth/authorizationMiddleware.ts b/packages/commons/src/auth/authorizationMiddleware.ts index 5d6d5096af..648603a8aa 100644 --- a/packages/commons/src/auth/authorizationMiddleware.ts +++ b/packages/commons/src/auth/authorizationMiddleware.ts @@ -3,7 +3,6 @@ import { ZodiosEndpointDefinition, Method, } from "@zodios/core"; -import { Request } from "express"; import { Problem, makeApiProblemBuilder, @@ -11,14 +10,11 @@ import { ApiError, unauthorizedError, CommonErrorCodes, - missingBearer, } from "pagopa-interop-models"; import { P, match } from "ts-pattern"; import { z } from "zod"; import { Middleware } from "../types/middleware.js"; -import { UserRole, readHeaders } from "../index.js"; -import { genericLogger, logger } from "../logging/index.js"; -import { readAuthDataFromJwtToken } from "./jwt.js"; +import { AuthData, fromAppContext, UserRole } from "../index.js"; type RoleValidation = | { @@ -28,15 +24,9 @@ type RoleValidation = | { isValid: true }; const hasValidRoles = ( - req: Request, + authData: AuthData, admittedRoles: UserRole[] ): RoleValidation => { - const jwtToken = req.headers.authorization?.split(" ")[1]; - if (!jwtToken) { - throw missingBearer; - } - - const authData = readAuthDataFromJwtToken(jwtToken, genericLogger); if (!authData.userRoles || authData.userRoles.length === 0) { return { isValid: false, @@ -77,22 +67,19 @@ export const authorizationMiddleware = admittedRoles: UserRole[] ): Middleware => (req, res, next) => { + // We assume that: + // - contextMiddleware already set ctx.serviceName and ctx.correlationId + // - authorizationMiddleware already validated the token and set ctx.authData + const ctx = fromAppContext(req.ctx); + try { - const validationResult = hasValidRoles(req as Request, admittedRoles); + const validationResult = hasValidRoles(ctx.authData, admittedRoles); if (!validationResult.isValid) { throw validationResult.error; } return next(); } catch (err) { - const headers = readHeaders(req as Request); - - const loggerInstance = logger({ - userId: headers?.userId, - organizationId: headers?.organizationId, - correlationId: headers?.correlationId, - }); - const problem = match(err) .with(P.instanceOf(ApiError), (error) => makeApiProblem( @@ -100,10 +87,10 @@ export const authorizationMiddleware = code: error.code, detail: error.detail, title: error.title, - correlationId: headers?.correlationId, + correlationId: ctx.correlationId, }), (error) => (error.code === "unauthorizedError" ? 403 : 500), - loggerInstance + ctx.logger ) ) .otherwise(() => @@ -112,7 +99,7 @@ export const authorizationMiddleware = "An unexpected error occurred during authorization checks" ), () => 500, - loggerInstance + ctx.logger ) ); diff --git a/packages/commons/src/auth/headers.ts b/packages/commons/src/auth/headers.ts index 25ecf25910..caa0104d38 100644 --- a/packages/commons/src/auth/headers.ts +++ b/packages/commons/src/auth/headers.ts @@ -1,61 +1,40 @@ import { Request } from "express"; -import { P, match } from "ts-pattern"; +import { badBearerToken, missingHeader } from "pagopa-interop-models"; import { z } from "zod"; -import { genericLogger } from "../logging/index.js"; -import { AuthData } from "./authData.js"; -import { readAuthDataFromJwtToken } from "./jwt.js"; +import { Logger } from "../logging/index.js"; -export const Headers = z.object({ - authorization: z.string().nullish(), - "x-correlation-id": z.string().nullish(), -}); +export function parseCorrelationIdHeader(req: Request): string | undefined { + const parsed = z + .object({ "x-correlation-id": z.string() }) + .safeParse(req.headers); -export type Headers = z.infer; - -export const ParsedHeaders = z - .object({ - correlationId: z.string(), - }) - .and(AuthData); -export type ParsedHeaders = z.infer; - -export const readCorrelationIdHeader = (req: Request): string | undefined => - match(req.headers) - .with( - { "x-correlation-id": P.string }, - (headers) => headers["x-correlation-id"] - ) - .otherwise(() => undefined); + if (parsed.success) { + return parsed.data["x-correlation-id"]; + } + return undefined; +} +export function parseAuthHeader(req: Request): string | undefined { + const parsed = z.object({ authorization: z.string() }).safeParse(req.headers); -export const readHeaders = (req: Request): ParsedHeaders | undefined => { - try { - const headers = Headers.parse(req.headers); - return match(headers) - .with( - { - authorization: P.string, - "x-correlation-id": P.string, - }, - (headers) => { - const authorizationHeader = headers.authorization.split(" "); - if ( - authorizationHeader.length !== 2 || - authorizationHeader[0] !== "Bearer" - ) { - return undefined; - } + if (parsed.success) { + return parsed.data.authorization; + } + return undefined; +} - const jwtToken = authorizationHeader[1]; - const authData = readAuthDataFromJwtToken(jwtToken, genericLogger); +export function jwtFromAuthHeader(req: Request, logger: Logger): string { + const authHeader = parseAuthHeader(req); + if (!authHeader) { + throw missingHeader("Authorization"); + } - return { - ...authData, - correlationId: headers["x-correlation-id"], - }; - } - ) - .otherwise(() => undefined); - } catch (error) { - return undefined; + const authHeaderParts = authHeader.split(" "); + if (authHeaderParts.length !== 2 || authHeaderParts[0] !== "Bearer") { + logger.warn( + `Invalid authentication provided for this call ${req.method} ${req.url}` + ); + throw badBearerToken; } -}; + + return authHeaderParts[1]; +} diff --git a/packages/commons/src/auth/jwt.ts b/packages/commons/src/auth/jwt.ts index c05ccec516..9722c2e949 100644 --- a/packages/commons/src/auth/jwt.ts +++ b/packages/commons/src/auth/jwt.ts @@ -78,12 +78,12 @@ export const verifyJwtToken = async ( const jwtHeader = decodeJwtTokenHeaders(jwtToken, logger); if (!jwtHeader?.kid) { logger.warn("Token verification failed: missing kid"); - return Promise.reject(false); + return Promise.resolve(false); } const secret: Secret = await getKey(jwksClients, jwtHeader.kid, logger); - return new Promise((resolve, _reject) => { + return new Promise((resolve) => { jwt.verify( jwtToken, secret, @@ -101,7 +101,7 @@ export const verifyJwtToken = async ( }); } catch (error) { logger.error(`Error verifying JWT token: ${error}`); - return Promise.reject(false); + return Promise.resolve(false); } }; diff --git a/packages/commons/src/context/context.ts b/packages/commons/src/context/context.ts index e7e22f5f11..22eb7fe75d 100644 --- a/packages/commons/src/context/context.ts +++ b/packages/commons/src/context/context.ts @@ -1,12 +1,14 @@ +import { constants } from "http2"; import { ZodiosRouterContextRequestHandler, zodiosContext, } from "@zodios/express"; import { v4 as uuidv4 } from "uuid"; import { z } from "zod"; +import { makeApiProblemBuilder, missingHeader } from "pagopa-interop-models"; import { AuthData } from "../auth/authData.js"; -import { Logger, logger } from "../logging/index.js"; -import { readCorrelationIdHeader } from "../auth/headers.js"; +import { genericLogger, Logger, logger } from "../logging/index.js"; +import { parseCorrelationIdHeader } from "../auth/headers.js"; export const AppContext = z.object({ serviceName: z.string(), @@ -25,21 +27,38 @@ export function fromAppContext(ctx: AppContext): WithLogger { return { ...ctx, logger: logger({ ...ctx }) }; } +const makeApiProblem = makeApiProblemBuilder({}); + export const contextMiddleware = ( serviceName: string, - overrideCorrelationId: boolean = false + readCorrelationIdFromHeader: boolean = true ): ZodiosRouterContextRequestHandler => - (req, _res, next): void => { - const correlationId = overrideCorrelationId - ? uuidv4() - : readCorrelationIdHeader(req) ?? uuidv4(); - - // eslint-disable-next-line functional/immutable-data - req.ctx = { - serviceName, - correlationId, - } as AppContext; - - next(); + async (req, res, next): Promise => { + const setCtx = (correlationId: string): void => { + // eslint-disable-next-line functional/immutable-data + req.ctx = { + serviceName, + correlationId, + } as AppContext; + }; + + if (readCorrelationIdFromHeader) { + const correlationIdHeader = parseCorrelationIdHeader(req); + + if (!correlationIdHeader) { + const problem = makeApiProblem( + missingHeader("X-Correlation-Id"), + () => constants.HTTP_STATUS_BAD_REQUEST, + genericLogger + ); + return res.status(problem.status).send(problem); + } + + setCtx(correlationIdHeader); + } else { + setCtx(uuidv4()); + } + + return next(); }; diff --git a/packages/models/src/errors.ts b/packages/models/src/errors.ts index b8e71d3e40..906e5967ca 100644 --- a/packages/models/src/errors.ts +++ b/packages/models/src/errors.ts @@ -193,6 +193,7 @@ const errorCodes = { tooManyRequestsError: "10004", notAllowedCertificateException: "10005", jwksSigningKeyError: "10006", + badBearerToken: "10007", } as const; export type CommonErrorCodes = keyof typeof errorCodes; @@ -354,18 +355,16 @@ export function jwtDecodingError(error: unknown): ApiError { export function missingHeader(headerName?: string): ApiError { const title = "Header has not been passed"; return new ApiError({ - detail: headerName - ? `Header ${headerName} not existing in this request` - : title, + detail: headerName ? `Missing ${headerName} request header` : title, code: "missingHeader", title, }); } -export const missingBearer: ApiError = new ApiError({ - detail: `Authorization Illegal header key.`, - code: "missingHeader", - title: "Bearer token has not been passed", +export const badBearerToken: ApiError = new ApiError({ + detail: `Bad Bearer Token format in Authorization header`, + code: "badBearerToken", + title: "Bad Bearer Token format", }); export const operationForbidden: ApiError = new ApiError({