Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl: Add support for {active,N} #2072

Merged
merged 4 commits into from
Mar 4, 2019
Merged

Conversation

essen
Copy link
Contributor

@essen essen commented Dec 24, 2018

There's probably missing stuff for DTLS, and the documentation is also missing, but this is a good time to get a first review. The added tests pass and check the option with listen, server and client sockets. I'll probably need some guidance about DTLS.

All functionality is there and tests pass! Documentation and possibly some refactoring remains.

@essen essen changed the base branch from master to maint December 24, 2018 23:35
@IngelaAndin IngelaAndin added team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI labels Jan 2, 2019
@IngelaAndin
Copy link
Contributor

We will start with testing what you got, more feedback will come once more people are back from the Christmas and New Year holidays.

@IngelaAndin
Copy link
Contributor

We have not had time to review the code yet but the new test is failing quite a lot.
Also other tests are failing but it not clear yet if that is related or not. More feedback to come
here are some examples of how the test fails at the moment.

** CT Error Notification 2019-01-07 07:08:53.319 ***
ssl_basic_SUITE:'-active_n/1-fun-1-' failed on line 2090
Reason: error

*** CT Error Notification 2019-01-07 07:04:05.217 ***
ssl_basic_SUITE:active_n_common failed on line 2134
Reason: error

@IngelaAndin IngelaAndin removed the testing currently being tested, tag is used by OTP internal CI label Jan 7, 2019
@essen
Copy link
Contributor Author

essen commented Jan 7, 2019

How did you get these line numbers? 2090 is not inside a fun here. Could you point in the diff where it occurs? Thanks.

@IngelaAndin
Copy link
Contributor

I built ssl on your PR branch and then I get the following:

*** CT Error Notification 2019-01-07 15:55:07.207 ***
ssl_basic_SUITE:active_n_common failed on line 2099
Reason: error

2093: active_n_common(S, N) ->
 2094:     ok = ssl:setopts(S, [{active,-N}]),
 2095:     receive
 2096:         {ssl_passive, S} -> ok
 2097:     after
 2098:         1000 ->
 2099:             error({error,ssl_passive_failure})
 2100:     end,
 2101:     [{active,false}] = ok(ssl:getopts(S, [active])),
 2102:     ok = ssl:setopts(S, [{active,0}]),
 2103:     receive
 2104:         {ssl_passive, S} -> ok
 2105:     after
 2106:         1000 ->
 2107:             error({error,ssl_passive_failure})
 2108:     end,
 2109:     ok = ssl:setopts(S, [{active,32767}]),
 2110:     {error,{options,_}} = ssl:setopts(S, [{active,1}]),
 2111:     {error,{options,_}} = ssl:setopts(S, [{active,-32769}]),
 2112:     ok = ssl:setopts(S, [{active,-32768}]),
 2113:     receive
 2114:         {ssl_passive, S} -> ok
 2115:     after
 2116:         1000 ->
 2117:             error({error,ssl_passive_failure})
 2118:     end,
 2119:     [{active,false}] = ok(ssl:getopts(S, [active])),
 2120:     ok = ssl:setopts(S, [{active,N}]),
 2121:     ok = ssl:setopts(S, [{active,true}]),
 2122:     [{active,true}] = ok(ssl:getopts(S, [active])),
 2123:     receive
 2124:         _ -> error({error,active_n})
 2125:     after
 2126:         0 ->
 2127:             ok
 2128:     end,
 2129:     ok = ssl:setopts(S, [{active,N}]),
 2130:     ok = ssl:setopts(S, [{active,once}]),
 2131:     [{active,once}] = ok(ssl:getopts(S, [active])),
 2132:     receive
 2133:         _ -> error({error,active_n})
 2134:     after
 2135:         0 ->
 2136:             ok
 2137:     end,
 2138:     {error,{options,_}} = ssl:setopts(S, [{active,32768}]),
 2139:     ok = ssl:setopts(S, [{active,false}]),
 2140:     [{active,false}] = ok(ssl:getopts(S, [active])),
 2141:     ok.

@IngelaAndin IngelaAndin added the waiting waiting for changes/input from author label Jan 8, 2019
@essen
Copy link
Contributor Author

essen commented Jan 12, 2019

I can reproduce. I was running this test case directly but when running the test suite it fails against some configurations indeed. DTLS is expected, sslv3 and tls1 are not. Will investigate.

@essen
Copy link
Contributor Author

essen commented Jan 14, 2019

I have pushed a fix for the sslv3 and tls1 cases. I have not yet looked into implementing DTLS but I will soon.

@IngelaAndin
Copy link
Contributor

As the option is emulated and never set on the real socket the handling ought to be quite similar.

@essen
Copy link
Contributor Author

essen commented Jan 16, 2019

I've pushed the commit for DTLS. The tests pass and the functionality should be complete. Documentation will still need to be updated.

The code looks very similar between tls/dtls but it's difficult to abstract because of small differences between tls_socket/dtls_socket. Open to suggestions.

@essen
Copy link
Contributor Author

essen commented Jan 16, 2019

