Stream compression after sasl #616

ludwikbukowski · 2015-12-09T18:09:14Z

fixed compresion advertising after tls negotiation.
it's now possible to change negotation features order ( XEP-0170) due to Negotiation for Stream Compression ignored by server after SASL #242
few more tests for tls, compression, autentication and bind order in connect_SUITE
tests work with Sending works both with tls and zlib compression escalus#88

michalwski · 2015-12-10T08:20:45Z

I think you have to change rebar.config in test/ejabberd_tests to use escalus from your branch.

michalwski · 2015-12-10T08:25:08Z

apps/ejabberd/src/ejabberd_c2s.erl

            stream_start_features_before_bind(S);
-        {_, _} ->
+        {_, <<>>, _} ->
+            stream_start_features_before_auth(S);


In this branch the user is authenticated so it's misleading to use function named *_before_auth (even if the output is the same). Maybe the function name should be changed to reflect both cases.

You're right it can lead to confusion

michalwski · 2015-12-11T10:43:20Z

test/ejabberd_tests/tests/connect_SUITE.erl

+    Value.
+
+%% tests for different order feature negotiation
+tls_compression_authenticate(Config) ->


According to XEP-0138 we shouldn't allow compression before authentication. It may be used by malicious and not-registered users to attack the server.

Hence order of feature negotiation should be also changed in escalus_connection:start function:
https://github.com/esl/escalus/blob/master/src/escalus_connection.erl#L70

I've done it here esl/escalus#88

ludwikbukowski · 2015-12-11T16:12:42Z

I will try do some code refactoring so its still WIP

michalwski · 2016-02-22T12:13:12Z

apps/ejabberd/src/ejabberd_c2s.erl

@@ -772,7 +756,7 @@ wait_for_session_or_sm({xmlstreamelement, El}, StateData0) ->
        #iq{type = set, xmlns = ?NS_SESSION} ->
            maybe_open_session(El, StateData);
        _ ->
-            fsm_next_state(wait_for_session_or_sm, StateData)
+            maybe_do_compress(El, wait_for_session_or_sm, StateData)


I think we should end up in the same state after enabling the compression, but this time compression shouldn't be allowed. How else the binding process will happen?

Ah, sorry, forgot that you have to open session after enabling a feature... no comment then...

michalwski · 2016-03-17T14:55:11Z

apps/ejabberd/src/ejabberd_c2s.erl

@@ -734,7 +718,7 @@ wait_for_bind_or_resume({xmlstreamelement, El}, StateData) ->
                                   StateData#state{resource = R, jid = JID})
            end;
        _ ->
-            fsm_next_state(wait_for_bind_or_resume, StateData)
+            maybe_do_compress(El, wait_for_bind_or_resume, StateData)
    end;

 wait_for_bind_or_resume(timeout, StateData) ->


I think this name is not valid any more. Now we can do bind or resume or compression here. How about naming it wait_for_feature_after_auth?

ludwikbukowski · 2016-03-18T11:34:16Z

Why tests don't pass?

Escalus dependency is updated in this PR to Stream Compression after SASL escalus#97 but before there was released another PR (Explicitly pass specs to escalus:create_users/2 #651) which breaks backward compatibility in escalus:create_users/2 function.
The fix for the issue on the Mongoose side is here Explicitly pass specs to escalus:create_users/2 #651 and it is now merged to rel-1.7 branch
In order to make this PR working we should merge branch rel-1.7 into master and then rebase this PR

michalwski · 2016-03-18T11:39:46Z

Alternatively, we can rebase this branch on top of rel-1.7. The amount of changes and possible conflicts (while rebasing) will be comparable.

michalwski · 2016-03-29T07:19:12Z

apps/ejabberd/src/ejabberd_c2s.erl

+maybe_do_compress(El, NextState, StateData) ->
+    SockMod = (StateData#state.sockmod):get_sockmod(StateData#state.socket),
+    {Zlib, _} = StateData#state.zlib,
+    #xmlel{name = Name, attrs = Attrs, children = _} = El,


How about moving this matching to the function's head?
Also this part:

children = _

is redundant

michalwski · 2016-03-29T07:32:11Z

test/ejabberd_tests/tests/connect_SUITE.erl

@@ -57,6 +60,13 @@ groups() ->
                     invalid_stream_namespace]},
     {pre_xmpp_1_0, [], [pre_xmpp_1_0_stream]},
     {starttls, test_cases()},
