Return-Path: X-Original-To: apmail-qpid-commits-archive@www.apache.org Delivered-To: apmail-qpid-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 718AC198DD for ; Wed, 6 Apr 2016 14:43:31 +0000 (UTC) Received: (qmail 67909 invoked by uid 500); 6 Apr 2016 14:43:31 -0000 Delivered-To: apmail-qpid-commits-archive@qpid.apache.org Received: (qmail 67878 invoked by uid 500); 6 Apr 2016 14:43:31 -0000 Mailing-List: contact commits-help@qpid.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@qpid.apache.org Delivered-To: mailing list commits@qpid.apache.org Received: (qmail 67869 invoked by uid 99); 6 Apr 2016 14:43:31 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 06 Apr 2016 14:43:31 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 10112DFDE0; Wed, 6 Apr 2016 14:43:31 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: astitcher@apache.org To: commits@qpid.apache.org Message-Id: X-Mailer: ASF-Git Admin Mailer 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, 6 Apr 2016 14:43:31 +0000 (UTC) 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 Authored: Thu Mar 31 02:05:17 2016 -0400 Committer: Andrew Stitcher 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