Skip to content

Commit 92d68ad

Browse files
mxaminclaude
andcommitted
Convert every remaining raw-node crossing to shadow copies (#356)
Roll the shadow pattern out beyond templates so no binding hands an lxml node to xmlsec anymore (fast path unchanged on matched libxml2): - template.c: all remaining functions, incl. the create shape (BeginNewDoc/EndNewDoc builds the detached template in a private doc) and the status-int C14N helper (FindFresh locates the created node). - tree.c: finders map results back by path (EndFind, None on not-found; find_parent shadows the whole tree); add_ids records id-attribute specs instead of writing lxml's ID hash with our libxml2. - ds.c: sign/verify run on a whole-document copy (BeginDoc) with the recorded IDs replayed so #id references resolve; sign reflects all mutation sites (DigestValue/SignatureValue/KeyInfo) via ReflectAll, verify just discards the copy. - enc.c: encrypt_binary/encrypt_uri reflect the mutated template; encrypt_xml/decrypt re-parse everything into one copy and reflect the replacement through lxml (element, content, or returned bytes). Replacing the document root cannot be expressed through lxml's API and raises a clear error on the shadow path. ReflectAll is two-phase (prefetch payloads from the re-parsed copy in its final state, then apply to the live tree in document order) because each graft moves a node out of the copy and would invalidate the indices later sites resolve through. Two template tests now attach the created template before asserting liveness: a shadow-created template lives in its own document until grafted, as lxml cannot express the raw path's "detached node inside an existing document". Validated under a real 2.14<->2.15 mismatch (full suite, 10k-iteration sign/verify/encrypt/decrypt loop, flat RSS, byte-identical output) and on a matched static wheel with and without PYXMLSEC_FORCE_SHADOW. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 72a78cb commit 92d68ad

9 files changed

Lines changed: 1498 additions & 106 deletions

File tree

converting-functions.md

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,18 @@ grep -n '_c_node\|_c_doc' src/template.c
1919
Read the `xmlSecTmpl*` function it wraps (xmlsec1's `src/templates.c`) and
2020
match it to a row:
2121

22-
| Shape | Examples | Covered? |
22+
| Shape | Examples | How |
2323
|---|---|---|
2424
| Adds a subtree under the element (any position, possibly with intermediate nodes) | `add_key_name`, `add_key_value`, `add_x509_data`, `x509_data_add_*`, `add_encrypted_key` | ✅ just follow step 3 |
25-
| Find-or-create, may set attributes on the existing node | `encrypted_data_ensure_key_info`, `encrypted_data_ensure_cipher_value` | ✅ just follow step 3 |
26-
| Returns a status `int`, mutates one child | `transform_add_c14n_inclusive_namespaces` | ✅ with a twist: after the call, locate the affected child on the copy (here: the `<InclusiveNamespaces>` under `shadow.root`) and pass *that* to `End`; discard the returned element and return `None` |
27-
| Returns a **detached** node — the element argument only supplies the document | `create`, `encrypted_data_create` |`End` would raise "unexpected result node"; needs its own reflect (no graft — parse the standalone result with lxml and return it) |
28-
| Reads or mutates the **whole document** | `ds.c` sign/verify, `enc.c` encrypt/decrypt, `tree.c` | ❌ needs a whole-document reflect strategy |
25+
| Find-or-create, may set attributes on the existing node | `encrypted_data_ensure_key_info`, `encrypted_data_ensure_cipher_value` | ✅ just follow step 3 (a call that can also *rename the prefix* of the existing node uses `EndReplace` instead of `End`) |
26+
| Returns a status `int`, mutates one child | `transform_add_c14n_inclusive_namespaces` | ✅ pass `PyXmlSec_LxmlShadowFindFresh(&shadow)` to `End`; discard the returned element and return `None` |
27+
| Returns a **detached** node — the element argument only supplies the document | `create`, `encrypted_data_create` |`BeginNewDoc`/`EndNewDoc`: the call runs against a private document and the result comes back as a new detached lxml element |
28+
| Read-only search | `tree.c` find_child/find_node (subtree), find_parent (whole doc) |`Begin` (or `BeginDoc` when the search leaves the subtree) + `EndFind`, which returns `None` on not-found |
29+
| Reads or mutates the **whole document**, possibly at several places | `ds.c` sign (whole-doc + multi-site), verify (read-only) |`BeginDoc` + `ReplayIds`, then `ReflectAll` (sign) or `Discard` (verify) |
30+
| **Replaces** nodes | `enc.c` encrypt_xml/decrypt (`encrypt_binary`/`encrypt_uri` are template-shaped: `Begin` + `ReflectAll`) |`BeginDoc`, remove the consumed live node/content first, then `ReflectAll` grafts the replacement (`_setroot` when the document root itself was replaced) |
31+
32+
All shapes are converted; `developer.md`'s "Beyond templates" section explains
33+
each reflect. This guide stays as the recipe should new bindings appear.
2934

3035
`res` does not have to be the topmost node the call created — `End` walks up
3136
to the topmost new ancestor itself. Any node inside the new subtree works.

developer.md

Lines changed: 75 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,54 @@ Child indices count exactly the node types lxml exposes as children (elements,
5656
comments, PIs, entity refs), so paths recorded on the raw copy resolve
5757
identically through lxml's `__getitem__`/`insert`. `End` assumes the xmlsec
5858
call mutates at most one place in the tree — true for all `xmlSecTmpl*`
59-
functions. Whole-document operations (`sign`, `encrypt`) mutate many places
60-
and will need a different reflect step on top of the same Begin machinery.
59+
functions.
60+
61+
## Beyond templates: the whole-code rollout
62+
63+
Every binding that used to hand a raw lxml node to xmlsec now goes through a
64+
shadow. The extra shapes (all in [src/lxml.c](src/lxml.c), contracts in
65+
[src/lxml.h](src/lxml.h)):
66+
67+
- **Create** (`template.create`, `encrypted_data_create`):
68+
`BeginNewDoc`/`EndNewDoc`. The call builds a *detached* subtree and only
69+
needs a document to allocate in — a private one on the shadow path. The
70+
result comes back as a new detached lxml element (in its own document; lxml
71+
moves it when the caller grafts it), instead of the raw path's "detached
72+
node inside the source document", which lxml's API cannot express.
73+
- **Finders** (`tree.find_child`/`find_node`/`find_parent`): `EndFind` maps
74+
the found copy node back by path and returns `None` on not-found.
75+
`find_parent` walks upward, so it uses the whole-document Begin.
76+
- **Whole-document** (`sign`, `verify`, `decrypt`, `find_parent`):
77+
`BeginDoc` serializes `element.getroottree()` (comments/PIs outside the
78+
root and the internal DTD subset survive), records the element's position
79+
through lxml's API, and hands back the copy's counterpart node.
80+
- **Multi-site reflect** (`sign`, `encrypt_binary`, `encrypt_uri`):
81+
`ReflectAll` scans the copy for *every* topmost untagged node and grafts
82+
each back — new subtrees via `insert`, new/changed text (DigestValue,
83+
SignatureValue) via the text slots. Two-phase: payloads are fetched from
84+
the re-parsed copy while it is still in its final state, then applied to
85+
the live tree in document order (a graft moves a node out of the re-parsed
86+
copy, which would invalidate later fetches).
87+
- **Replacement reflect** (`encrypt_xml`, `decrypt`): encryption/decryption
88+
*replace* nodes, so the live target (or its content, or the document root
89+
via `_setroot`) is removed first and `ReflectAll` grafts what took its
90+
place. `encrypt_xml` re-serializes the template into the same shadow doc
91+
(`ImportElement`); a template attached inside the target's own tree is
92+
therefore copied, not moved. `verify` needs no reflect at all — `Discard`
93+
just frees the copy.
94+
- **ID registration** (`tree.add_ids`, `SignatureContext.register_id`): these
95+
used to write lxml's ID hash with our libxml2. Under the shadow they record
96+
the id-attribute specs in a registry keyed by document identity
97+
(`RecordId`), and every `BeginDoc` replays them onto the copy
98+
(`ReplayIds`) so `#id` references resolve. The replay scans the whole copy
99+
for the recorded attribute names — a superset of the raw registration. The
100+
registry holds no strong references to documents (lxml objects refuse weak
101+
references, so entries are validated by a stored `_c_doc` address and
102+
capped in size).
103+
- **Prefix rename** (`encrypted_data_ensure_key_info(ns=...)` on an existing
104+
KeyInfo): `EndReplace` swaps the live element for the copy's version, since
105+
lxml cannot rename a prefix in place; the returned element is then a new
106+
object rather than the original proxy.
61107

62108
## The fast path: shadows only when needed
63109

@@ -114,11 +160,30 @@ on matched libraries — safe everywhere, so the whole suite must pass).
114160

115161
## Status
116162

117-
-`template.add_reference`, `template.add_transform`,
118-
`template.ensure_key_info` — one per reflect shape. Validated under a real
119-
2.14 ↔ 2.15 mismatch: full suite green, 10k-iteration loop with no crash,
120-
no leak, byte-identical output.
121-
- ⬜ Rest of `src/template.c` — mechanical: follow the step-by-step guide in
122-
[converting-functions.md](converting-functions.md).
123-
-`src/ds.c` (sign/verify), `src/enc.c` (encrypt/decrypt), `src/tree.c`
124-
need a whole-document reflect strategy.
163+
- ✅ All of `src/template.c` (create, references, transforms, key info, x509,
164+
encrypted data, C14N namespaces).
165+
-`src/tree.c` (find_child/find_node/find_parent, add_ids).
166+
-`src/ds.c` (register_id, sign, verify; the binary operations never
167+
touched nodes).
168+
-`src/enc.c` (encrypt_binary, encrypt_uri, encrypt_xml, decrypt).
169+
- Validated under a real 2.14 ↔ 2.15 mismatch: full suite green, 10k-iteration
170+
sign/verify/encrypt/decrypt loop with no crash, no leak, byte-identical
171+
output; and on a matched static build: full suite green on both the fast
172+
path and `PYXMLSEC_FORCE_SHADOW=1`.
173+
- ⬜ Endgame: turn the import-time guard into a mode switch (mismatch sets
174+
the shadow flag instead of refusing to import) and retire
175+
`PYXMLSEC_SKIP_VERSION_CHECK`. That is the actual user-facing resolution
176+
of #356, kept as its own change.
177+
178+
Known, deliberate divergences on the shadow path (all invisible to the
179+
documented API): created templates live in their own document until grafted;
180+
`encrypted_data_ensure_key_info(ns=...)` on an existing KeyInfo returns a new
181+
element object; `register_id` skips the live duplicate-id check (it runs per
182+
copy instead); `encrypt_xml` copies rather than moves a template that is
183+
attached inside the target tree; signature/encryption contexts keep no live
184+
result nodes after the call (they never usefully did). One hard limitation:
185+
operations that would replace the **document root** (encrypting the root
186+
element, decrypting a root `EncryptedData`) raise `xmlsec.Error` — lxml's API
187+
cannot swap a document's root, and morphing it in place would rewrite
188+
namespace prefixes, breaking signatures over the content. Re-parse the
189+
document or work on a subelement instead.

src/ds.c

Lines changed: 65 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,39 @@ static PyObject* PyXmlSec_SignatureContextRegisterId(PyObject* self, PyObject* a
146146
goto ON_FAIL;
147147
}
148148

149+
// Shadow mode: never touch lxml's document (its ID hash) with our libxml2
150+
// (issue #356). Validate through lxml's API and record the spec; the
151+
// whole-document shadows (sign/verify/decrypt) replay it onto their
152+
// private copies. The duplicate-id check runs there, per copy.
153+
if (PyXmlSec_LxmlShadowIsActive()) {
154+
PyObject* key;
155+
PyObject* value;
156+
if (id_ns != NULL) {
157+
key = PyUnicode_FromFormat("{%s}%s", id_ns, id_attr);
158+
} else {
159+
key = PyUnicode_FromString(id_attr);
160+
}
161+
if (key == NULL) {
162+
goto ON_FAIL;
163+
}
164+
value = PyObject_CallMethod((PyObject*)node, "get", "O", key);
165+
Py_DECREF(key);
166+
if (value == NULL) {
167+
goto ON_FAIL;
168+
}
169+
if (value == Py_None) {
170+
Py_DECREF(value);
171+
PyErr_SetString(PyXmlSec_Error, "missing attribute.");
172+
goto ON_FAIL;
173+
}
174+
Py_DECREF(value);
175+
if (PyXmlSec_LxmlShadowRecordId(node, id_attr, id_ns) < 0) {
176+
goto ON_FAIL;
177+
}
178+
PYXMLSEC_DEBUGF("%p: register id - ok", self);
179+
Py_RETURN_NONE;
180+
}
181+
149182
if (id_ns != NULL) {
150183
attr = xmlHasNsProp(node->_c_node, XSTR(id_attr), XSTR(id_ns));
151184
} else {
@@ -189,21 +222,39 @@ static PyObject* PyXmlSec_SignatureContextSign(PyObject* self, PyObject* args, P
189222

190223
PyXmlSec_SignatureContext* ctx = (PyXmlSec_SignatureContext*)self;
191224
PyXmlSec_LxmlElementPtr node = NULL;
225+
xmlNodePtr target;
226+
PyXmlSec_LxmlShadow shadow;
192227
int rv;
193228

194229
PYXMLSEC_DEBUGF("%p: sign - start", self);
195230
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&:sign", kwlist, PyXmlSec_LxmlElementConverter, &node)) {
196231
goto ON_FAIL;
197232
}
198233

234+
// References (URI="", "#id") reach anywhere in the document, so the
235+
// shadow covers the whole tree (issue #356); registered IDs are replayed
236+
// onto the copy so they resolve. Signing fills several places inside
237+
// <Signature> (DigestValue, SignatureValue, KeyInfo), all reflected by
238+
// the multi-site reflection.
239+
if (PyXmlSec_LxmlShadowBeginDoc(&shadow, node, &target) < 0) {
240+
goto ON_FAIL;
241+
}
242+
if (PyXmlSec_LxmlShadowReplayIds(&shadow) < 0) {
243+
PyXmlSec_LxmlShadowDiscard(&shadow);
244+
goto ON_FAIL;
245+
}
199246
Py_BEGIN_ALLOW_THREADS;
200-
rv = xmlSecDSigCtxSign(ctx->handle, node->_c_node);
247+
rv = xmlSecDSigCtxSign(ctx->handle, target);
201248
PYXMLSEC_DUMP(xmlSecDSigCtxDebugDump, ctx->handle);
202249
Py_END_ALLOW_THREADS;
203250
if (rv < 0) {
251+
PyXmlSec_LxmlShadowDiscard(&shadow);
204252
PyXmlSec_SetLastError("failed to sign");
205253
goto ON_FAIL;
206254
}
255+
if (PyXmlSec_LxmlShadowReflectAll(&shadow) < 0) {
256+
goto ON_FAIL;
257+
}
207258
PYXMLSEC_DEBUGF("%p: sign - ok", self);
208259
Py_RETURN_NONE;
209260

@@ -224,17 +275,29 @@ static PyObject* PyXmlSec_SignatureContextVerify(PyObject* self, PyObject* args,
224275

225276
PyXmlSec_SignatureContext* ctx = (PyXmlSec_SignatureContext*)self;
226277
PyXmlSec_LxmlElementPtr node = NULL;
278+
xmlNodePtr target;
279+
PyXmlSec_LxmlShadow shadow;
227280
int rv;
228281

229282
PYXMLSEC_DEBUGF("%p: verify - start", self);
230283
if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O&:verify", kwlist, PyXmlSec_LxmlElementConverter, &node)) {
231284
goto ON_FAIL;
232285
}
233286

287+
// Verification is read-only: whole-document shadow, replayed IDs, and no
288+
// reflection at all — the copy is simply discarded.
289+
if (PyXmlSec_LxmlShadowBeginDoc(&shadow, node, &target) < 0) {
290+
goto ON_FAIL;
291+
}
292+
if (PyXmlSec_LxmlShadowReplayIds(&shadow) < 0) {
293+
PyXmlSec_LxmlShadowDiscard(&shadow);
294+
goto ON_FAIL;
295+
}
234296
Py_BEGIN_ALLOW_THREADS;
235-
rv = xmlSecDSigCtxVerify(ctx->handle, node->_c_node);
297+
rv = xmlSecDSigCtxVerify(ctx->handle, target);
236298
PYXMLSEC_DUMP(xmlSecDSigCtxDebugDump, ctx->handle);
237299
Py_END_ALLOW_THREADS;
300+
PyXmlSec_LxmlShadowDiscard(&shadow);
238301

239302
if (rv < 0) {
240303
PyXmlSec_SetLastError("failed to verify");

0 commit comments

Comments
 (0)