Skip to content

Commit

Permalink
Fix jwt token verification skipping invalid WELL_KNOWN_URLS (#1002)
Browse files Browse the repository at this point in the history
  • Loading branch information
Viktor-K authored Oct 7, 2024
1 parent 516d558 commit c287f17
Show file tree
Hide file tree
Showing 18 changed files with 246 additions and 161 deletions.
3 changes: 2 additions & 1 deletion packages/agreement-process/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from "pagopa-interop-commons";
import healthRouter from "./routers/HealthRouter.js";
import agreementRouter from "./routers/AgreementRouter.js";
import { config } from "./config/config.js";

const serviceName = "agreement-process";

Expand All @@ -17,7 +18,7 @@ app.disable("x-powered-by");

app.use(contextMiddleware(serviceName));
app.use(healthRouter);
app.use(authenticationMiddleware);
app.use(authenticationMiddleware(config));
app.use(loggerMiddleware(serviceName));
app.use(agreementRouter(zodiosCtx));

Expand Down
2 changes: 1 addition & 1 deletion packages/api-gateway/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ app.use(contextMiddleware(serviceName, true));
// Unauthenticated routes
app.use(healthRouter);

app.use(authenticationMiddleware);
app.use(authenticationMiddleware(config));

// Authenticated routes - rate limiter and logger need authentication data to work
app.use(loggerMiddleware(serviceName));
Expand Down
3 changes: 2 additions & 1 deletion packages/attribute-registry-process/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from "pagopa-interop-commons";
import attributeRouter from "./routers/AttributeRouter.js";
import healthRouter from "./routers/HealthRouter.js";
import { config } from "./config/config.js";

const serviceName = "attribute-registry-process";

Expand All @@ -17,7 +18,7 @@ app.disable("x-powered-by");

app.use(contextMiddleware(serviceName));
app.use(healthRouter);
app.use(authenticationMiddleware);
app.use(authenticationMiddleware(config));
app.use(loggerMiddleware(serviceName));
app.use(attributeRouter(zodiosCtx));

Expand Down
3 changes: 2 additions & 1 deletion packages/authorization-process/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
} from "pagopa-interop-commons";
import healthRouter from "./routers/HealthRouter.js";
import authorizationRouter from "./routers/AuthorizationRouter.js";
import { config } from "./config/config.js";

const serviceName = "authorization-process";

Expand All @@ -16,7 +17,7 @@ app.disable("x-powered-by");

app.use(contextMiddleware(serviceName));
app.use(healthRouter);
app.use(authenticationMiddleware);
app.use(authenticationMiddleware(config));
app.use(authorizationRouter(zodiosCtx));

export default app;
2 changes: 1 addition & 1 deletion packages/backend-for-frontend/.env
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ PORT=3600
LOG_LEVEL=info
BACKEND_FOR_FRONTEND_INTERFACE_VERSION="0.0"

WELL_KNOWN_URLS="http://127.0.0.1:4500/jwks.json"
WELL_KNOWN_URLS="https://uat.selfcare.pagopa.it/.well-known/jwks.json,https://dev.interop.pagopa.it/.well-known/jwks.json,http://127.0.0.1:4500/jwks.json"
ACCEPTED_AUDIENCES="dev.interop.pagopa.it/ui,refactor.dev.interop.pagopa.it/ui"

