qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From astitc...@apache.org
Subject qpid-proton git commit: PROTON-1135: Do not pipeline SASL frames into AMQP frames from client side - Test that we will accept pipelined frames though.
Date Wed, 06 Apr 2016 14:43:31 GMT
Repository: qpid-proton
Updated Branches:
  refs/heads/master c7dff22d4 -> 6738c16ff


PROTON-1135: Do not pipeline SASL frames into AMQP frames from client side
- Test that we will accept pipelined frames though.


Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/6738c16f
Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/6738c16f
Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/6738c16f

Branch: refs/heads/master
Commit: 6738c16ffcb93981f1bb936b21e86515b1787f54
Parents: c7dff22
Author: Andrew Stitcher <astitcher@apache.org>
Authored: Thu Mar 31 02:05:17 2016 -0400
Committer: Andrew Stitcher <astitcher@apache.org>
Committed: Wed Apr 6 00:45:07 2016 -0400

----------------------------------------------------------------------
 proton-c/src/sasl/sasl-internal.h   |   1 -
 proton-c/src/sasl/sasl.c            |  70 ++++-------------
 tests/python/proton_tests/engine.py |  32 +++++---
 tests/python/proton_tests/sasl.py   | 128 ++++++++++++-------------------
 4 files changed, 89 insertions(+), 142 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/6738c16f/proton-c/src/sasl/sasl-internal.h