+     {feature_order, [stream_features_test,


As we are in process of adopting parallel tests I strongly recommend using the parallel property for this group and escalus_fresh module. You can take a look at this SUITE [1] for inspiration how to use it (also outside story - test case server_announces_csi).

[1]. https://github.com/esl/MongooseIM/blob/684bc0fbcab4c6e298961bcb7a0147c2b0678c0a/test/ejabberd_tests/tests/xep_0352_csi_SUITE.erl

michalwski · 2016-03-29T08:08:04Z

Additionally I have a general comment/tip. Please set your IDE to use 4 spaces for tab and re-indent your changes. Currently you have 2 spaces.

Introduce a suite to test the order of stream feature advertisment and negotiation. To begin with test the stream compression feature.

…to negotiate compression after tls. There were matches on `tls` module which does not exist.

Fixed test for compression-authentication order Authentication before compression in stream_features_test

* Introducing state "wait_for_feature_before_auth" instead of "wait_for_feature_request" * Introducing state "waif_for_feature_after_auth" instead of "wait_for_bind_or_resume"

ludwikbukowski added the WIP 🚧 label Dec 9, 2015

ludwikbukowski force-pushed the stream-compression-after-SASL branch 2 times, most recently from 2e16000 to ab60c8c Compare December 9, 2015 18:12

michalwski reviewed Dec 10, 2015
View reviewed changes

ludwikbukowski force-pushed the stream-compression-after-SASL branch 3 times, most recently from 8505806 to e85a282 Compare December 10, 2015 16:29

michalwski reviewed Dec 11, 2015
View reviewed changes

ludwikbukowski force-pushed the stream-compression-after-SASL branch from 6dfb78b to 3349951 Compare February 17, 2016 11:40

michalwski mentioned this pull request Feb 22, 2016

MongooseIM 1.7.0 sprint 0 #679

Closed

7 tasks

michalwski added this to the MongooseIM 1.7.0 milestone Feb 22, 2016

michalwski reviewed Feb 22, 2016
View reviewed changes

michalwski mentioned this pull request Feb 22, 2016

Sending works both with tls and zlib compression esl/escalus#88

Merged

michalwski added the XEPs label Feb 24, 2016

ludwikbukowski force-pushed the stream-compression-after-SASL branch from e898148 to 18be0ae Compare March 17, 2016 13:12

michalwski reviewed Mar 17, 2016
View reviewed changes

ludwikbukowski force-pushed the stream-compression-after-SASL branch 4 times, most recently from 56960ca to 546a708 Compare March 21, 2016 11:01

ludwikbukowski added waiting-for-review ready and removed WIP 🚧 labels Mar 24, 2016

michalwski reviewed Mar 29, 2016
View reviewed changes

ludwikbukowski removed ready waiting-for-review labels Mar 29, 2016

Joseph Yiasemides and others added 12 commits March 29, 2016 14:16

Test Mongoose's stream feature negotiation

35cbad4

Introduce a suite to test the order of stream feature advertisment and negotiation. To begin with test the stream compression feature.

Compression advertised after tls negotiation. It's now also possible …

abcc433

…to negotiate compression after tls. There were matches on `tls` module which does not exist.

Test for tls, compression and autentication in connect_SUITE.

bb6b432

SASL now is possible before compression.

4a9010a

more tests in feature_order test group

86505c2

This version works but needs refactoring

7d1346e

Compression possible after resource binding

f3819f8

Tests for different order of negotiation binding and compression.

47deb55

Compression possible only after authentication.

2426914

Fixed test for compression-authentication order Authentication before compression in stream_features_test

Updated escalus dependency

e4cf1f1

Separation of compression checks.

daa9403

Differentiation ejabberd_c2s states depending on the auth.

8b3981a

* Introducing state "wait_for_feature_before_auth" instead of "wait_for_feature_request" * Introducing state "waif_for_feature_after_auth" instead of "wait_for_bind_or_resume"

ludwikbukowski force-pushed the stream-compression-after-SASL branch from 546a708 to b4e811c Compare March 29, 2016 13:04

ludwikbukowski added waiting-for-review ready labels Mar 29, 2016

ludwikbukowski added 2 commits March 29, 2016 15:19

Parallel tests in group feature_order

a30497d

workeround to ct_tty_hook for parlallel test cases

629010e

ludwikbukowski force-pushed the stream-compression-after-SASL branch from b4e811c to 629010e Compare March 29, 2016 13:19

michalwski merged commit 9f845ae into master Mar 31, 2016

michalwski deleted the stream-compression-after-SASL branch March 31, 2016 06:37

michalwski mentioned this pull request Mar 31, 2016

Negotiation for Stream Compression ignored by server after SASL #242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stream compression after sasl #616

Stream compression after sasl #616

ludwikbukowski commented Dec 9, 2015

michalwski commented Dec 10, 2015

michalwski Dec 10, 2015

ludwikbukowski Dec 10, 2015

michalwski Dec 11, 2015

ludwikbukowski Feb 17, 2016

ludwikbukowski commented Dec 11, 2015

michalwski Feb 22, 2016

michalwski Feb 22, 2016

michalwski Mar 17, 2016

ludwikbukowski commented Mar 18, 2016

michalwski commented Mar 18, 2016

michalwski Mar 29, 2016

michalwski Mar 29, 2016

michalwski commented Mar 29, 2016

Stream compression after sasl #616

Stream compression after sasl #616

Conversation

ludwikbukowski commented Dec 9, 2015

michalwski commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludwikbukowski commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludwikbukowski commented Mar 18, 2016

michalwski commented Mar 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalwski commented Mar 29, 2016