And one more push to remove some unnecessary code and hopefully fix Dialyzer. Edit: Fixed!

@IngelaAndin IngelaAndin added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author labels Jan 17, 2019
@IngelaAndin
Copy link
Contributor

The below is no longer true form the user perspective. An optimization will add those two application data TLS-records into one application data delivery. Hopefully I can merge my branch in the beginning of next week so that you can see it to. Otherwise test seems to be doing good now. I hope my team will have some time to look closer at the code soon.

%% For TLSv1 and SSLv3 we receive the first byte as a separate message.
 2045:     %% Therefore we need twice the number of active messages.
 2046:     {ok, Infos} = ssl:connection_information(C, [protocol]),
 2047:     Multiplier = case Infos of
 2048:         [{_, sslv3}] -> 2;
 2049:         [{_, tlsv1}] -> 2;

@essen
Copy link
Contributor Author

essen commented Jan 18, 2019

Sounds good!

@IngelaAndin
Copy link
Contributor

Due to test problems unrelated to the ssl application I can not merge today. But hopefully tomorrow we I can merge so that you can remove multiplier workaround in the test case. When it comes to the documentation it probably does not need to be so extensive and mostly refer to inet. As it happens I am just in the process of reworking the documentation to use type specs so preferably we will postpone the documentation until I am done with that.

@essen
Copy link
Contributor Author

essen commented Jan 22, 2019

I will try removing the unreachable clause tomorrow and I will then wait for the documentation.

Note that the documentation in inet doesn't mention tcp_passive, it's mentioned in gen_tcp:connect but that doesn't look like the right place. Might be worth revisiting at the same time.

@IngelaAndin
Copy link
Contributor

I have now merge my optimization sha: 6ce69af that
should render multiplier workaround in test unnecessary.

@essen
Copy link
Contributor Author

essen commented Jan 23, 2019

I've rebased and removed the multiplier workaround.

@IngelaAndin IngelaAndin removed the testing currently being tested, tag is used by OTP internal CI label Jan 24, 2019
throw(einval);
{_, N} when is_integer(N), N > 32767 ->
throw(einval);
{_, N} when is_integer(N) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check of active N value is done in several places in a similar way, I was thinking it could be nice with some helper function that preforms the actual check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little difficult to have a common check_active_n because we have 3 scenarios where it is called: DTLS sockets, TLS sockets and TLS listening sockets. The code path for each of these is different. In the implementation of this PR the latter 2 share a common check_active_n while DTLS has its own. I'm not seeing an obvious way to share the code between the 3 currently.

Of course the two clauses checking the upper and lower bounds are common but we don't gain much (if anything) from refactoring that.

Copy link
Contributor

@bmk bmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok. See my comment about the active check/calculation code.

/BMK

@essen
Copy link
Contributor Author

essen commented Feb 11, 2019

I'll try to refactor a bit.

@IngelaAndin IngelaAndin added testing currently being tested, tag is used by OTP internal CI and removed waiting waiting for changes/input from author testing currently being tested, tag is used by OTP internal CI labels Feb 18, 2019
@IngelaAndin
Copy link
Contributor

I tried to put your branch in testing again, but I am afraid that my branch that shrinks the outer state record,that I merged today creates some problems for your branch. Could you please rebase on latest maint.

@IngelaAndin
Copy link
Contributor

That is problems are not seen as merge conflicts but rather as compiler errors after merging as state data has been moved around a lot.

@essen
Copy link
Contributor Author

essen commented Feb 19, 2019

I have fixed the issue.

This also made one of the tests fail. The fix is in the most recent commit. Please check it. Not sure it is necessary to update socket_options in other clauses.

@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Feb 19, 2019
@IngelaAndin
Copy link
Contributor

I think the code is looking good :) And I do think that it will be sufficient for now to update the socket_options in the place that you have. However, it might be interesting to handle this
in the new dist_read_application_data @RaimoNiskanen if there could be a benefit in making it
{active, N} instead of active? Just saying we should think about it.

@IngelaAndin
Copy link
Contributor

We are currently running our own branch that includes you commits to speed up testing so it can make OTP-21.3, as this time we got new collisions with our changes.

@essen
Copy link
Contributor Author

essen commented Feb 22, 2019

Glad to hear, especially considering I am fairly busy at the moment.

@essen
Copy link
Contributor Author

essen commented Mar 4, 2019

All good?

@IngelaAndin
Copy link
Contributor

Yes it is looking good, we will merge our version of the branch today. I was away on "sports holiday" (Swedish kids has a week of school to do winter sports) last week there of the delay.

@essen
Copy link
Contributor Author

essen commented Mar 4, 2019

Swedish kids are lucky. :-) Cheers

@RaimoNiskanen RaimoNiskanen merged commit 59c1634 into erlang:maint Mar 4, 2019
@RaimoNiskanen RaimoNiskanen removed the testing currently being tested, tag is used by OTP internal CI label Mar 4, 2019
@RaimoNiskanen
Copy link
Contributor

I did a local repository merge to resolve the merge conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants