[#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type - #811
[#809] Fix DSML gateway NPE on abandonRequest and on missing Content-Type#811vharseko wants to merge 4 commits into
Conversation
maximthomas
left a comment
There was a problem hiding this comment.
Both defects are real and the fixes are correct. I reproduced everything locally: with DSMLServlet.java reverted to master a single <abandonRequest/> throws NullPointerException out of doPost and the fake LDAP endpoint sees bind, abandon with no unbind — the leak, exactly as described. With the patch it sees bind, abandon, unbind.
Worth adding to the description: the socket regression only affects callers that reach createSocket(), i.e. plain LDAP or StartTLS. StopDS and ManageAccount install an SSLConnectionFactory unconditionally and CryptoManagerImpl sets setUseSSL(true), so those take createSSLSocket() and are unaffected. Actually affected: the DSML gateway (default web.xml has ldap.usessl=false) and LDAPConnectionArgumentParser tools (import-ldif, export-ldif, backup, restore, rebuild-index, manage-tasks) without --useSSL. Also note BindException is not a ConnectException, so the broken code escaped the per-address catch into the outer catch (Exception ex) — the fix restores failover across multiple A records.
Four things to address before merge.
The new log call is silently dropped (medium)
opendj-dsml-servlet/src/main/java/org/opends/dsml/protocol/DSMLServlet.java
} catch (Exception e) {
// the client gets an empty response: at least make the cause visible
Logger.getLogger(PKG_NAME).log(Level.SEVERE, "Unable to send the DSML response", e);
}LDAPConnection.connectToHost() calls JDKLogging.disableLogging() on every non-verbose connection, which does LogManager.getLogManager().reset(); Logger.getLogger("").setLevel(Level.OFF). After the first LDAP connection attempt in the JVM this record has no handlers and an OFF root level and is never emitted — I confirmed isLoggable(SEVERE)=false, rootLevel=OFF, rootHandlers=0. printStackTrace() at least reached catalina.out, so this is a net loss.
SLF4J is not an alternative here: the war ships slf4j-api plus the i18n bridge but no SLF4JServiceProvider, so it would be a NOP. The working option is the container log — but init(ServletConfig) never calls super.init(config), so getServletContext() NPEs today:
public void init(ServletConfig config) throws ServletException {
super.init(config); // currently missing
...
}
...
} catch (Exception e) {
getServletContext().log("Unable to send the DSML response", e);
}authzid accumulates across batch requests (medium)
connection = null in the finally is right, and it also fixes a second latent bug: previously a 2nd <batchRequest> in the same SOAP body was silently skipped (connection != null left connected == false). I verified this — two empty batch requests open one connection on master and two on this branch.
That makes the following reachable, because connOptions is built once per doPost and addSASLProperty appends to a per-key list:
connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());With ldap.authzidtypeisid=true and two batch requests each carrying an <authRequest>, the second bind fails client-side. Reproduced:
<ns0:errorResponse type="couldNotConnect">
<ns0:message>org.opends.server.tools.LDAPConnectionException:
The "authzid" SASL property only accepts a single value</ns0:message>
</ns0:errorResponse>Build LDAPConnectionOptions per batch request, or clear authzid before setting it.
The two commits are not independently mergeable (medium)
testAbandonRequestIsProcessedAndConnectionIsClosed cannot pass without a8616b08a8 — against a ~/.m2 copy of opendj-server-legacy built from master it fails with couldNotConnect: Address already in use (Bind failed). So the socket fix cannot be split off into its own issue, and mvn -pl opendj-dsml-servlet test only reports 64 passing tests when opendj-server-legacy has already been rebuilt from this branch. CI's full-reactor mvn verify is fine; the command in the description is misleading.
The ordering also matters the other way round: with the default ldap.usessl=false the gateway has been unable to connect at all since #279 (every release 4.5.5 through 5.1.2), which is what masked the abandon NPE. Fixing the socket without fixing the NPE would re-enable the gateway straight into the crash — so both belong in this PR.
malformedRequest response drops the requestID (low)
createXMLParsingErrorResponse() deliberately re-parses the buffered stream with SAX to recover the request ID and calls batchResponse.setRequestID(...). The new block builds a bare ErrorResponse, so clients cannot correlate the reply:
ErrorResponse errorResponse = objFactory.createErrorResponse();
errorResponse.setType(MALFORMED_REQUEST);
errorResponse.setMessage("Content-Type does not match SOAP 1.1 or SOAP 1.2");
batchResponses.add(objFactory.createBatchResponseErrorResponse(errorResponse));Nothing has been read from is at that point and the mark is still valid, so createXMLParsingErrorResponse(is, objFactory, batchResponse, "Content-Type does not match SOAP 1.1 or SOAP 1.2") would keep the two malformed paths consistent.
Nits
- Dead null check: with the
finallynulling it,connectionis alwaysnullatif ( connection == null ), and the method-scope declaration is no longer needed — a loop-localLDAPConnection connection = new LDAPConnection(...)inside thetryreads better. Given #790/#793 this will probably be flagged by CodeQL. - Mockito is already available:
org.mockito:mockito-all:1.10.19:testandassertj-corecome from the root pom's top-level<dependencies>, so they are already onopendj-dsml-servlet's test classpath — the hand-rolledProxystubs are a fair choice, but "no new test dependency" isn't a constraint. Worth a word in the description to pre-empt the question. - Silent failures in the fake server:
FakeLdapServer.serve()ends withe.printStackTrace(), so a server-side error prints and the test still passes. Record the exception and assert it isnullinawaitDisconnect(). defaultValue(Method)primitive gap: returnsnullforchar/byte/short/float/doublereturns, which would NPE out of the proxy. No call site hits it today;returnType.isPrimitive()catch-all removes the trap.- Reply SOAP version: a malformed
Authorizationheaderbreaks out of the header loop, soContent-Typemay never be examined and a SOAP 1.2 request gets a SOAP 1.1 reply.continueinstead ofbreakavoids it. - HTTP status: an unsupported
Content-Typenow yields200+malformedRequestinstead of the previousServletException→ 500. Better, and consistent with how XML parse errors are already reported, but415alongside the body would be more correct HTTP — and deployments alerting on 5xx will see this class of request vanish. setReuseAddress(true): harmless, and it now applies to the implicit local bind thatconnect()performs — the flag survives the connect. Pre-#279 (new Socket(addr, port)) did not set it; either way not a defect.connect()still has no timeout, soconnectToHost'stimeoutparameter never reaches the socket connect — pre-existing, but a candidate follow-up.- Shared
BatchResponse: all batch requests in one SOAP body share a single<batchResponse>whoserequestIDis overwritten each iteration, so two batches produce one<batchResponse requestID="2">holding both batches' elements. Pre-existing, but only observable now that the second batch actually runs. - Coverage gaps: no test for the multi-
batchRequestreconnect, for the SOAP 1.2 path, or for an abandon mid-batch letting later operations run; andcreateSocket()has no test in the module that owns it — a few lines binding aServerSocketand assertingconnectToHostsucceeds would pin the #279 regression inopendj-server-legacy. - Why this went unnoticed: nothing in the repo referenced
DSMLServletfrom a test before this PR, and although the module wires Cargo/Tomcat intopre-integration-testit has no IT classes — the container starts, deploys and stops without a single request.DSMLServletTestCaseis this servlet's first coverage; nice to have it.
|
Thanks — every point checked out, and the two medium ones were reproduced before fixing. Pushed as f13ffb0; the description has been rewritten accordingly. The new log call is silently droppedConfirmed: Took the container log as suggested, with the missing super.init(config);
...
getServletContext().log("Unable to send the DSML response", e);
|
maximthomas
left a comment
There was a problem hiding this comment.
All four points from the previous round are addressed, and I re-checked each against the code rather than the description:
- container log —
super.init(config)is in place, sogetServletContext()is valid; thecatchnow writes there; authzid—getSASLProperties()does hand out the live map (return saslProperties;) andaddSASLProperty()stores the key verbatim, soremove("authzid")is the right handle — but see below;- commit split — description now states the dependency and carries the caveat on the
mvn -plcommand; requestID—createXMLParsingErrorResponse()reuses the buffered stream;isis untouched at that point, and the new assertion pins it.
Both suites are green here: DSMLServletTestCase 10/10 (the eight of this PR plus two probes of my own) and LDAPConnectionTestCase 2/2 against a real server under -Pprecommit. One defect remains in the authzid fix.
authzid still survives into a batch request that has no authRequest (medium)
opendj-dsml-servlet/src/main/java/org/opends/dsml/protocol/DSMLServlet.java
The remove sits inside the authRequest != null branch, so it only runs when the next batch request also asks for an authorization identity:
if (batchRequest.authRequest != null) {
if (authenticationIsID) {
connOptions.getSASLProperties().remove("authzid");
connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());A body whose first batch request carries an authRequest and whose second does not leaves the first authzid in the shared options. Recording the SASL credentials at the fake endpoint:
body: batch "1" <authRequest principal="dn:cn=first"/>, batch "2" no authRequest
bind 1: mech=PLAIN creds=dn:cn=first|u:user|password
bind 2: mech=PLAIN creds=dn:cn=first|u:user|password <-- batch 2 runs as cn=first
The control case is correct (dn:cn=first then dn:cn=second), which is why testAuthzIdIsNotAccumulatedAcrossBatchRequests passes: both of its batch requests carry an authRequest. The operations of the second batch execute under an identity the request never asked for — gated on ldap.authzidtypeisid=true (the web.xml default is false) and still subject to proxied-auth privileges on the server, hence medium rather than high.
Clearing unconditionally at the top of each iteration covers both cases:
// the options are shared by all the batch requests of this SOAP body
connOptions.getSASLProperties().remove("authzid");
if (batchRequest.authRequest != null) {
if (authenticationIsID) {
connOptions.addSASLProperty("authzid=" + batchRequest.authRequest.getPrincipal());A mixed body would make a good third case in that test.
Nits
- Two logging sinks in one class: four
Logger.getLogger(PKG_NAME)calls remain (safeSetFeature×2,createSafeDocument×2) and are dead for exactly the reason now documented in the commit message. All four are instance methods, sogetServletContext().log(...)is available to them too. - One bind per batch request: two batch requests now cost two binds (confirmed), where the extras used to be skipped. Password verification is deliberately expensive, so a body holding many batch requests is a cheap amplifier for a client with valid credentials — a cap on the number of batch requests per body would bound it.
- Shared
batchResponse: the new multi-batch test makes it observable — its own reply is<batchResponse requestID="2">holding both batches'authResponseelements. Agreed it is pre-existing and belongs in its own issue; just noting the tests now document the odd shape. LDAPConnectionTestCaseis Linux-only in CI:.github/workflows/build.ymlsets-P precommitonly whenrunner.os == 'Linux', andopendj-server-legacydisables surefire, so the test runs on the five Ubuntu matrix entries and nowhere else. That is the platform where the bind is refused, so #279 is pinned where it matters; the macOS variant you describe (Socket is not connected) is not covered.findFreePort()for the closed-port case is inherently racy, butrerunFailingTestsCount=3absorbs a stray bind. Fine as is.- Unrelated:
GrizzlyLDAPConnectionFactoryTestCase.testClientSideConnectTimeoutfails in my environment on this branch. It is inopendj-grizzly, untouched here, and depends on an unroutable address — mentioning it only so it is not mistaken for a regression of this PR.
createSocket() has been binding the new client socket to the target server address instead of connecting to it since OpenIdentityPlatform#279, so every plain or StartTLS connection made through org.opends.server.tools.LDAPConnection fails with "Address already in use" (server on the same host) or "Cannot assign requested address" (remote server). Affects the DSML gateway, stop-ds, manage-account and the other tools built on LDAPConnectionArgumentParser.
… on missing Content-Type performLDAPRequest() returns null for an abandon request, but doPost() dereferenced the result unconditionally, so a batch containing <abandonRequest/> ended in a NullPointerException; as the connection was closed after the loop instead of in a finally, one LDAP connection was leaked per request. messageFactory was only assigned when a SOAP 1.1 or SOAP 1.2 Content-Type header was present, and was then dereferenced both when parsing the request and when sending the response: a POST without Content-Type ended in a NullPointerException, and, when an error response had already been queued, in an empty HTTP 200 instead of that error. A missing or unsupported Content-Type is now answered with a malformedRequest batch response. Also log the failure instead of printing the stack trace when the response cannot be sent, and add regression tests for both defects.
Report the failure to send the response to the container log: the java.util.logging record was dropped, as connectToHost() resets the LogManager and turns the root logger off on every non-verbose connection. This needs super.init(config), without which getServletContext() throws. Drop the authzid of the previous batch request before setting the new one: the connection options are shared by the whole SOAP body and addSASLProperty() appends to the values of a key, so a second authRequest made SASL PLAIN reject a multi-valued authzid. Now that the connection is never reused, make it a loop local and remove the dead null check that guarded the reuse. Build the malformed Content-Type response with createXMLParsingErrorResponse(), like the other two malformed paths, so that the requestID is recovered; and keep reading the headers after a malformed Authorization one, so that the reply keeps the SOAP version of the request. Cover the SOAP 1.2 path, the per-batch-request connection and the authzid, let the fake LDAP endpoint serve several connections and fail the test on a server-side error, and pin the createSocket() regression of OpenIdentityPlatform#279 with a test in the module that owns it.
…uest of a SOAP body The connection options are built once per doPost() and shared by all the batch requests of the SOAP body, but the authzid was dropped only when the next batch request carried an authRequest of its own. A body whose first batch request asks for an authorization identity and whose second does not left the first authzid in the options, so the operations of the second one ran under an identity the request never asked for. It is gated on ldap.authzidtypeisid=true, which the shipped web.xml leaves at false, and still subject to the proxied-auth privileges of the server. The clearing now happens at the top of every iteration, before the authRequest is looked at. DSMLServletTestCase records the authorization identity of every SASL bind at the fake endpoint: the existing test now asserts the identities themselves instead of the mere absence of an error, and a new one pins the mixed body, where the second batch request must bind with no authzid at all. The four remaining Logger.getLogger(PKG_NAME) calls are replaced by getServletContext().log(), so the class has a single logging sink: they were dead for the reason already documented for the response path, which moves to the class javadoc.
f13ffb0 to
9ddd7da
Compare
|
The remaining defect is fixed and the branch has been rebased onto master, so this is a force push:
|
|
The two follow-ups are filed and linked from the description:
|
Fixes #809
1.
abandonRequestcaused an NPE and leaked the LDAP connectionperformLDAPRequest()deliberately returnsnullfor an abandon request (no response element is defined for it in DSMLv2), butdoPost()guarded thenullonly when adding to the response list and then dereferenced it unconditionally, so a batch containing<abandonRequest abandonID="1"/>ended in aNullPointerExceptionescaping to the container as a 500.Since the connection block was not wrapped in
try/finally,connection.close(nextMessageID)was skipped as well:org.opends.server.tools.LDAPConnectioncloses its socket only inclose(), so one connection to the directory server was leaked per request — trivially repeatable until the connection handler is exhausted.The loop now skips the
nullresult, and the connection is closed in afinally. It is no longer reused either, which fixes a second latent defect: a secondbatchRequestin the same SOAP body used to be silently skipped, because the leftover connection leftconnected == false. Now that it really runs, two more things had to follow:LDAPConnectionOptionsare built once perdoPostand shared by everybatchRequestof the SOAP body, so withldap.authzidtypeisid=truetheauthzidof one of them used to reach the bind of the next. It fails in two ways:addSASLProperty()appends to the values of a key, so two<authRequest>in a row produced a multi-valued property, which SASL PLAIN rejects client-side (The "authzid" SASL property only accepts a single value); and abatchRequestcarrying no<authRequest>inherited the authorization identity of the previous one, running its operations under an identity the request never asked for. Theauthzidis now dropped at the top of every iteration, before theauthRequestis looked at;connection == nullcheck that guarded the reuse is gone.2. NPE when the request has no
Content-TypeheadermessageFactorywas assigned only inside the header loop, when aContent-Typeheader matching SOAP 1.1 or SOAP 1.2 was present. A POST without that header is legal HTTP and left the field atnull, which was then dereferenced when parsing the request (NPE → 500) and again on the response path, where it was swallowed bycatch (Exception e) { e.printStackTrace(); }— a client whose credentials were rejected got an empty HTTP 200 instead of the error.A missing or unsupported
Content-Typeis now answered with amalformedRequestbatch response (SOAP 1.1 is used for the reply), which also replaces theServletExceptionpreviously thrown for a non-SOAP content type. It is built bycreateXMLParsingErrorResponse(), like the other two malformed paths, so therequestIDis recovered from the buffered stream and the client can correlate the reply.A malformed
Authorizationheader no longerbreaks out of the header loop either: theContent-Typemay still be ahead, and it decides which SOAP version the error is reported with.Failures to send the response are reported to the container log.
java.util.loggingis not usable here:LDAPConnection.connectToHost()callsJDKLogging.disableLogging()on every non-verbose connection, which doesLogManager.reset()and sets the root level toOFF, so after the first LDAP connection in the JVM the record would be dropped; and the war shipsslf4j-apiwithout anySLF4JServiceProvider.init(ServletConfig)now callssuper.init(config), without whichgetServletContext()would throw an NPE.The four
Logger.getLogger(PKG_NAME)calls ofsafeSetFeature()andcreateSafeDocument()were dead for exactly that reason, so they go to the container log as well and the servlet has a single logging sink; the rationale sits in the class javadoc.Both defects predate the Open Identity Platform fork (imported in 1577c0a).
3. Prerequisite:
LDAPConnectionnever connected its socketFound while writing the regression test: since #279,
LDAPConnection.createSocket()binds the newly created client socket to the target server address instead of connecting to it, so every plain or StartTLS connection made throughorg.opends.server.tools.LDAPConnectionfails — withAddress already in useorCannot assign requested addresswhere the bind is refused, and withSocket is not connectedwheresetReuseAddress(true)lets the bind through and the unconnected socket is handed toLDAPWriter. NeitherBindExceptionnorSocketExceptionis aConnectException, so the failure also escaped the per-addresscatchinto the outercatch (Exception ex), losing the result code (-1) on the way; the fix restores both the result code and the failover across the addresses of a host.Only the callers that reach
createSocket()are affected, i.e. plain LDAP or StartTLS without anSSLConnectionFactory: the DSML gateway (the defaultweb.xmlhasldap.usessl=false) and theLDAPConnectionArgumentParsertools (import-ldif,export-ldif,backup,restore,rebuild-index,manage-tasks) used without--useSSL.stop-dsandmanage-accountinstall anSSLConnectionFactoryunconditionally andCryptoManagerImplsetssetUseSSL(true), so all three go throughcreateSSLSocket()and are unaffected;ldapsearch/ldapmodifygo through the SDK, which is why this went unnoticed.It is kept as a separate commit for review, but it cannot be tracked as its own pull request: the DSML tests below need it, and with the default
ldap.usessl=falsethe gateway has been unable to connect at all since #279 (every release from 4.5.5 to 5.1.2), which is exactly what masked the abandon NPE. Fixing the socket alone would re-enable the gateway straight into the crash.Tests
DSMLServletTestCasedrivesdoPost()against a fake LDAP endpoint (a real socket that answers the binds and records both the message types it receives and the authorization identity of every SASL bind), with the servlet API stubbed throughjava.lang.reflect.Proxy. Mockito and AssertJ are on the test classpath of every module through the root pom, the hand-written stubs are simply a closer fit for these interfaces:bind → abandon → unbind);batchRequestof a SOAP body gets its own connection (bind → abandon → unbind, twice);authzidof abatchRequestdoes not survive into the bind of the next one: two<authRequest>in a row bind asdn:cn=firstthendn:cn=second, and abatchRequestwithout an<authRequest>binds with noauthzidat all;Content-Typeis answered withmalformedRequestcarrying therequestID, and no connection is opened;Content-Typelikewise;Content-Typeis missing;Authorizationheader does not downgrade the reply to SOAP 1.1.LDAPConnectionTestCasepins #279 in the module that owns the code: a plain connection to the test server succeeds, and a closed port is reported asCLIENT_SIDE_CONNECT_ERRORinstead of escaping the per-addresscatchwithout a result code. Both fail whenconnect()is turned back intobind().mvn -pl opendj-dsml-servlet test→Tests run: 69, Failures: 0, Errors: 0, andmvn -pl opendj-server-legacy verify -Pprecommit -Dit.test=LDAPConnectionTestCase→Tests run: 2, Failures: 0, Errors: 0. Note that the DSML command needsopendj-server-legacyto have been installed from this branch first, since those tests exercisecreateSocket(); the full reactor build used by CI is unaffected.Follow-ups
Two defects of the gateway are deliberately left out, both raised in review and both only observable now that the second and later
batchRequestof a SOAP body really run:batchResponse, whoserequestIDis overwritten by each of them, so the client cannot tell which element answers which request;batchRequestturns a single POST into an amplifier, so the number of batch requests per body wants a cap.