TENANT_PROCESS_URL="http://localhost:3500"
Expand Down
2 changes: 1 addition & 1 deletion packages/backend-for-frontend/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ app.use(
`/backend-for-frontend/${config.backendForFrontendInterfaceVersion}`,
healthRouter,
authorizationRouter(zodiosCtx, clients, allowList, redisRateLimiter),
authenticationMiddleware,
authenticationMiddleware(config),
// Authenticated routes - rate limiter need authentication data to work
rateLimiterMiddleware(redisRateLimiter),
catalogRouter(zodiosCtx, clients, fileManager),
Expand Down
16 changes: 12 additions & 4 deletions packages/backend-for-frontend/src/services/authorizationService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,21 @@ import {
USER_ROLES,
WithLogger,
decodeJwtToken,
getJwksClients,
userRoles,
verifyJwtToken,
} from "pagopa-interop-commons";
import { TenantId, genericError, unsafeBrandId } from "pagopa-interop-models";
import { PagoPAInteropBeClients } from "../clients/clientsProvider.js";
import { config } from "../config/config.js";
import {
missingClaim,
missingSelfcareId,
tenantLoginNotAllowed,
tokenVerificationFailed,
} from "../model/errors.js";
import { PagoPAInteropBeClients } from "../clients/clientsProvider.js";
import { validateSamlResponse } from "../utilities/samlValidator.js";
import { BffAppContext } from "../utilities/context.js";
import { validateSamlResponse } from "../utilities/samlValidator.js";

const SUPPORT_USER_ID = "5119b1fa-825a-4297-8c9c-152e055cabca";

Expand All @@ -53,6 +54,8 @@ export function authorizationServiceBuilder(
allowList: string[],
rateLimiter: RateLimiter
) {
const jwksClients = getJwksClients(config);

const readJwt = async (
identityToken: string,
logger: Logger
Expand All @@ -61,12 +64,17 @@ export function authorizationServiceBuilder(
sessionClaims: SessionClaims;
selfcareId: string;
}> => {
const verified = await verifyJwtToken(identityToken, logger);
const verified = await verifyJwtToken(
identityToken,
jwksClients,
config,
logger
);
if (!verified) {
throw tokenVerificationFailed();
}

const decoded = decodeJwtToken(identityToken);
const decoded = decodeJwtToken(identityToken, logger);

const userRoles: string[] = decoded?.organization?.roles
? decoded.organization.roles.map((r: { role: string }) => r.role)
Expand Down
3 changes: 2 additions & 1 deletion packages/catalog-process/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from "pagopa-interop-commons";
import eservicesRouter from "./routers/EServiceRouter.js";
import healthRouter from "./routers/HealthRouter.js";
import { config } from "./config/config.js";

const serviceName = "catalog-process";

Expand All @@ -17,7 +18,7 @@ app.disable("x-powered-by");

app.use(contextMiddleware(serviceName));
app.use(healthRouter);
app.use(authenticationMiddleware);
app.use(authenticationMiddleware(config));
app.use(loggerMiddleware(serviceName));
app.use(eservicesRouter(zodiosCtx));

Expand Down
31 changes: 17 additions & 14 deletions packages/commons-test/test/jwt.unit.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
/* eslint-disable functional/immutable-data */
import jwt from "jsonwebtoken";
import { readAuthDataFromJwtToken } from "pagopa-interop-commons";
import {
genericLogger,
readAuthDataFromJwtToken,
} from "pagopa-interop-commons";
import { invalidClaim } from "pagopa-interop-models";
import { describe, expect, it } from "vitest";
import { P, match } from "ts-pattern";
Expand Down Expand Up @@ -109,7 +112,7 @@ describe("JWT tests", () => {
...mockUiToken,
"user-roles": "admin",
});
expect(readAuthDataFromJwtToken(token)).toEqual({
expect(readAuthDataFromJwtToken(token, genericLogger)).toEqual({
externalId: {
origin: "IPA",
value: "5N2TR557",
Expand All @@ -127,7 +130,7 @@ describe("JWT tests", () => {
"user-roles": "security,api",
});

expect(readAuthDataFromJwtToken(token)).toEqual({
expect(readAuthDataFromJwtToken(token, genericLogger)).toEqual({
externalId: {
origin: "IPA",
value: "5N2TR557",
Expand All @@ -145,7 +148,7 @@ describe("JWT tests", () => {
"user-roles": "api,invalid-role",
});

expect(() => readAuthDataFromJwtToken(token)).toThrowError(
expect(() => readAuthDataFromJwtToken(token, genericLogger)).toThrowError(
invalidClaim(
"Validation error: Invalid enum value. Expected 'admin' | 'security' | 'api' | 'support', received 'invalid-role' at \"user-roles[1]\""
)
Expand All @@ -158,7 +161,7 @@ describe("JWT tests", () => {
"user-roles": "",
});

expect(() => readAuthDataFromJwtToken(token)).toThrowError(
expect(() => readAuthDataFromJwtToken(token, genericLogger)).toThrowError(
invalidClaim(
'Validation error: String must contain at least 1 character(s) at "user-roles"'
)
Expand All @@ -167,7 +170,7 @@ describe("JWT tests", () => {

it("should successfully read auth data from a M2M token", async () => {
const token = getMockSignedToken(mockM2MToken);
expect(readAuthDataFromJwtToken(token)).toEqual({
expect(readAuthDataFromJwtToken(token, genericLogger)).toEqual({
externalId: {
origin: "",
value: "",
Expand All @@ -192,7 +195,7 @@ describe("JWT tests", () => {
jti: undefined,
});

expect(() => readAuthDataFromJwtToken(token)).toThrowError(
expect(() => readAuthDataFromJwtToken(token, genericLogger)).toThrowError(
invalidClaim(`Validation error: Required at "aud"; Required at "jti"`)
);
});
Expand All @@ -209,7 +212,7 @@ describe("JWT tests", () => {
aud: "",
});

expect(() => readAuthDataFromJwtToken(token)).toThrowError(
expect(() => readAuthDataFromJwtToken(token, genericLogger)).toThrowError(
invalidClaim(
'Validation error: String must contain at least 1 character(s) at "aud"'
)
Expand All @@ -218,7 +221,7 @@ describe("JWT tests", () => {

it("should successfully read auth data from an Internal token", async () => {
const token = getMockSignedToken(mockInternalToken);
expect(readAuthDataFromJwtToken(token)).toEqual({
expect(readAuthDataFromJwtToken(token, genericLogger)).toEqual({
externalId: {
origin: "",
value: "",
Expand All @@ -232,7 +235,7 @@ describe("JWT tests", () => {

it("should successfully read auth data from a Maintenance token", async () => {
const token = getMockSignedToken(mockMaintenanceToken);
expect(readAuthDataFromJwtToken(token)).toEqual({
expect(readAuthDataFromJwtToken(token, genericLogger)).toEqual({
externalId: {
origin: "",
value: "",
Expand All @@ -249,7 +252,7 @@ describe("JWT tests", () => {
role: "invalid-role",
});

expect(() => readAuthDataFromJwtToken(token)).toThrowError(
expect(() => readAuthDataFromJwtToken(token, genericLogger)).toThrowError(
invalidClaim(
"Validation error: Invalid discriminator value. Expected 'm2m' | 'internal' | 'maintenance' | at \"role\""
)
Expand All @@ -258,7 +261,7 @@ describe("JWT tests", () => {

it("should successfully read auth data from a Support token", async () => {
const token = getMockSignedToken(mockSupportToken);
expect(readAuthDataFromJwtToken(token)).toEqual({
expect(readAuthDataFromJwtToken(token, genericLogger)).toEqual({
externalId: {
origin: "IPA",
value: "5N2TR557",
Expand All @@ -276,7 +279,7 @@ describe("JWT tests", () => {
"user-roles": "support,invalid-role",
});

expect(() => readAuthDataFromJwtToken(token)).toThrowError(
expect(() => readAuthDataFromJwtToken(token, genericLogger)).toThrowError(
invalidClaim(
"Validation error: Invalid enum value. Expected 'admin' | 'security' | 'api' | 'support', received 'invalid-role' at \"user-roles[1]\""
)
Expand All @@ -294,7 +297,7 @@ describe("JWT tests", () => {
aud: ["dev.interop.pagopa.it/ui", "dev.interop.pagopa.it/fake"],
});

const authData = readAuthDataFromJwtToken(token);
const authData = readAuthDataFromJwtToken(token, genericLogger);
expect(authData).toMatchObject({
userRoles: match(mockToken)
.with({ role: P.not(P.nullish) }, (t) => [t.role])
Expand Down
Loading

0 comments on commit c287f17

Please sign in to comment.