Repository navigation
Use AAD in the payload encryption scheme #146
pseudomuto
started this conversation in
General
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
A number of people have reached out asking about having WorkflowID and/or ActivityType bound as AEAD additional
authenticated data (AAD) and validated on decrypt. The ultimate goal is to prevent ciphertext in one "context" from
being replayed and decrypted in another.
I spent a little time thinking about how this could work, and figured I'd start a discussion about it. What follows
here, at least before any discussion, is my initial understanding of the problem and some ways we could go about
addressing it using AAD. By no means is this meant to be exhaustive or perfectly accurate.
What AAD actually does
An AEAD seal is
Seal(nonce, plaintext, aad) -> ciphertext||tag. The AAD is hashed into the auth tag but is notencrypted and does not appear in the output.
Openrecomputes the tag over the ciphertext plus whatever AAD you handit. So, if it isn't bit-identical to what was sealed, you get an error, not plaintext.
It binds a ciphertext to a context, so a ciphertext produced in context A can't be relocated into context B. Effectively
anti-cut-and-paste. That's the whole feature.
I can think of two invariants/rules that govern whether a given binding is worth anything here:
Opensite must agree with theSealsite.The first rule is that if we stash the value in the payload's Metadata next to the blob, for example, an attacker who
moves the blob also moves the metadata. Therefore, the tag validates, and we've added a field with zero security. 😬
The second rule states that a binding is safely enforceable only if, for every site where a payload legitimately
appears, every payload arriving there was sealed somewhere carrying the same descriptor. Otherwise, some payloads at
that site are unbound, meaning they have to stay openable, and an attacker can just use those.
Rule 2 is what separates ActivityType from WorkflowID here IMO. More on that below.
Why ActivityType works
Our codec runs on the payload visitor from Temporal's API. The callback receives
VisitPayloadsContext.Parent, which is documented as "the parent message for this payload". The visitor passes the enclosing typed message, not the Payloads wrapper. So for an activity input, Parent is*command.ScheduleActivityTaskCommandAttributes, which hasactivity_typeandactivity_idright there alongside input.I (looking at you, Claude) enumerated every parent message in the visitor that carries an input payload and checked what
descriptors sit next to it. Full result, against Temporal API v1.62.8:
ScheduleActivityTaskCommandAttributes,ActivityTaskScheduledEventAttributes,PollActivityTaskQueueResponse,StartActivityExecutionRequest,DescribeActivityExecutionResponseactivity_type(andactivity_id); the describe response needs one hop intoinfoStartWorkflowExecutionRequest,SignalWithStartWorkflowExecutionRequest,StartChildWorkflowExecutionCommandAttributes/...InitiatedEventAttributes,ContinueAsNewWorkflowExecutionCommandAttributes,WorkflowExecutionContinuedAsNewEventAttributes,WorkflowExecutionStartedEventAttributes,NewWorkflowExecutionInfoworkflow_typeSignalWorkflowExecutionRequest,SignalExternalWorkflowExecution*,WorkflowExecutionSignaledEventAttributes,BatchOperationSignal,PostResetOperation_SignalWorkflowupdate.InputnameScheduleNexusOperationCommandAttributes,NexusOperationScheduledEventAttributesservice+operationRespondActivityTaskCompletedRequest,ActivityTaskCompletedEventAttributesCompleteWorkflowExecutionCommandAttributes,WorkflowExecutionCompletedEventAttributesRecordActivityTaskHeartbeatRequestThe split here seems to roughly be inputs versus results. An input message has to say what it is invoking, whereas a
result is correlated with its scheduling event via scheduled_event_id, so it carries no descriptor at all.
For activity inputs specifically, all five seal/open sites carry
activity_type, so rule 2 above is satisfied and thebinding is symmetric/complete for that payload class.
Why WorkflowID does not work
Well, in sum...continue-as-new (CAN) kills it 💀
A CAN'd workflow's input is sealed at
ContinueAsNewWorkflowExecutionCommandAttributes, which hasworkflow_typebutno
workflow_id. The same stored bytes are served back at the new run'sWorkflowExecutionStartedEventAttributes,which does have
workflow_id.So one open site legitimately receives both workflow-id-bound payloads (normal starts) and unbound ones (CAN), which
violates rule 2 and makes the check bypassable. You could try to discriminate on
continued_execution_run_idbeingnon-empty or something, but that field comes from the untrusted upstream, so an attacker just sets it to downgrade to
unbound.
There is also the task-token problem.
RespondWorkflowTaskCompleted,RespondActivityTaskCompleted,RecordActivityTaskHeartbeat, andRespondQueryTaskCompletedcarry only an opaquetask_token. Getting a WorkflowIDthere means parsing a server-internal proto with no stability guarantee, or making the proxy stateful across poll and
respond. Neither is acceptable IMO.
Important
The SDK can do things we can't. The worker already holds the activity task info in memory, or the caller passes
identifiers in explicitly, as in
CompleteActivityWithOptions, which takes WorkflowID, ActivityType, WorkflowType,and TaskQueue next to the TaskToken.
The SDK allows for this already
For example, the Go SDK v1.42.0 added
converter.SerializationContext:exposed via
DataConverterWithSerializationContextandPayloadCodecWithSerializationContext. The doc comment callsout this use case verbatim! ("...using workflow ID as associated data for encryption.")
So ActivityType is explicitly among the things the SDK expects codecs to bind to. It's worth noting that the SDK path is
strictly more capable than ours because the worker holds task state. It can bind activity results too, which we cannot.
It also gets provenance right in a way we'd have to be very careful about. Things like for child workflows WorkflowID is
the child's ID, not the parent's. For external signals, WorkflowID is the target workflow's ID.
The SDK repo is pretty blunt about stating our predicament. From their own gRPC-interceptor codec, which wraps the same
proxy.NewPayloadVisitorInterceptorwe use:That is true for WorkflowID. But I don't believe it's true for ActivityType, because that one is always in the message.
The one real blocker, and a potential fix
Parent messages mix payload classes, and the visitor tells us the parent but not the field.
PollActivityTaskQueueResponsevisits Header, HeartbeatDetails, and Input under one parent. Input must be bound toactivity_type; HeartbeatDetails was sealed at RecordActivityTaskHeartbeat with no descriptor and must stay unbound.WorkflowExecutionStartedEventAttributesvisits ContinuedFailure, Header, Input, and LastCompletionResult under oneparent. LastCompletionResult is the previous run's result, sealed at
CompleteWorkflowExecutionCommandAttributeswithno descriptor.
So a "bind uniformly per parent message" scheme is wrong, and we need some field-level discrimination. Here are two
possible ways to handle that:
set of message types rather than all ~200 cases.
What this buys, and what it doesn't
Even without WorkflowID, I think this is worth implementing. By using ActivityType in AAD, feeding
ChargeCard's sealedinput to
RefundCardbecomes a decryption failure. Call that intra-workflow type confusion: both activities live underthe same WorkflowID, so WorkflowID binding does nothing about it. This is why I rank ActivityType above WorkflowID,
regardless of the implementation details.
Binding both would, of course, be better. ActivityType alone doesn't stop workflow A's
ChargeCardinput from beingreplayed into workflow B's
ChargeCard, and WorkflowID is what would close that. It just isn't available to us at thislayer, for the CAN and task-token reasons above.
This is nice for a lot of messages, but we should be realistic when it comes to the rest:
event structure, reorder/drop events, replay whole tasks, and invent identifiers. If the real ask is "prove this
history wasn't tampered with," that needs a signed per-workflow transcript kind of thing, which I think is a different
design and not solved here.
results remains undetectable at this layer.
ChargeCardgets payloads sealed underChargeCard. AAD binds context, not identity.Also, two properties of our envelope are worth noting:
common.Payloadwholesale, metadata and all, so compression's marker, for example, will be tag-coveredwhen it lands
just AEAD key agreement, not a binding: the key material is plaintext metadata beside the blob, so moving the pair
together still opens. It buys nothing toward context binding.
Namespace. Also AAD?
The namespace is the only binding available at every site in both directions. It's set unforgeably and independently of the
ciphertext, and both upstream serialization contexts also carry it, so it's the one value that works identically in proxy
and SDK deployments.
It also closes a potential gap.
Vault.Opennever receives a namespace, by design. The KEK is selected by ID from thekey material, so a payload sealed for Namespace A opens fine when presented in Namespace B as long as the KEK is
registered, which it always is under a default KEK. Today that separation is configuration only.
Warning
This proxy translates namespaces. The cipher's namespace comes from the local-name header, so the AAD must bind the
upstream name or payloads won't open under an SDK codec or a differently-configured proxy.
Some thoughts about implementation details
We'll need a binding descriptor in the envelope (read: new key in metadata), plus some kind of permissive/strict mode
setting. AAD isn't recoverable from the ciphertext, so the envelope must record which binding was used (e.g.
encryption-aad=ns+at) in its metadata, or the decrypt side has nothing to rebuild it from.This descriptor is susceptible to attack, but it doesn't have to be trusted so long as the binding type is folded into
the AAD bytes themselves. Rewriting
ns+attonsmakes the decryptor rebuild a different AAD, and the tag check fails.Every edit to the descriptor lands on "won't open," which is not different than what happens now when any change to
ciphertext is made upstream. So the descriptor is a hint for reconstruction, not a security concern.
The security concern is the decrypt-side policy. If an absent or weaker descriptor falls back to opening with no AAD, an
attacker never touches the descriptor, they just source an unbound copy instead. Payloads sealed before rollout, or the
classes we can't bind at all like results and heartbeat details. So each namespace needs a configured minimum, checked
before Open is attempted, rejecting anything below it rather than quietly falling back.
Caution
Strict mode isn't a flag you flip once a migration window closes. History is immutable and replayable for the whole
retention period, CAN chains and LastCompletionResult carry old payloads forward across runs, and anything sealed
before rollout stays unbound forever. Going strict makes those histories unopenable, so it's closer to a one-way
per-namespace decision than a rollout stage. We have to be very careful here.
We also need some kind of canonical, versioned AAD encoding. The requirement is injectivity, meaning two different field
tuples must never produce the same bytes. The scheme version, the binding type, and the key ID belong in there as fields
rather than left sitting in metadata.
Folding those in is what makes the descriptor safe to ignore above, and it also closes the gap noted earlier. The key
material is plaintext metadata beside the blob today, so putting the key ID inside the AAD is what would make the
DEK/ciphertext pairing genuinely tag-covered rather than merely mismatch-detected.
Naive concatenation only breaks down where two variable-length fields sit adjacent, so a namespace followed by a type
name is already injective when only the type varies. Nexus is one case that isn't.
serviceandoperationare bothnamed by whoever writes the service, so
("foo", "BarBaz")and("fooBar", "Baz")concatenate identically, which is abit of a showstopper.
Length-prefixing each field is a cheap fix, a byte or two apiece, and unlike a reserved delimiter it doesn't depend on a
magic string no one should use. For example, proto strings are valid UTF-8 and
0xFFnever appears in valid UTF-8, soit would work as a separator, but Go strings hold arbitrary bytes in memory and we'd be building AAD from in-memory
values before the marshal-time check ever runs (:boom:).
Canonical has to mean byte-exact across implementations, since a proxy and an SDK codec must produce identical AAD
for the same payload or nothing opens across the boundary. That means:
translation question above.
It would be immutable once written, same as the rest of the envelope. A version's encoding never changes, because old
payloads have to keep opening. Anything we get wrong or need to change becomes a new version number, not an edit.
These requirements rule out protobuf, which is otherwise a good candidate for this kind of thing. From
Caution
If we also implemented AAD in pkg/codec for SDK use, payloads sealed with SDK-only bindings (anything involving
WorkflowID or activity results) would be permanently unopenable by the proxy. That's not a migration problem. It's
permanent, so mixed deployments per namespace are unsupportable and this has to be an explicitly documented choice.
Things we could/should do
parsing, and it closes the intra-workflow type-confusion gap. Blocked only on field-level discrimination.
VisitPayloadsContextin api-go.(service + operation). Same property, same code path, no extra plumbing.
the canonical one before anything freezes.
Pass
availability.
Potential flow
Blue nodes are the four pieces that don't exist yet. Everything else is a path we already run, just with an AAD argument
that is
niltoday.flowchart TB L["new in this proposal"] U["api-go:<br>field descriptor on VisitPayloadsContext"] subgraph SEAL["Seal, outbound"] direction TB S1["visitor callback:<br>parent message + field"] S2{"bindable input field?"} S3["AAD = namespace"] S4["AAD = namespace + type descriptor:<br>activity_type, workflow_type,<br>signal name, update name,<br>Nexus service + operation"] S5["canonical AAD encoding:<br>versioned, length-prefixed,<br>scheme version and KEK ID folded in"] S6["DEK.Encrypt(pt, aad)"] S7["envelope metadata:<br>encryption-aad=ns+at"] S1 --> S2 S2 -- "no: results, heartbeat details" --> S3 S2 -- "yes" --> S4 S3 --> S5 S4 --> S5 S5 --> S6 S6 --> S7 end subgraph OPEN["Open, inbound"] direction TB O1["sealed payload,<br>encryption-aad descriptor in metadata"] O2{"descriptor meets<br>configured minimum?"} O3["reject, fail closed"] O4["rebuild AAD from<br>parent message + field"] O5["DEK.Decrypt(ct, aad)"] O6{"tag valid?"} O7["original payload restored"] O1 --> O2 O2 -- "no: below minimum" --> O3 O2 -- "yes" --> O4 O4 --> O5 O5 --> O6 O6 -- "no: wrong context" --> O3 O6 -- "yes" --> O7 end U --> S1 U --> O4 S7 -. "history, upstream" .-> O1 classDef added fill:#256abf,stroke:#3987e5,stroke-width:2px,color:#ffffff class L,U,S5,S7,O2 addedThe four: the upstream field descriptor, a canonical AAD encoder, the
encryption-aadenvelope key, and astrict/permissive mode that decides what to do with a descriptor weaker than the configured minimum.
Open questions
Four things, roughly in order of how much I think they'd change the shape of this:
confusion and not much else. Results stay unbound, history remains forgeable. I think it's worth shipping, but if the
ask behind "can we bind WorkflowID" is really "prove this history wasn't tampered with," then none of this answers it
, and we should say so plainly instead.
VisitPayloadsContextis small and purelyadditive, so I'm thinking it's likely not a concern. If there's a reason it can't land, the pointer-identity stopgap
is the fallback (and it's meaningfully worse).
one an SDK codec or a differently-configured proxy would also arrive at. Is there a case that breaks?
The SDK side binds anything the proxy can't compute, such as WorkflowID or activity results. If that's a dealbreaker for
anyone's deployment, much better to hear it now than after the envelope format is frozen.
All reactions