----------------------------------------------------------------------
diff --git a/proton-c/src/sasl/sasl-internal.h b/proton-c/src/sasl/sasl-internal.h
index b3f4c7f..063cd0d 100644
--- a/proton-c/src/sasl/sasl-internal.h
+++ b/proton-c/src/sasl/sasl-internal.h
@@ -59,7 +59,6 @@ enum pni_sasl_state {
   SASL_POSTED_MECHANISMS,
   SASL_POSTED_RESPONSE,
   SASL_POSTED_CHALLENGE,
-  SASL_PRETEND_OUTCOME,
   SASL_RECVED_OUTCOME_SUCCEED,
   SASL_RECVED_OUTCOME_FAIL,
   SASL_POSTED_OUTCOME,

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/6738c16f/proton-c/src/sasl/sasl.c
----------------------------------------------------------------------
diff --git a/proton-c/src/sasl/sasl.c b/proton-c/src/sasl/sasl.c
index 29d377e..47dc76c 100644
--- a/proton-c/src/sasl/sasl.c
+++ b/proton-c/src/sasl/sasl.c
@@ -108,7 +108,6 @@ static bool pni_sasl_is_client_state(enum pni_sasl_state state)
   return state==SASL_NONE
       || state==SASL_POSTED_INIT
       || state==SASL_POSTED_RESPONSE
-      || state==SASL_PRETEND_OUTCOME
       || state==SASL_RECVED_OUTCOME_SUCCEED
       || state==SASL_RECVED_OUTCOME_FAIL
       || state==SASL_ERROR;
@@ -116,20 +115,17 @@ static bool pni_sasl_is_client_state(enum pni_sasl_state state)
 
 static bool pni_sasl_is_final_input_state(pni_sasl_t *sasl)
 {
-  enum pni_sasl_state last_state = sasl->last_state;
   enum pni_sasl_state desired_state = sasl->desired_state;
-  return last_state==SASL_RECVED_OUTCOME_SUCCEED
-      || last_state==SASL_RECVED_OUTCOME_FAIL
-      || last_state==SASL_ERROR
-      || desired_state==SASL_POSTED_OUTCOME
-      || ( desired_state==SASL_RECVED_OUTCOME_SUCCEED && last_state>=SASL_POSTED_INIT);
+  return desired_state==SASL_RECVED_OUTCOME_SUCCEED
+      || desired_state==SASL_RECVED_OUTCOME_FAIL
+      || desired_state==SASL_ERROR
+      || desired_state==SASL_POSTED_OUTCOME;
 }
 
 static bool pni_sasl_is_final_output_state(pni_sasl_t *sasl)
 {
   enum pni_sasl_state last_state = sasl->last_state;
-  return last_state==SASL_PRETEND_OUTCOME
-      || last_state==SASL_RECVED_OUTCOME_SUCCEED
+  return last_state==SASL_RECVED_OUTCOME_SUCCEED
       || last_state==SASL_RECVED_OUTCOME_FAIL
       || last_state==SASL_ERROR
       || last_state==SASL_POSTED_OUTCOME;
@@ -164,11 +160,6 @@ void pni_sasl_set_desired_state(pn_transport_t *transport, enum pni_sasl_state
d
     if (sasl->last_state==desired_state && desired_state==SASL_POSTED_CHALLENGE)
{
       sasl->last_state = SASL_POSTED_MECHANISMS;
     }
-    // If we already pretended to receive outcome and we actually received outcome
-    // we must set last_state here as we've already stoped outputting from this layer
-    if (sasl->last_state==SASL_PRETEND_OUTCOME && (desired_state==SASL_RECVED_OUTCOME_SUCCEED
|| desired_state==SASL_RECVED_OUTCOME_FAIL) ) {
-        sasl->last_state = desired_state;
-    }
     sasl->desired_state = desired_state;
     // Don't emit transport event on error as there will be a TRANSPORT_ERROR event
     if (desired_state != SASL_ERROR) pni_emit(transport);
@@ -188,12 +179,6 @@ static void pni_post_sasl_frame(pn_transport_t *transport)
                     out.size, out.start);
       pni_emit(transport);
       break;
-    case SASL_PRETEND_OUTCOME:
-      if (sasl->last_state < SASL_POSTED_INIT) {
-        desired_state = SASL_POSTED_INIT;
-        continue;
-      }
-      break;
     case SASL_POSTED_MECHANISMS: {
       // TODO: Hardcoded limit of 16 mechanisms
       char *mechs[16];
@@ -317,15 +302,17 @@ static ssize_t pn_input_read_sasl(pn_transport_t* transport, unsigned
int layer,
     return pn_dispatcher_input(transport, bytes, available, false, &transport->halt);
   }
 
+  if (!pni_sasl_is_final_output_state(sasl)) {
+    return pni_passthru_layer.process_input(transport, layer, bytes, available);
+  }
+
   if (pni_sasl_impl_can_encrypt(transport)) {
     sasl->max_encrypt_size = pni_sasl_impl_max_encrypt_size(transport);
     if (transport->trace & PN_TRACE_DRV)
       pn_transport_logf(transport, "SASL Encryption enabled: buffer=%d", sasl->max_encrypt_size);
     transport->io_layers[layer] = &sasl_encrypt_layer;
-  } else if (sasl->client) {
-    transport->io_layers[layer] = &pni_passthru_layer;
   } else {
-    return pni_passthru_layer.process_input(transport, layer, bytes, available);
+    transport->io_layers[layer] = &pni_passthru_layer;
   }
   return transport->io_layers[layer]->process_input(transport, layer, bytes, available);
 }
@@ -386,8 +373,12 @@ static ssize_t pn_output_write_sasl(pn_transport_t* transport, unsigned
int laye
     return pn_dispatcher_output(transport, bytes, available);
   }
 
-  // We only get here if there is nothing to output and we're a final output state
-  if (sasl->outcome != PN_SASL_OK && pni_sasl_is_final_input_state(sasl)) {
+  if (!pni_sasl_is_final_input_state(sasl)) {
+    return pni_passthru_layer.process_output(transport, layer, bytes, available );
+  }
+
+  // We only get here if there is nothing to output and we're in a final state
+  if (sasl->outcome != PN_SASL_OK) {
     return PN_EOS;
   }
 
@@ -397,8 +388,6 @@ static ssize_t pn_output_write_sasl(pn_transport_t* transport, unsigned
int laye
     if (transport->trace & PN_TRACE_DRV)
       pn_transport_logf(transport, "SASL Encryption enabled: buffer=%d", sasl->max_encrypt_size);
     transport->io_layers[layer] = &sasl_encrypt_layer;
-  } else if (sasl->client) {
-    return pni_passthru_layer.process_output(transport, layer, bytes, available );
   } else {
     transport->io_layers[layer] = &pni_passthru_layer;
   }
@@ -541,26 +530,6 @@ void pn_sasl_free(pn_transport_t *transport)
   }
 }
 
-// This is a hack to tell us that
-// no actual negotiation is going to happen and we can go
-// straight to the AMQP layer; it can only work on the client side
-// As the server doesn't know if SASL is even active until it sees
-// the SASL header from the client first.
-static void pni_sasl_force_anonymous(pn_transport_t *transport)
-{
-  pni_sasl_t *sasl = transport->sasl;
-  if (sasl->client) {
-    // Pretend we got sasl mechanisms frame with just ANONYMOUS
-    if (pni_init_client(transport) &&
-        pni_process_mechanisms(transport, "ANONYMOUS")) {
-      pni_sasl_set_desired_state(transport, SASL_PRETEND_OUTCOME);
-    } else {
-      sasl->outcome = PN_SASL_PERM;
-      pni_sasl_set_desired_state(transport, SASL_RECVED_OUTCOME_FAIL);
-    }
-  }
-}
-
 void pni_sasl_set_remote_hostname(pn_transport_t * transport, const char * fqdn)
 {
   pni_sasl_t *sasl = transport->sasl;
@@ -600,10 +569,6 @@ void pn_sasl_allowed_mechs(pn_sasl_t *sasl0, const char *mechs)
     pni_sasl_t *sasl = get_sasl_internal(sasl0);
     free(sasl->included_mechanisms);
     sasl->included_mechanisms = mechs ? pn_strdup(mechs) : NULL;
-    if (mechs && strcmp(mechs, "ANONYMOUS")==0 ) {
-      pn_transport_t *transport = get_transport_internal(sasl0);
-      pni_sasl_force_anonymous(transport);
-    }
 }
 
 void pn_sasl_set_allow_insecure_mechs(pn_sasl_t *sasl0, bool insecure)
@@ -666,9 +631,6 @@ int pn_do_mechanisms(pn_transport_t *transport, uint8_t frame_type, uint16_t
cha
 {
   pni_sasl_t *sasl = transport->sasl;
 
-  // If we already pretended we got the ANONYMOUS mech then ignore
-  if (sasl->last_state==SASL_PRETEND_OUTCOME) return 0;
-
   // This scanning relies on pn_data_scan leaving the pn_data_t cursors
   // where they are after finishing the scan
   pn_string_t *mechs = pn_string("");

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/6738c16f/tests/python/proton_tests/engine.py
----------------------------------------------------------------------
diff --git a/tests/python/proton_tests/engine.py b/tests/python/proton_tests/engine.py
index 0a6eb8d..e7708da 100644
--- a/tests/python/proton_tests/engine.py
+++ b/tests/python/proton_tests/engine.py
@@ -2665,10 +2665,16 @@ class SaslEventTest(CollectorTest):
     s.allowed_mechs("ANONYMOUS PLAIN")
     transport.bind(conn)
     self.expect(Event.CONNECTION_INIT, Event.CONNECTION_BOUND)
-    transport.push(str2bin('AMQP\x03\x01\x00\x00\x00\x00\x00\x1c\x02\x01\x00\x00\x00S@'
-                           '\xc0\x0f\x01\xe0\x0c\x01\xa3\tANONYMOUS\x00\x00\x00\x10'
-                           '\x02\x01\x00\x00\x00SD\xc0\x03\x01P\x00AMQP\x00\x01\x00'
-                           '\x00'))
+    transport.push(str2bin(
+        # SASL
+        'AMQP\x03\x01\x00\x00'
+        # @sasl-mechanisms(64) [sasl-server-mechanisms=@PN_SYMBOL[:ANONYMOUS]]
+        '\x00\x00\x00\x1c\x02\x01\x00\x00\x00S@\xc0\x0f\x01\xe0\x0c\x01\xa3\tANONYMOUS'
+        # @sasl-outcome(68) [code=0]
+        '\x00\x00\x00\x10\x02\x01\x00\x00\x00SD\xc0\x03\x01P\x00'
+        # AMQP
+        'AMQP\x00\x01\x00\x00'
+         ))
     self.expect(Event.TRANSPORT)
     p = transport.pending()
     bytes = transport.peek(p)
@@ -2676,6 +2682,7 @@ class SaslEventTest(CollectorTest):
 
     server = Transport(Transport.SERVER)
     server.push(bytes)
+    assert s.outcome == SASL.OK
     assert server.sasl().outcome == SASL.OK
 
   def testPipelinedServerWriteFirst(self):
@@ -2690,14 +2697,21 @@ class SaslEventTest(CollectorTest):
     p = transport.pending()
     bytes = transport.peek(p)
     transport.pop(p)
-    self.expect(Event.CONNECTION_INIT, Event.CONNECTION_BOUND, Event.TRANSPORT)
-    transport.push(str2bin('AMQP\x03\x01\x00\x00\x00\x00\x00\x1c\x02\x01\x00\x00\x00S@'
-                           '\xc0\x0f\x01\xe0\x0c\x01\xa3\tANONYMOUS\x00\x00\x00\x10'
-                           '\x02\x01\x00\x00\x00SD\xc0\x03\x01P\x00AMQP\x00\x01\x00'
-                           '\x00'))
+    self.expect(Event.CONNECTION_INIT, Event.CONNECTION_BOUND)
+    transport.push(str2bin(
+        # SASL
+        'AMQP\x03\x01\x00\x00'
+        # @sasl-mechanisms(64) [sasl-server-mechanisms=@PN_SYMBOL[:ANONYMOUS]]
+        '\x00\x00\x00\x1c\x02\x01\x00\x00\x00S@\xc0\x0f\x01\xe0\x0c\x01\xa3\tANONYMOUS'
+        # @sasl-outcome(68) [code=0]
+        '\x00\x00\x00\x10\x02\x01\x00\x00\x00SD\xc0\x03\x01P\x00'
+        # AMQP
+        'AMQP\x00\x01\x00\x00'
+        ))
     self.expect(Event.TRANSPORT)
     p = transport.pending()
     bytes = transport.peek(p)
     transport.pop(p)
+    assert s.outcome == SASL.OK
     # XXX: the bytes above appear to be correct, but we don't get any
     # sort of event indicating that the transport is authenticated

http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/6738c16f/tests/python/proton_tests/sasl.py
----------------------------------------------------------------------
diff --git a/tests/python/proton_tests/sasl.py b/tests/python/proton_tests/sasl.py
index 6adb77d..29b578d 100644
--- a/tests/python/proton_tests/sasl.py
+++ b/tests/python/proton_tests/sasl.py
@@ -80,6 +80,15 @@ def _testSaslMech(self, mech, clientUser='user@proton', authUser='user@proton',
 class Test(common.Test):
   pass
 
+def consumeAllOuput(t):
+  stops = 0
+  while stops<1:
+    out = t.peek(1024)
+    l = len(out)
+    t.pop(l)
+    if l <= 0:
+      stops += 1
+
 class SaslTest(Test):
 
   def setUp(self):
@@ -92,104 +101,67 @@ class SaslTest(Test):
   def pump(self):
     pump(self.t1, self.t2, 1024)
 
-  # Note that due to server protocol autodetect, there can be no "pipelining"
-  # of protocol frames from the server end only from the client end.
-  #
-  # This is because the server cannot know which protocol layers are active
-  # and therefore which headers need to be sent,
-  # until it sees the respective protocol headers from the client.
+  # We have to generate the client frames manually because proton does not
+  # generate pipelined SASL and AMQP frames together
   def testPipelinedClient(self):
+    # TODO: When PROTON-1136 is fixed then remove this test
     if "java" in sys.platform:
-      raise Skipped("Proton-J does not support client pipelining")
+      raise Skipped("Proton-J does not support pipelined client input")
 
-    # Client
-    self.s1.allowed_mechs('ANONYMOUS')
     # Server
     self.s2.allowed_mechs('ANONYMOUS')
 
-    assert self.s1.outcome is None
+    c2 = Connection()
+    self.t2.bind(c2)
+
     assert self.s2.outcome is None
 
     # Push client bytes into server
-    out1 = self.t1.peek(1024)
-    self.t1.pop(len(out1))
-    self.t2.push(out1)
-
-    out2 = self.t2.peek(1024)
-    self.t2.pop(len(out2))
+    self.t2.push(str2bin(
+        # SASL
+        'AMQP\x03\x01\x00\x00'
+        # @sasl-init(65) [mechanism=:ANONYMOUS, initial-response=b"anonymous@fuschia"]
+        '\x00\x00\x002\x02\x01\x00\x00\x00SA\xd0\x00\x00\x00"\x00\x00\x00\x02\xa3\x09ANONYMOUS\xa0\x11anonymous@fuschia'
+        # AMQP
+        'AMQP\x00\x01\x00\x00'
+        # @open(16) [container-id="", channel-max=1234]
+        '\x00\x00\x00!\x02\x00\x00\x00\x00S\x10\xd0\x00\x00\x00\x11\x00\x00\x00\x0a\xa1\x00@@`\x04\xd2@@@@@@'
+        ))
+
+    consumeAllOuput(self.t2)
 
-    assert self.s1.outcome is None
-
-    self.t1.push(out2)
-
-    assert self.s1.outcome == SASL.OK
     assert self.s2.outcome == SASL.OK
+    assert c2.state & Endpoint.REMOTE_ACTIVE
 
-  def testPipelinedClientFail(self):
-    if "java" in sys.platform:
-      raise Skipped("Proton-J does not support client pipelining")
-
+  def testPipelinedServer(self):
     # Client
     self.s1.allowed_mechs('ANONYMOUS')
-    # Server
-    self.s2.allowed_mechs('PLAIN DIGEST-MD5 SCRAM-SHA-1')
-
-    assert self.s1.outcome is None
-    assert self.s2.outcome is None
-
-    # Push client bytes into server
-    out1 = self.t1.peek(1024)
-    self.t1.pop(len(out1))
-    self.t2.push(out1)
-
-    out2 = self.t2.peek(1024)
-    self.t2.pop(len(out2))
-
-    assert self.s1.outcome is None
-
-    self.t1.push(out2)
-
-    assert self.s1.outcome == SASL.AUTH
-    assert self.s2.outcome == SASL.AUTH
-
-  def testSaslAndAmqpInSingleChunk(self):
-    if "java" in sys.platform:
-      raise Skipped("Proton-J does not support client pipelining")
-
-    self.s1.allowed_mechs('ANONYMOUS')
-    self.s2.allowed_mechs('ANONYMOUS')
 
-    # do some work to generate AMQP data
     c1 = Connection()
-    c2 = Connection()
     self.t1.bind(c1)
-    c1._transport = self.t1
-    self.t2.bind(c2)
-    c2._transport = self.t2
 
-    c1.open()
-
-    # get all t1's output in one buffer then pass it all to t2
-    out1_sasl_and_amqp = str2bin("")
-    t1_still_producing = True
-    while t1_still_producing:
-      out1 = self.t1.peek(1024)
-      self.t1.pop(len(out1))
-      out1_sasl_and_amqp += out1
-      t1_still_producing = out1
-
-    t2_still_consuming = True
-    while t2_still_consuming:
-      num = min(self.t2.capacity(), len(out1_sasl_and_amqp))
-      self.t2.push(out1_sasl_and_amqp[:num])
-      out1_sasl_and_amqp = out1_sasl_and_amqp[num:]
-      t2_still_consuming = num > 0 and len(out1_sasl_and_amqp) > 0
+    assert self.s1.outcome is None
 
-    assert len(out1_sasl_and_amqp) == 0, (len(out1_sasl_and_amqp), out1_sasl_and_amqp)
+    # Push server bytes into client
+    # Commented out lines in this test are where the client input processing doesn't
+    # run after output processing even though there is input waiting
+    self.t1.push(str2bin(
+        # SASL
+        'AMQP\x03\x01\x00\x00'
+        # @sasl-mechanisms(64) [sasl-server-mechanisms=@PN_SYMBOL[:ANONYMOUS]]
+        '\x00\x00\x00\x1c\x02\x01\x00\x00\x00S@\xc0\x0f\x01\xe0\x0c\x01\xa3\tANONYMOUS'
+        # @sasl-outcome(68) [code=0]
+        '\x00\x00\x00\x10\x02\x01\x00\x00\x00SD\xc0\x03\x01P\x00'
+        # AMQP
+        'AMQP\x00\x01\x00\x00'
+        # @open(16) [container-id="", channel-max=1234]
+        '\x00\x00\x00!\x02\x00\x00\x00\x00S\x10\xd0\x00\x00\x00\x11\x00\x00\x00\x0a\xa1\x00@@`\x04\xd2@@@@@@'
+        ))
+
+    consumeAllOuput(self.t1)
 
-    # check that t2 processed both the SASL data and the AMQP data
-    assert self.s2.outcome == SASL.OK
-    assert c2.state & Endpoint.REMOTE_ACTIVE
+    assert self.s1.outcome == SASL.OK
+    assert c1.state & Endpoint.REMOTE_ACTIVE
 
   def testPipelined2(self):
     if "java" in sys.platform:


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message