[#843] Cap the number of operations accepted per batchRequest in the DSML gateway - #844
Conversation
…nd the request body size in the DSML gateway Every batchRequest element of a SOAP body is executed over its own connection and bind, and password verification is deliberately expensive, so a small POST holding many batchRequest elements amplified into many binds; the SOAP message is also parsed into memory, so an unbounded body was an unbounded allocation. Cap both: ldap.dsml.batchrequests.max (default 1, as DSMLv2 describes a single batchRequest per body) and ldap.dsml.request.maxsize (default 10485760 bytes). Excess elements and oversized bodies are rejected with a notAttempted errorResponse; the declared Content-Length is refused without reading the body, and chunked bodies are capped while streamed.
Hoist the declared Content-Length check above the malformed-Content-Type fallback and guard it on an empty batchResponses: it used to add a second errorResponse next to a credentials error, and let the fallback SAX-parse an oversized body to recover the requestID. Stop echoing the configured caps to the unauthenticated client and drop the word batchRequest from the excess message, which counts every element of the SOAP body. Parse the context-params without NumberFormatException as control flow, reuse positiveValue() for ldap.dsml.dereference.anyuri.maxsize, rethrow ServletException out of init() instead of printStackTrace(), and close the request input stream with try-with-resources (CodeQL java/input-resource-leak). New tests: a credentials error is not doubled by the size check, an oversized body without a Content-Type is rejected unread with a single error and no requestID, a body of exactly the cap is accepted, and the excess-batch test now proves partial search results arrive next to the notAttempted error.
…batchRequest in the DSML gateway A compare on an attribute stored under a salted password scheme costs a full password verification, so an unbounded batch let a single authenticated POST buy on the order of 90k expensive verifications within the request-size cap. A batchRequest holding more operations than ldap.dsml.batchrequest.operations.max (default 10000: large batches are a designed use of DSMLv2) is now rejected as a whole with a notAttempted errorResponse before the gateway even connects.
maximthomas
left a comment
There was a problem hiding this comment.
LGTM
Merge preconditions
-
Merge #835 first. It is still
CHANGES_REQUESTED; the requested fixes are present in
102f1ecee9on this branch, so it needs a re-review, not new work. -
Release-note the new default. Unlike #835's
ldap.dsml.batchrequests.max=1— which broke
nothing, because a secondbatchRequestelement never worked before #811 — a cap of 10000
operations can reject batches that work today.DEFAULT_MAX_OPERATIONSlives in code
(DSMLServlet.java:149), so an existing deployment inherits it on upgrade without touching
itsweb.xml. Suggested note:The DSML gateway now accepts at most 10000 operations per
batchRequest. Bulk-provisioning
clients that send larger batches must raiseldap.dsml.batchrequest.operations.maxin
web.xml, or split their batches. Batches over the cap are rejected as a whole with a
notAttemptederrorResponse.
Severity note for the issue/PR text
The PR body says "one authenticated POST bought an unbounded amount of CPU". Verified against
this repo, that is right, and slightly conservative:
opendj-server-legacy/resource/config/config.ldif:92— the stock global ACI "Self entry read"
grants(read,search,compare)onuserPassword||authPasswordtoldap:///self. Any
authenticated user can self-compare their own password, so the amplifier is reachable under
default ACIs. (config.ldif:88only withholds it for other entries.)config.ldif:998— the default storage scheme is Salted SHA-1, which is cheap. The expensive
case needs PBKDF2/bcrypt configured, which the PR body already states correctly.- The shipped
web.xmlexample forldap.userdniscn=Directory Manager(commented out). A
gateway deployed that way binds as a root DN, bypasses ACIs entirely, and the amplifier becomes
pre-authentication — worth one sentence in the issue
Split out of the review of #835.
#835 capped the number of
batchRequestelements per SOAP body (each element costs its own connection and bind), but the number of operations inside onebatchRequeststayed unbounded: within the 10 MiB request-size cap a single body holds on the order of 90k<compareRequest>elements assertinguserPassword, and a compare against an attribute stored under a salted password scheme (PBKDF2, bcrypt, …) costs the same deliberately slow password verification as a bind — one authenticated POST bought an unbounded amount of CPU.A new
web.xmlcontext-paramldap.dsml.batchrequest.operations.maxnow caps the operation count perbatchRequest. The default is generous (10000) because large batches are a designed use of DSMLv2 (bulk provisioning); raise it explicitly for bigger bulk loads. A batch over the cap is rejected as a whole with a singlenotAttemptederrorResponse, before the gateway even opens the LDAP connection: the count is known up front once the batch is unmarshalled,notAttemptedis then literally true, and a provisioning batch applied halfway is worse than one not attempted. The configured value is not echoed to the client, and a non-positive value is rejected at servlet init, matching the #835 caps.Tests: a batch over the cap is refused without a single connection to the directory server and keeps the
requestIDfor correlation; a batch of exactly the cap is fully processed; the init-time validation covers the new parameter.Release note
The DSML gateway now accepts at most 10000 operations per
batchRequest. Bulk-provisioning clients that send larger batches must raiseldap.dsml.batchrequest.operations.maxinweb.xml, or split their batches. Batches over the cap are rejected as a whole with anotAttemptederrorResponse. The default lives in the servlet, so an existing deployment inherits it on upgrade without touching itsweb.xml.Stacked on #835: the first two commits are that PR, only
aa471dba74is new here. Merge #835 first.Fixes #843