diff --git a/docs/architecture/adrs/0165-login-never-answers-404.es.md b/docs/architecture/adrs/0165-login-never-answers-404.es.md index adc406f1..d1871ad7 100644 --- a/docs/architecture/adrs/0165-login-never-answers-404.es.md +++ b/docs/architecture/adrs/0165-login-never-answers-404.es.md @@ -14,20 +14,30 @@ | Código | Significado | Estado anterior | | --- | --- | --- | | `AUTH_002` | El código de tenant no existe | 404 | -| `AUTH_004` | El usuario no existe en ese tenant | 404 | +| `AUTH_004` | El IdP autenticó a alguien sin cuenta UMS en ese tenant | 404 | | `AUTH_006` | Las credenciales no autentican | 401 | Los mensajes de `AUTH_004` y `AUTH_006` son **idénticos palabra por palabra**: «No pudimos iniciar sesión. Verifique sus credenciales.» Alguien los escribió así deliberadamente, para que quien -intenta entrar no pueda distinguir «ese usuario no existe» de «existe y te equivocaste». El estado -HTTP deshacía ese cuidado: bastaba mirar si la respuesta era 404 o 401 para saber cuál de las dos -cosas había pasado, sin leer el cuerpo. - -Eso es un **oráculo de enumeración**: un atacante sin credenciales puede recorrer una lista de -correos contra `POST /api/v1/auth/login` y separar los que existen de los que no, a razón de una -petición por candidato. Lo que se obtiene no es acceso, pero sí la mitad del trabajo: una lista -depurada de cuentas reales sobre la que concentrar el resto del esfuerzo. Y la mitad cara, porque -es la que no se puede adivinar. +intenta entrar no pueda distinguir un desenlace del otro. El estado HTTP deshacía ese cuidado: +bastaba mirar si la respuesta era 404 o 401 para saber cuál de las dos cosas había pasado, sin leer +el cuerpo. Eso es un **oráculo**: el estado contesta lo que el mensaje se negaba a contestar. + +Conviene decir con precisión a quién sirve cada oráculo, porque no son el mismo y la primera +redacción de este ADR los confundió: + +- **`AUTH_004` no es «el usuario no existe».** En la rama local, un usuario inexistente devuelve + `AUTH_006` —el mismo código que una contraseña equivocada—, y ahí nunca hubo nada que filtrar + (`AuthenticateUserCommandHandler.AuthenticateLocalAsync`). `AUTH_004` sólo se emite en la rama + federada y **después** de que la cadena de IdP haya autenticado con éxito (`AuthenticateIdpAsync`): + para llegar a ese 404 hay que traer credenciales válidas del IdP. No es, por tanto, el barrido + anónimo de una lista de correos, sino un oráculo para quien ya está dentro — alguien con cuenta en + el IdP corporativo puede recorrer inquilinos y averiguar en cuáles tiene cuenta UMS una identidad + federada. Sigue siendo lo que el mensaje calla a propósito y el estado regalaba. +- **El oráculo anónimo y barato era `AUTH_002`.** La búsqueda del inquilino ocurre antes de tocar + credencial alguna, así que `{ tenantCode: «CANDIDATO», username: …, password: cualquiera }` separa + códigos de inquilino reales de inventados a razón de una petición por candidato y sin credencial + de ningún tipo. Ese sí es el barrido a escala, y §2.3 explica por qué aquí no se cierra del todo. El desajuste salió a la luz por otro camino: la verificación de contrato Pact esperaba `400` para un tenant inexistente y la API respondía `404`. El contrato tenía razón por accidente. @@ -41,10 +51,10 @@ responde una pregunta que quien pregunta no ha demostrado tener derecho a hacer. ### §2.2 — Todo lo que huele a credencial responde 401 -`AUTH_004` (usuario inexistente) pasa a `401`, el mismo estado que `AUTH_006` (credenciales -inválidas) y que `AUTH_005` (cuenta inactiva). Las tres respuestas son ahora indistinguibles desde -fuera salvo por el `code` del cuerpo, que es información que ya se entrega a quien la pide de -frente y que las pantallas necesitan para elegir el mensaje. +`AUTH_004` (el IdP autenticó a alguien que aquí no tiene cuenta) pasa a `401`, el mismo estado que +`AUTH_006` (credenciales inválidas) y que `AUTH_005` (cuenta inactiva). Las tres respuestas son +ahora indistinguibles desde fuera salvo por el `code` del cuerpo, que es información que ya se +entrega a quien la pide de frente y que las pantallas necesitan para elegir el mensaje. > El `code` sigue diferenciándolas. Eso es deliberado y no reabre el oráculo en la práctica: quien > enumera lo hace a escala y automatizado, y el cuerpo ya viaja con el mensaje que lo delataría de @@ -72,6 +82,20 @@ precisión lo que pasó —la petición trae un código que no existe— y es lo > `/auth/login`— y merece su propio ADR y su propia conversación con producto. Queda señalado > aquí para que la asimetría no se lea como olvido. +### §2.4 — `/client/authenticate` tampoco responde ya 404 + +Al escribir §2.3 se comprobó que el endpoint que se citaba como ejemplo de enumeración cerrada +tenía el mismo agujero: `ClientAuthEndpoints.GetStatusCode` mapeaba `AUTH_004` a `404` mientras +colapsaba a 401 todo lo demás de nivel usuario o inquilino. Ahí el defecto era más nítido que en el +portal, porque ese endpoint persigue la indistinguibilidad como diseño explícito —`SpanishMessage` +devuelve la MISMA cadena exacta para `AUTH_002`, `AUTH_003`, `AUTH_004`, `AUTH_006` y `AUTH_017`, y +no hay ningún humano al otro lado a quien darle la pista—, de modo que el estado era la única señal +que quedaba en pie. El comentario de G-053 en ese método afirmaba que «AUTH_004/005 conservan su +semántica»: `AUTH_005` sí había colapsado a 401, `AUTH_004` se quedó en 404 y nadie lo notó. + +`AUTH_004` responde ahora `401` también allí. G-053 no se amplía ni se reinterpreta: se termina de +aplicar donde ya regía. + ## Consecuencias - El front no cambia: `auth.service.ts` decide el mensaje por el `code` del cuerpo, no por el @@ -83,13 +107,14 @@ precisión lo que pasó —la petición trae un código que no existe— y es lo dejó de responder hace tiempo. - Quien lea `MapAuthError` en el futuro verá 401 donde el instinto REST pediría 404. El comentario del método explica por qué, y este ADR es la razón larga: **no es un descuido, es la decisión**. +- Ningún test afirmaba el `404` de `AUTH_004` en `/client/authenticate` (§2.4). Que se pudiera + cambiar sin tocar una sola aserción es, en sí, parte del hallazgo: la rama no estaba cubierta. ## Alternativas descartadas -- **Dejar `AUTH_002` en 404 y mover sólo `AUTH_004`.** Cierra el oráculo de cuentas, que es el que - importa, pero deja el contrato Pact roto y obliga a explicar por qué un endpoint de - autenticación devuelve 404 para una cosa y 401 para otra sin que la diferencia sea la - sensibilidad del dato. +- **Dejar `AUTH_002` en 404 y mover sólo `AUTH_004`.** Cierra la filtración de nivel usuario, pero + deja el contrato Pact roto y obliga a explicar por qué un endpoint de autenticación devuelve 404 + para una cosa y 401 para otra sin que la diferencia sea la sensibilidad del dato. - **Colapsar `AUTH_002`, `AUTH_004` y `AUTH_006` en un único 401 sin `code`.** Es lo más hermético y lo peor de usar: quien teclea mal el código de su organización merece que se lo digan. Se descarta por coste de usabilidad frente a una ganancia que §2.2 ya obtiene en su mayor parte. diff --git a/docs/architecture/adrs/0165-login-never-answers-404.md b/docs/architecture/adrs/0165-login-never-answers-404.md index e8b27ce8..302dfbce 100644 --- a/docs/architecture/adrs/0165-login-never-answers-404.md +++ b/docs/architecture/adrs/0165-login-never-answers-404.md @@ -14,19 +14,30 @@ | Code | Meaning | Previous status | | --- | --- | --- | | `AUTH_002` | The tenant code does not exist | 404 | -| `AUTH_004` | The user does not exist in that tenant | 404 | +| `AUTH_004` | The IdP authenticated someone with no UMS account in that tenant | 404 | | `AUTH_006` | The credentials do not authenticate | 401 | The messages for `AUTH_004` and `AUTH_006` are **word-for-word identical**: "No pudimos iniciar sesión. Verifique sus credenciales." Someone wrote them that way on purpose, so that whoever is -trying to sign in cannot tell "that user does not exist" from "it exists and you got it wrong." -The HTTP status undid that care: looking at whether the response was 404 or 401 was enough to know -which of the two had happened, without reading the body at all. - -That is an **enumeration oracle**: an attacker with no credentials can run a list of email -addresses against `POST /api/v1/auth/login` and separate the ones that exist from the ones that do -not, one request per candidate. What that yields is not access, but it is half the work — and the -expensive half, because it is the half you cannot guess. +trying to sign in cannot tell one outcome from the other. The HTTP status undid that care: looking +at whether the response was 404 or 401 was enough to know which of the two had happened, without +reading the body at all. That is an **oracle**: the status answers what the message refused to. + +It is worth saying precisely who each oracle serves, because they are not the same one and this +ADR's first draft conflated them: + +- **`AUTH_004` is not "the user does not exist."** On the local branch, a user who does not exist + yields `AUTH_006` — the same code as a wrong password — and there was never anything to leak + there (`AuthenticateUserCommandHandler.AuthenticateLocalAsync`). `AUTH_004` is emitted only on + the federated branch, and only **after** the IdP chain has authenticated successfully + (`AuthenticateIdpAsync`): reaching that 404 requires valid IdP credentials. So it is not the + anonymous sweep of an email list; it is an oracle for someone already inside — a holder of a + corporate IdP account can walk tenants and learn which of them a federated identity has a UMS + account in. It is still what the message withholds on purpose and the status handed over free. +- **The anonymous, cheap oracle was `AUTH_002`.** The tenant lookup happens before any credential + is touched, so `{ tenantCode: "CANDIDATE", username: …, password: anything }` separates real + tenant codes from invented ones at one request per candidate, with no credentials of any kind. + That is the sweep that scales — and §2.3 explains why it is not fully closed here. The mismatch surfaced from another direction: the Pact provider verification expected `400` for an unknown tenant and the API answered `404`. The contract happened to be right. @@ -40,10 +51,10 @@ question the asker has not shown any right to ask. ### §2.2 — Anything credential-shaped answers 401 -`AUTH_004` (user does not exist) becomes `401`, the same status as `AUTH_006` (invalid credentials) -and `AUTH_005` (inactive account). The three responses are now indistinguishable from outside -except by the body's `code`, which is information already handed to anyone who asks directly and -which the screens need in order to choose their message. +`AUTH_004` (the IdP authenticated someone who has no account here) becomes `401`, the same status +as `AUTH_006` (invalid credentials) and `AUTH_005` (inactive account). The three responses are now +indistinguishable from outside except by the body's `code`, which is information already handed to +anyone who asks directly and which the screens need in order to choose their message. > The `code` still tells them apart. That is deliberate and does not reopen the oracle in practice: > enumeration is done at scale and automated, and the body already carries the message that would @@ -72,6 +83,20 @@ contract expects. > G-053 to `/auth/login` — and deserves its own ADR and its own conversation with product. It is > flagged here so the asymmetry does not read as an oversight. +### §2.4 — `/client/authenticate` stops answering 404 as well + +Writing §2.3 turned up the same hole in the very endpoint being cited as the closed case: +`ClientAuthEndpoints.GetStatusCode` mapped `AUTH_004` to `404` while collapsing everything else at +user or tenant level to 401. The defect is sharper there than on the portal, because that endpoint +pursues indistinguishability as explicit design — `SpanishMessage` returns the SAME exact string for +`AUTH_002`, `AUTH_003`, `AUTH_004`, `AUTH_006` and `AUTH_017`, and there is no human on the other +end who needs the hint — so the status was the only signal left standing. G-053's comment on that +method claimed "AUTH_004/005 keep their semantics": `AUTH_005` had in fact collapsed to 401, +`AUTH_004` stayed at 404 and nobody noticed. + +`AUTH_004` now answers `401` there too. G-053 is neither widened nor reinterpreted: it is finished +where it already applied. + ## Consequences - The front end does not change: `auth.service.ts` picks its message from the body's `code`, not @@ -83,13 +108,15 @@ contract expects. - Anyone reading `MapAuthError` in the future will see 401 where REST instinct would ask for 404. The method's comment says why, and this ADR is the long form: **it is not an oversight, it is the decision**. +- No test asserted the `404` for `AUTH_004` on `/client/authenticate` (§2.4). That it could be + changed without touching a single assertion is itself part of the finding: the branch was + uncovered. ## Alternatives rejected -- **Leave `AUTH_002` at 404 and move only `AUTH_004`.** Closes the account oracle, which is the one - that matters, but leaves the Pact contract broken and forces an explanation of why an - authentication endpoint returns 404 for one thing and 401 for another when the difference is not - the sensitivity of the data. +- **Leave `AUTH_002` at 404 and move only `AUTH_004`.** Closes the user-level leak, but leaves the + Pact contract broken and forces an explanation of why an authentication endpoint returns 404 for + one thing and 401 for another when the difference is not the sensitivity of the data. - **Collapse `AUTH_002`, `AUTH_004` and `AUTH_006` into a single 401 with no `code`.** The most airtight and the worst to use: someone who mistypes their organisation's code deserves to be told so. Rejected on usability cost against a gain that §2.2 already captures for the most part. diff --git a/docs/architecture/technical-debt.md b/docs/architecture/technical-debt.md index fd2be2b1..387527d7 100644 --- a/docs/architecture/technical-debt.md +++ b/docs/architecture/technical-debt.md @@ -180,7 +180,7 @@ What the gate had been hiding — each of these is a live defect, not a type-ann - **Description**: `Ums.ContractTest` failed 37/38 from the moment of the resync and stayed red on `main` (`c011e42a`). Two failures, one interaction each, both on `POST /api/v1/auth/login`: 1. The pact expected `{ status, title }` — ASP.NET's default `ProblemDetails` — and the API returns `LoginErrorResponse`: `{ code, message, supportReferenceId }`. 2. The pact expected `400` for an unknown tenant and the API answered `404`. -- **What the second one actually was**: not a contract nicety. `AUTH_002` (unknown tenant) and `AUTH_004` (unknown user) both returned `404`, while `AUTH_006` (bad credentials) returned `401`. The messages for `AUTH_004` and `AUTH_006` are word-for-word identical — deliberately, so that nobody can tell "that account does not exist" from "wrong password". The status code gave away exactly what the message was written to hide, which makes the login endpoint an **enumeration oracle**: one request per candidate email separates real accounts from invented ones. +- **What the second one actually was**: not a contract nicety. `AUTH_002` (unknown tenant) and `AUTH_004` both returned `404`, while `AUTH_006` (bad credentials) returned `401`. The messages for `AUTH_004` and `AUTH_006` are word-for-word identical — deliberately, so that nobody can tell one outcome from the other. The status code gave away exactly what the message was written to hide. Which oracle each one is, precisely: `AUTH_002` is the anonymous one — the tenant lookup precedes any credential check, so one request per candidate separates real tenant codes from invented ones with no credentials at all. `AUTH_004` is **not** "unknown user" (the local branch answers `AUTH_006` for that, and never leaked); it fires only after a successful IdP authentication, so it is an oracle for someone already inside: which tenants a federated identity has a UMS account in. See [ADR-0165](./adrs/0165-login-never-answers-404.md) — its first draft conflated the two and was corrected. - **Why it went unnoticed**: the contract suite had been red since the resync, so a genuine finding was sitting inside a check everyone had learned to expect red. The pact was right and nobody read it. - **A second, quieter defect in the same file**: the `Content-Type` assertion used `Match.Type("application/problem+json")`. A type matcher on a string matches **any** string, so that line reported OK against a content type the API no longer sends. The check existed and checked nothing — the same shape as the GraphQL-key finding in TD-004. @@ -190,7 +190,8 @@ What the gate had been hiding — each of these is a live defect, not a type-ann - `Content-Type` is now asserted by exact value. - `AUTH_002` → `400`; `AUTH_004` → `401`, indistinguishable from `AUTH_006`. **No login branch returns 404.** Recorded as [ADR-0165](./adrs/0165-login-never-answers-404.md), because 401-where-REST-would-say-404 is the kind of thing a future reader corrects back unless the reason is written down. - The front end needed no change: `auth.service.ts` selects its message from the body's `code`, never from the status. +- `/client/authenticate` had the same 404 for `AUTH_004` — inside the endpoint G-053 had hardened, where every message is already the same string and the status was the last signal left. Now 401 as well (ADR-0165 §2.4). No test asserted the old 404. - **Verified**: `Ums.ContractTest` 38/38. -- **Still owed**: report upstream — `unimar-ums` has the same `MapAuthError`. +- **Reported upstream**: `unimar-ums` has the same `MapAuthError`, the same `ClientAuthEndpoints` 404 and the same pact defects — [unimar-peru/unimar-ums#214](https://github.com/unimar-peru/unimar-ums/issues/214). diff --git a/src/apps/ums.api/Ums.Presentation/Endpoints/Identity/Auth/ClientAuthEndpoints.cs b/src/apps/ums.api/Ums.Presentation/Endpoints/Identity/Auth/ClientAuthEndpoints.cs index 8352bb20..85b4e28f 100644 --- a/src/apps/ums.api/Ums.Presentation/Endpoints/Identity/Auth/ClientAuthEndpoints.cs +++ b/src/apps/ums.api/Ums.Presentation/Endpoints/Identity/Auth/ClientAuthEndpoints.cs @@ -469,13 +469,19 @@ private static async Task HandleClientAuthenticateAsync( // (AUTH_002) y «tenant inactivo» (AUTH_003) NO deben distinguirse de «credenciales // inválidas» (AUTH_006). Antes AUTH_002→404 y AUTH_003→400 permitían enumerar códigos // de inquilino comparando el status contra el 401 de credenciales. Ahora todos colapsan - // a 401 con un mensaje genérico e indistinguible. AUTH_004/005 (nivel usuario, tras auth - // del IDP) y AUTH_011/012 (infraestructura) conservan su semántica. + // a 401 con un mensaje genérico e indistinguible. AUTH_011/012 (infraestructura) + // conservan su semántica: no ponen ninguna credencial en duda. + // + // ADR-0165 §2.4: AUTH_004 se sumó al colapso, tarde. `SpanishMessage` ya devolvía para él + // la MISMA cadena exacta que para AUTH_002/003/006/017 —aquí no hay humano a quien darle + // la pista—, así que el 404 era la única señal que quedaba en pie, y distinguía «el IdP te + // autenticó pero no tienes cuenta UMS en este inquilino» de «no te autenticaste». Con el + // cuerpo ya uniformado, el status era el último resquicio del oráculo que G-053 cerró. private static int GetStatusCode(string error) => error switch { var e when e.StartsWith("AUTH_002") => StatusCodes.Status401Unauthorized, var e when e.StartsWith("AUTH_003") => StatusCodes.Status401Unauthorized, - var e when e.StartsWith("AUTH_004") => StatusCodes.Status404NotFound, + var e when e.StartsWith("AUTH_004") => StatusCodes.Status401Unauthorized, var e when e.StartsWith("AUTH_005") => StatusCodes.Status401Unauthorized, var e when e.StartsWith("AUTH_006") => StatusCodes.Status401Unauthorized, // ADR-UMS-095: bloqueo temporal de cuenta (AUTH_017). Anti-enumeración G-053: en este endpoint