Throw SSLException if SSLEngine inbound is closed before outbound.

[prbprbprb]

Fixes #839.

This should be mostly uncontroversial as it is already documented
to do so[1] but could cause app compat issues. A quick scan of
AOSP suggests no major issues however there is a CTS test for the
old behaviour[2] which will need changing.

The bulk of this change is regression tests for the correct behaviour
for the various possible orderings of close calls and TLS close
alerts. The behaviour change test is
closingInboundBeforeClosingOutboundShouldFail() in place of
closingInboundShouldOnlyCloseInbound(). Changes outside
ConscryptEngineTest are minimal.

Close behaviour before handshaking starts is undefined and we differ
from the RI, but I don't think that's problematic.

Obviously also needs documenting in Conscrypt and Android release
notes.

This also means that STATE_CLOSED_INBOUND is never reached, which
means it can be eliminated in a future CL allowing some minor
simplifications.

NB This can be merged independently of #844 and I'll rebase that
change on top of it.

[1] https://developer.android.com/reference/javax/net/ssl/SSLEngine#closeInbound()
[2] https://cs.android.com/android/platform/superproject/+/master:libcore/harmony-tests/src/test/java/org/apache/harmony/tests/javax/net/ssl/SSLEngineTest.java;l=611

[kruton]

I don't think the documentation indicates at all that this will throw SSLException if you don't call closeOutbound first. This will probably cause you a lot of app compat headaches; it's already difficult enough for app developers to use SSLEngine correctly and this seems to add to the gotchas.

Also concerning is that the stated reason for throwing does not seem to be covered:

@throws SSLException if this engine has not received the proper SSL/TLS close notification message from the peer.

Is this handled implicitly by the state transitions in reaction to BoringSSL's signal that it received end-of-stream from the other side?

[prbprbprb]

Yeah, my wording is a poor there, I'll fix it... I carried it over from the bug report.

Both peers are required to send close alerts at the end of the application data stream and not to close their inbound connection until they have received one from the peer. RFC 5246 also states that on receipt of a close alert, the connection transitions to fully closed and sends its own alert and "If the application protocol will not transfer any additional data, but will only close the underlying transport connection, then the implementation MAY choose to close the transport without waiting for the responding close_notify", which is what ConscryptEngine does.

So, if an app has called closeOutbound or a close alert has been received from the peer then it's OK to call closeInbound. The bulk of the tests codify that, but need better wording/docs I think.

However I didn't realise the spec changed for TLS 1.3 and now says "Each party MUST send a "close_notify" alert before closing its write side of the connection, unless it has already sent some error alert. This does not have any effect on its read side of the connection. Note that this is a change from versions of TLS prior to TLS 1.3 in which implementations were required to react to a "close_notify" by discarding pending writes and sending an immediate "close_notify" alert of their own" - that doesn't change the requirement to send a close_notify before closing the inbound connection, but it might mean we need to revisit the way we respond to incoming close alerts in a future change.

And of course this is related to PR #844 where ConscryptEngineSocket prevents the close alerts from flowing anyway by closing the underlying socket too soon.

Agree about app compat issues though, and so this change should be gated in some way. I'm designing something so we can gate on targetSdkLevel in the Android platform and some other criterion (using the same Annotations) on other platforms. Meanwhile I'll move most of the tests to a separate PR and clarify the wording.