Qpid
  1. Qpid
  2. QPID-3396

Specifying username/password in JMS clients should not be mandatory

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.18
    • Fix Version/s: None
    • Component/s: Java Client
    • Labels:
      None

      Description

      Description of problem:
      Section 3.2.2 of the Programming in Apache Qpid guide says that the
      username/password in the JNDI connection URL is optional:

      amqp://[<user>:<pass>@][<clientid>]<virtualhost>[?<option>='<value>'[&<option>='<value>']]

      However skipping the [<user>:<pass>@] part in an URL leads to exception raised.

      How reproducible:
      100%

      Steps to Reproduce:
      1. Set auth=no in /etc/qpidd.conf
      2. Run connectionURLWithoutUserInfo in attached JUnit test

      Actual results:
      Exception raised:

      User information not found on url between indicies 7 and 1
      amqp://clientid/test?brokerlist='tcp://localhost:5672' ^ at
      org.apache.qpid.url.URLHelper.parseError(URLHelper.java:143) at
      org.apache.qpid.url.URLHelper.parseError(URLHelper.java:138) at
      org.apache.qpid.client.url.URLParser.parseURL(URLParser.java:111) at
      org.apache.qpid.client.url.URLParser.<init>(URLParser.java:42) at
      org.apache.qpid.client.AMQConnectionURL.<init>(AMQConnectionURL.java:63) at
      com.gs.mrg.eval.PLAIN_AuthenticationExample.connectionURLWithoutUserInfo(PLAIN_AuthenticationExample.java:109)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at
      sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
      at
      sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      at java.lang.reflect.Method.invoke(Method.java:597) at
      org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
      at
      org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
      at
      org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
      at
      org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
      at
      org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
      at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:274) at
      org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
      at
      org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:48)
      at org.junit.runners.ParentRunner$3.run(ParentRunner.java:242) at
      org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:58) at
      org.junit.runners.ParentRunner.runChildren(ParentRunner.java:240) at
      org.junit.runners.ParentRunner.access$000(ParentRunner.java:48) at
      org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:233) at
      org.junit.runners.ParentRunner.run(ParentRunner.java:303) at
      org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:49)
      at
      org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
      at
      org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
      at
      org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
      at
      org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
      at
      org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

      Expected results:
      No exception raised, the broker should authenticate the connection request.

        Activity

        Pavel Moravec created issue -
        Pavel Moravec made changes -
        Field Original Value New Value
        Attachment CredentialsMandatory.java [ 12489465 ]
        Robbie Gemmell made changes -
        Fix Version/s Future [ 12315490 ]
        Fix Version/s 0.12 [ 12316848 ]
        Pavel Moravec made changes -
        Affects Version/s 0.18 [ 12322451 ]
        Affects Version/s 0.10 [ 12316273 ]
        Fix Version/s Future [ 12315490 ]
        Hide
        Pavel Moravec added a comment -

        Simple patch proposal.

        When parsing connectionURL detects no credentials, don't raise exception but set username and password to some dummy-like values.

        As C++ broker deals with anonymous users as "anonymous@QPID" (where QPID is realm), username was set to "anonymous".

        Show
        Pavel Moravec added a comment - Simple patch proposal. When parsing connectionURL detects no credentials, don't raise exception but set username and password to some dummy-like values. As C++ broker deals with anonymous users as "anonymous@QPID" (where QPID is realm), username was set to "anonymous".
        Pavel Moravec made changes -
        Attachment 0001-connectionURL-credentials-optional.patch [ 12540657 ]
        Hide
        Rajith Attapattu added a comment -

        What if the SASL mech is PLAIN?
        In that case if the user name and password is empty we should probably throw an exception rather than silently setting it to "anonymous".

        Alternatively, we could set it to "anonymous", but check them at the SASL layer and throw an appropriate exception that would point to the fact that the URL is missing the credentials.

        We should make sure we provide the end-user a proper notification as to what the real issue is.

        Show
        Rajith Attapattu added a comment - What if the SASL mech is PLAIN? In that case if the user name and password is empty we should probably throw an exception rather than silently setting it to "anonymous". Alternatively, we could set it to "anonymous", but check them at the SASL layer and throw an appropriate exception that would point to the fact that the URL is missing the credentials. We should make sure we provide the end-user a proper notification as to what the real issue is.
        Pavel Moravec made changes -
        Attachment 0001-connectionURL-credentials-optional.patch [ 12540657 ]
        Hide
        Pavel Moravec added a comment -

        New version of patch. It again sets username to "anonymous" and password to "" (only when the credentials are missing), but further:

        • if sasl_mechs is present and not ANONYMOUS, it raises an exception
        • if sasl_mechs is not present, it logs warning that sasl_mechs is being set to ANONYMOUS (as we assume that no credentials means ANONYMOUS mechanism so we have to restrict the client to it)
        Show
        Pavel Moravec added a comment - New version of patch. It again sets username to "anonymous" and password to "" (only when the credentials are missing), but further: if sasl_mechs is present and not ANONYMOUS, it raises an exception if sasl_mechs is not present, it logs warning that sasl_mechs is being set to ANONYMOUS (as we assume that no credentials means ANONYMOUS mechanism so we have to restrict the client to it)
        Pavel Moravec made changes -
        Attachment 0001-connectionURL-credentials-optional.patch [ 12540669 ]
        Hide
        Rajith Attapattu added a comment -

        "if sasl_mechs is not present, it logs warning that sasl_mechs is being set to ANONYMOUS (as we assume that no credentials means ANONYMOUS mechanism so we have to restrict the client to it)"

        This is not correct. Simply bcos user/pass is missing, we can't force the client to use "ANONYMOUS".
        For example for GSSAPI and EXTERNAL you don't need a user/pass. Infact the customer who logged the original issue was using EXTERNAL and was wondering why he needs to specify user/pass.

        Again I think the correct behaviour is to look at the selected mech (after negotiation with the peer) and then see if user/pass is required. If so then throw an exception.

        Show
        Rajith Attapattu added a comment - "if sasl_mechs is not present, it logs warning that sasl_mechs is being set to ANONYMOUS (as we assume that no credentials means ANONYMOUS mechanism so we have to restrict the client to it)" This is not correct. Simply bcos user/pass is missing, we can't force the client to use "ANONYMOUS". For example for GSSAPI and EXTERNAL you don't need a user/pass. Infact the customer who logged the original issue was using EXTERNAL and was wondering why he needs to specify user/pass. Again I think the correct behaviour is to look at the selected mech (after negotiation with the peer) and then see if user/pass is required. If so then throw an exception.
        Hide
        Pavel Moravec added a comment -

        > Again I think the correct behaviour is to look at the selected mech (after negotiation with the peer) and then see if user/pass is required. If so then throw an exception.

        With the current C++ broker and auth=no, the broker sends in connection.start AMQP command auth.mechanisms "ANONYMOUS" and also "PLAIN" - the second is due to allowing some tests (per gsim). If a client chooses PLAIN mechanism here and sends whatever credentials, the broker silently ignores them.

        But that would cause a problem in our case as:

        • client library got no crednetials from the client
        • broker offers PLAIN and ANONYMOUS mechs, not telling anything about no auth required
        • library chooses PLAIN as more secure and fails due to no credentials

        Therefore I suggest the patch that checks during URL parsing if credentials are missing and sasl_mechs is not specified - only in that case, it restricts the mechanisms to ANONYMOUS GSSAPI EXTERNAL as these dont require credetials in this way.

        I tested the patch works fine in all combinations "auth=yes/no" x "credentials provided / not provided" x "/etc/sasl2/qpidd.conf restricted mech_list to ..". I have not verify GSSAPI mechanism only.

        Show
        Pavel Moravec added a comment - > Again I think the correct behaviour is to look at the selected mech (after negotiation with the peer) and then see if user/pass is required. If so then throw an exception. With the current C++ broker and auth=no, the broker sends in connection.start AMQP command auth.mechanisms "ANONYMOUS" and also "PLAIN" - the second is due to allowing some tests (per gsim). If a client chooses PLAIN mechanism here and sends whatever credentials, the broker silently ignores them. But that would cause a problem in our case as: client library got no crednetials from the client broker offers PLAIN and ANONYMOUS mechs, not telling anything about no auth required library chooses PLAIN as more secure and fails due to no credentials Therefore I suggest the patch that checks during URL parsing if credentials are missing and sasl_mechs is not specified - only in that case, it restricts the mechanisms to ANONYMOUS GSSAPI EXTERNAL as these dont require credetials in this way. I tested the patch works fine in all combinations "auth=yes/no" x "credentials provided / not provided" x "/etc/sasl2/qpidd.conf restricted mech_list to ..". I have not verify GSSAPI mechanism only.
        Pavel Moravec made changes -
        Attachment 0001-connectionURL-credentials-optional.patch [ 12540669 ]
        Hide
        Pavel Moravec added a comment -

        Patch proposal, see comment Starting with "> Again I think .." for details.

        Show
        Pavel Moravec added a comment - Patch proposal, see comment Starting with "> Again I think .." for details.
        Pavel Moravec made changes -
        Justin Ross made changes -
        Assignee Rajith Attapattu [ rajith ]
        Hide
        Fraser Adams added a comment -

        Comment following a conversation with Pavel on the Qpid Users Mailing List:

        The Jira comments at one point mention:

        "New version of patch. It again sets username to "anonymous" and
        password to "" (only when the credentials are missing)"

        I actually tried explicitly doing:

        amqp://anonymous:""@clientid/test?brokerlist='tcp://localhost:5672'

        in one of my tests, but that resulted in:

        connectionfactory.ConnectionFactory=amqp://anonymous:""@clientid/test?brokerlist='tcp://localhost:5672',
        destination.publishedAddress=amq.match}
        main 2013-01-25 11:48:53,809 WARN
        [apache.qpid.jndi.PropertiesFileInitialContextFactory] Unable to
        createFactories:Illegal character in authority between indicies 7 and
        1
        amqp://anonymous:""@clientid/test?brokerlist='tcp://localhost:5672'
        ^
        createJMSSession() connectionFactory lookup failed, retrying

        same with anonymous:anonymous etc.

        So I'm guessing that what you are talking about is "under the hood"
        and bypasses that particular test??

        I personally think that this bug notice should be extended to cover implicit and explicit settings of anonymous - what d'you reckon?

        Show
        Fraser Adams added a comment - Comment following a conversation with Pavel on the Qpid Users Mailing List: The Jira comments at one point mention: "New version of patch. It again sets username to "anonymous" and password to "" (only when the credentials are missing)" I actually tried explicitly doing: amqp://anonymous:""@clientid/test?brokerlist='tcp://localhost:5672' in one of my tests, but that resulted in: connectionfactory.ConnectionFactory=amqp://anonymous:""@clientid/test?brokerlist='tcp://localhost:5672', destination.publishedAddress=amq.match} main 2013-01-25 11:48:53,809 WARN [apache.qpid.jndi.PropertiesFileInitialContextFactory] Unable to createFactories:Illegal character in authority between indicies 7 and 1 amqp://anonymous:""@clientid/test?brokerlist='tcp://localhost:5672' ^ createJMSSession() connectionFactory lookup failed, retrying same with anonymous:anonymous etc. So I'm guessing that what you are talking about is "under the hood" and bypasses that particular test?? I personally think that this bug notice should be extended to cover implicit and explicit settings of anonymous - what d'you reckon?
        Hide
        Alex Rudyy added a comment -

        Hi Rajith,

        Your newly added test fails on 0-8/0-9/0-9-1 path as corresponding changes are not implemented there. Here is jenkins URL with the test failure: https://builds.apache.org/view/M-R/view/Qpid/job/Qpid-Java-Java-BDB-TestMatrix/jdk=JDK%201.6%20%28latest%29,label=Ubuntu,profile=java-bdb.0-9-1/786/

        Are you going to fix it?

        Also, I would like to point out that the SASL mechanism at connection URL in new test is specified incorrectly. The correct option name is "sasl_mechs" (not "sasl_mech").

        Show
        Alex Rudyy added a comment - Hi Rajith, Your newly added test fails on 0-8/0-9/0-9-1 path as corresponding changes are not implemented there. Here is jenkins URL with the test failure: https://builds.apache.org/view/M-R/view/Qpid/job/Qpid-Java-Java-BDB-TestMatrix/jdk=JDK%201.6%20%28latest%29,label=Ubuntu,profile=java-bdb.0-9-1/786/ Are you going to fix it? Also, I would like to point out that the SASL mechanism at connection URL in new test is specified incorrectly. The correct option name is "sasl_mechs" (not "sasl_mech").
        Hide
        Rajith Attapattu added a comment -

        Let me look at implementing the same for the 0-8/0-9 path.
        I will fix the test as well.

        Show
        Rajith Attapattu added a comment - Let me look at implementing the same for the 0-8/0-9 path. I will fix the test as well.
        Hide
        Rajith Attapattu added a comment -

        Alex and Robbie, could you please have a look at the following patch for the 0-8/0-9 code path?

        --- a/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java
        +++ b/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java
        @@ -37,6 +37,7 @@ import org.apache.qpid.framing.ConnectionStartOkBody;
         import org.apache.qpid.framing.FieldTable;
         import org.apache.qpid.framing.FieldTableFactory;
         import org.apache.qpid.framing.ProtocolVersion;
        +import org.apache.qpid.jms.ConnectionURL;
         import org.apache.qpid.properties.ConnectionStartProperties;
         
         import javax.security.sasl.Sasl;
        @@ -108,6 +109,13 @@ public class ConnectionStartMethodHandler implements StateAwareMethodListener<Co
                             throw new AMQException(null, "No supported security mechanism found, passed: " + new String(body.getMechanisms()), null);
                         }
         
        +                ConnectionURL connectionURL = session.getAMQConnection().getConnectionURL();
        +                if ((connectionURL.getUsername() == null || connectionURL.getPassword() == null)
        +                        && CallbackHandlerRegistry.getInstance().isUserPassRequired(mechanism))
        +                {
        +                    throw new AMQException(null,"Username and Password is required for the selected mechanism : " + mechanism,null);
        +                }
        +
                         byte[] saslResponse;
                         try
                         {
        

        It appears the 0-8/0-9 code path does not allow the mech list to be constrained by the list provided in the connection URL, so we need to exclude the test from this code path as it will only work as expected bcos the broker is selecting CRAM-MD5 which requires user/pass.

        When I run the testExceptionWhenUserPassIsRequired with the above patch, I see the correct exception being raised (due to the broker selecting CRAM-MD5) but the test fails as the connection close seems to time out. So I'm not sure about the viability of this fix being done on the 0-8/0-9 code path.

        org.apache.qpid.AMQTimeoutException: Server did not respond in a timely fashion [error code 408: Request Timeout]
        at org.apache.qpid.client.util.BlockingWaiter.block(BlockingWaiter.java:177)
        at org.apache.qpid.client.state.StateWaiter.await(StateWaiter.java:114)
        at org.apache.qpid.client.state.StateWaiter.await(StateWaiter.java:91)
        at org.apache.qpid.client.AMQConnectionDelegate_8_0.makeBrokerConnection(AMQConnectionDelegate_8_0.java:135)
        at org.apache.qpid.client.AMQConnection.makeBrokerConnection(AMQConnection.java:619)
        at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:398)
        at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:222)
        at org.apache.qpid.test.unit.client.connection.ConnectionTest.testExceptionWhenUserPassIsRequired(ConnectionTest.java:362)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at junit.framework.TestCase.runTest(TestCase.java:154)

        Show
        Rajith Attapattu added a comment - Alex and Robbie, could you please have a look at the following patch for the 0-8/0-9 code path? --- a/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java +++ b/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java @@ -37,6 +37,7 @@ import org.apache.qpid.framing.ConnectionStartOkBody; import org.apache.qpid.framing.FieldTable; import org.apache.qpid.framing.FieldTableFactory; import org.apache.qpid.framing.ProtocolVersion; + import org.apache.qpid.jms.ConnectionURL; import org.apache.qpid.properties.ConnectionStartProperties; import javax.security.sasl.Sasl; @@ -108,6 +109,13 @@ public class ConnectionStartMethodHandler implements StateAwareMethodListener<Co throw new AMQException( null , "No supported security mechanism found, passed: " + new String (body.getMechanisms()), null ); } + ConnectionURL connectionURL = session.getAMQConnection().getConnectionURL(); + if ((connectionURL.getUsername() == null || connectionURL.getPassword() == null ) + && CallbackHandlerRegistry.getInstance().isUserPassRequired(mechanism)) + { + throw new AMQException( null , "Username and Password is required for the selected mechanism : " + mechanism, null ); + } + byte [] saslResponse; try { It appears the 0-8/0-9 code path does not allow the mech list to be constrained by the list provided in the connection URL, so we need to exclude the test from this code path as it will only work as expected bcos the broker is selecting CRAM-MD5 which requires user/pass. When I run the testExceptionWhenUserPassIsRequired with the above patch, I see the correct exception being raised (due to the broker selecting CRAM-MD5) but the test fails as the connection close seems to time out. So I'm not sure about the viability of this fix being done on the 0-8/0-9 code path. org.apache.qpid.AMQTimeoutException: Server did not respond in a timely fashion [error code 408: Request Timeout] at org.apache.qpid.client.util.BlockingWaiter.block(BlockingWaiter.java:177) at org.apache.qpid.client.state.StateWaiter.await(StateWaiter.java:114) at org.apache.qpid.client.state.StateWaiter.await(StateWaiter.java:91) at org.apache.qpid.client.AMQConnectionDelegate_8_0.makeBrokerConnection(AMQConnectionDelegate_8_0.java:135) at org.apache.qpid.client.AMQConnection.makeBrokerConnection(AMQConnection.java:619) at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:398) at org.apache.qpid.client.AMQConnection.<init>(AMQConnection.java:222) at org.apache.qpid.test.unit.client.connection.ConnectionTest.testExceptionWhenUserPassIsRequired(ConnectionTest.java:362) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:154)

          People

          • Assignee:
            Rajith Attapattu
            Reporter:
            Pavel Moravec
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development