feat: jpa, messaging, notification, mongo, graphql 어댑터터 리펙토링
This commit is contained in:
@@ -0,0 +1,116 @@
|
||||
# Notification payload at rest — threat model and decision (NTF-INT-007)
|
||||
|
||||
Wave 2 Task D6 offers two branches and requires that one be chosen and implemented fully:
|
||||
|
||||
- **(a) encryption** — a codec/port, ciphertext envelope, key id, rotation and history, row migration,
|
||||
and a decryption-failure contract;
|
||||
- **(b) restriction** — this document plus a static restriction proving the variable types cannot
|
||||
carry sensitive values.
|
||||
|
||||
The plan recommends (b) **"if and only if the variable types can genuinely be restricted to
|
||||
non-sensitive values"**. They cannot. This document records why, what that leaves, and when the
|
||||
remaining branch lands.
|
||||
|
||||
## What is stored, and where
|
||||
|
||||
`CanonicalNotificationPlanWriter.request(...)` puts `encoded.variablesPayload()` into
|
||||
`NotificationRequestRecord` verbatim. `JpaNotificationRequestStore` writes that record to
|
||||
`notification_request.variables_payload` with no transformation. There is no encryption anywhere on
|
||||
this path.
|
||||
|
||||
## Why the restriction branch is unavailable
|
||||
|
||||
Template variables are a closed algebra — `NotificationVariable` permits `TextValue`, `NumberValue`,
|
||||
`BooleanValue`, `NullValue`, `ListValue`, `ObjectValue` — which is a real improvement over the
|
||||
`Map<String, Object>` it replaced. But `TextValue` holds an arbitrary UTF-8 string of up to 8 KiB,
|
||||
and that is not an oversight to be tightened: **the variables are the recipient-specific content of
|
||||
the message**. A password-reset code, an order total, a delivery address, a patient's appointment
|
||||
time — those are what a notification is for.
|
||||
|
||||
A restriction to "non-sensitive values" would therefore be one of two things, and both are worse
|
||||
than the problem:
|
||||
|
||||
- **unenforceable** — a comment saying callers should not put sensitive data in a field designed to
|
||||
carry the message's content, which is a policy no type checks and no reviewer can see violated;
|
||||
- **enforced and useless** — a type that refuses free text, which does not restrict the capability so
|
||||
much as delete it.
|
||||
|
||||
The precondition on the plan's recommendation is false. Branch (b) is not available.
|
||||
|
||||
> **Status: branch (a) implemented at the storage boundary.** `NotificationPayloadProtection` is the
|
||||
> application-owned port, `AesGcmNotificationPayloadProtection` the AES-GCM implementation, and
|
||||
> `NotificationRecordMapper` applies it — as a **required** constructor argument, so a composition
|
||||
> cannot assemble the notification stores while leaving the payload in plaintext. What remains before
|
||||
> the facade can be imported is the row migration for any deployment that already has plaintext rows,
|
||||
> and the `local-notification-*` lanes. The analysis below is kept as written, because it is what the
|
||||
> decision rests on.
|
||||
|
||||
## Decision: branch (a), landing with the persistence wiring
|
||||
|
||||
Encryption is therefore the required branch. Its scope is unchanged from the plan: a codec behind an
|
||||
application port, a ciphertext envelope carrying its key id, key rotation with history so an old row
|
||||
stays readable, a migration for existing rows, and an explicit contract for what a decryption failure
|
||||
does to a request.
|
||||
|
||||
**It lands in the change unit that makes the write path reachable, and not before.** The reason is a
|
||||
fact the spec did not have: `NotificationJpaPersistenceFacade`, which assembles
|
||||
`JpaNotificationRequestStore`, is imported by nothing. The composition root's component scan excludes
|
||||
the persistence package by design, and no configuration imports the facade — so the notification
|
||||
capability has **no JPA persistence at all**, and no deployment currently writes this payload
|
||||
anywhere. The defect is real in the code and latent in the runtime.
|
||||
|
||||
Designing key rotation and a row migration for rows that no deployment produces would be building the
|
||||
migration before the table. Worse, it would settle the envelope's shape before the store that has to
|
||||
read it is wired, which is the order that produces an envelope the store cannot use.
|
||||
|
||||
**One correction, learned by trying it.** This section said the envelope "lands with the wiring".
|
||||
Wiring the facade first — to register the SMTP assembler — made
|
||||
`NotificationPayloadAtRestContractTest` fail on the case asserting the write path is reachable from
|
||||
no composition, which is exactly what that case is for. The wave forbids connecting wiring over a
|
||||
known security finding on a runtime path, so the wiring was reverted and the envelope built first.
|
||||
The honest ordering is **envelope before or with the wiring, never after**, and the contract test now
|
||||
enforces it by failing on the wiring alone.
|
||||
|
||||
### The envelope, and why it has a key id
|
||||
|
||||
```
|
||||
byte version always 1
|
||||
byte keyIdLength 1..255 UTF-8 bytes
|
||||
byte[] keyId
|
||||
byte[12] nonce
|
||||
byte[] ciphertext + GCM tag
|
||||
```
|
||||
|
||||
The key id is the reason there is a format at all. This repository's callback protection stores nonce
|
||||
and ciphertext and nothing else, so the day the active key changes, every row written under the
|
||||
previous one becomes unreadable and nothing in the row can say which key it needed — that is not a
|
||||
rotation story with a gap in it, it is the absence of one. `SecretMaterialProvider` already exposes
|
||||
`keyById`, so reading the id back and asking for that specific key makes rotation a change of default
|
||||
rather than a data migration. The version byte costs one byte and is what allows the format to change
|
||||
at all.
|
||||
|
||||
The header is passed as **AAD**, not merely prefixed: without that, the key id is attacker-editable
|
||||
and an envelope could be redirected at a key of the attacker's choosing.
|
||||
|
||||
A failed decryption throws `NotificationPayloadUnreadableException` rather than returning empty. A
|
||||
caller handed an empty payload renders every variable as nothing and sends "Hello , your code is " to
|
||||
a real person — the failure delivered instead of reported. All three causes (unknown key, wrong key,
|
||||
modified ciphertext) collapse into one message, because telling them apart tells an attacker which of
|
||||
the three they achieved.
|
||||
|
||||
## What must not be done instead
|
||||
|
||||
**Requiring `PAYLOAD_ENCRYPTION` in `INGEST_ONLY` is not a fix.** That secret is consumed by exactly
|
||||
one thing — `AesGcmCallbackPayloadProtection`, which protects raw callback bodies — and by nothing on
|
||||
the accept path. Demanding it would make a deployment supply a key that protects nothing while the
|
||||
payload it appears to be about stays in plaintext. The repository already has one defect of that
|
||||
exact shape: `backend.graphql.cursor.key-ids`, which production refuses to start without and which no
|
||||
code signs a cursor with (GQL-INT-003). Adding a second would make the pattern a habit.
|
||||
|
||||
## Consequence
|
||||
|
||||
Notification is **not promoted to Stable**, per the index's scope boundaries, until branch (a) is
|
||||
complete. The three notification Compose lanes stay non-blocking. `NotificationPayloadAtRestContractTest`
|
||||
holds every fact this decision rests on, so the decision expires automatically if any of them stops
|
||||
being true — in particular, the assertion that no encryption sits on the accept path fails the moment
|
||||
somebody adds one, which is the change this document is waiting for.
|
||||
@@ -15,7 +15,7 @@ when this page, the YAML tree and `docs/registries/env-keys.yaml` disagree.
|
||||
| Property | Environment variable | Default | Meaning |
|
||||
|---|---|---|---|
|
||||
| `enabled` | `APP_NOTIFICATION_PLATFORM_ENABLED` | `false` | Binds nothing at all while false: no runtime, no schema check, no scheduler thread, no secret required |
|
||||
| `mode` | `APP_NOTIFICATION_PLATFORM_MODE` | `SERVING` | `SERVING` refuses to start without a working provider; `ACCEPT_ONLY` stores requests and does not dispatch |
|
||||
| `mode` | `APP_NOTIFICATION_PLATFORM_MODE` | `SERVING` | `SERVING` refuses to start without a working provider; `INGEST_ONLY` stores requests and does not dispatch |
|
||||
|
||||
## Dispatch
|
||||
|
||||
|
||||
@@ -1,8 +1,8 @@
|
||||
# Notification Delivery Platform — module mapping
|
||||
|
||||
> Source design: `notification-superpowers-package/docs/superpowers/specs/2026-08-10-notification-platform-design.md`
|
||||
> Source design: `docs/superpowers/specs/2026-08-10-notification-platform-design.md`
|
||||
>
|
||||
> Source plan: `notification-superpowers-package/docs/superpowers/plans/2026-08-10-notification-platform-implementation-plan.md`
|
||||
> Source plan: `docs/superpowers/plans/2026-08-10-notification-platform-implementation-plan.md`
|
||||
|
||||
## Why a mapping exists
|
||||
|
||||
|
||||
Reference in New Issue
Block a user