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

Add Assert for TlsHandler.MediationStream #51

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

yyjdelete
Copy link
Collaborator

@yyjdelete yyjdelete commented May 23, 2021

Check whether MqttServerAndClient will hit the assert in pipeline.=>It hit the assert

Actual fix is not included.
(I do this later when I'm free, and the fix should like Azure/DotNetty#374)

@@ -60,6 +63,7 @@ public void ExpandSource(int count)
if (sslBuffer.IsEmpty)
{
// there is no pending read operation - keep for future
Debug.Assert(_readCompletionSource == null);
Copy link
Collaborator Author

@yyjdelete yyjdelete May 23, 2021

Choose a reason for hiding this comment

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

ReadAsync can also be called with zero length array, and it should be finished if any data is available.

Copy link
Owner

@cuteant cuteant May 23, 2021

Choose a reason for hiding this comment

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

真是仔细啊, 👍

@yyjdelete
Copy link
Collaborator Author

MqttServerAndClient not failed this time, but see on some other test(TlsHandlerTest.TlsRead) in net5.0_windows

System.IO.IOException : The write operation failed, see inner exception.
---- DotNetty.Codecs.DecoderException : Exception of type 'DotNetty.Codecs.DecoderException' was thrown.
-------- Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException : Method Debug.Fail failed with '
', and was translated to Microsoft.VisualStudio.TestPlatform.TestHost.DebugAssertException to avoid terminating the process hosting the test.
......
at DotNetty.Handlers.Tls.TlsHandler.MediationStream.ResetSource() in D:\a\1\s\src\DotNetty.Handlers\Tls\TlsHandler.MediationStream.NetCore.cs:line 50

@cuteant
Copy link
Owner

cuteant commented May 23, 2021

能否把证书一块更新下?

…and seems obsoleted and no longer work on net5.0+ubuntu20.04

Generated with
```
openssl genrsa -out ssl.key 2048
openssl req -new -x509 -key ssl.key -out dotnetty.com.cer -days 3650 -subj /CN=dotnetty.com
openssl req -new -x509 -key ssl.key -out contoso.com.cer -days 3650 -subj /CN=contoso.com
openssl pkcs12 -export -out dotnetty.com.pfx -inkey ssl.key -in dotnetty.com.cer
openssl pkcs12 -export -out contoso.com.pfx -inkey ssl.key -in contoso.com.cer
```
password="password"
@yyjdelete
Copy link
Collaborator Author

奇怪了, 怎么我改了测试之后mac下都跑不过了, Assert.False(ch.Finish());也被触发了, TlsHandler的代码我都还没开始改...
下周再看看..

Mac
1. System.PlatformNotSupportedException : The requested combination of SslProtocols (Tls, Tls12) is not valid for this platform because it skips intermediate versions
2. System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
---- Interop+AppleCrypto+SslException : bad protocol version
(serverProtocol: Tls | Tls12, clientProtocol: Tls | Tls11)

Windows:
1. System.TypeInitializationException : The type initializer for 'Platform' threw an exception.
---- System.ComponentModel.Win32Exception : The client and server cannot communicate, because they do not possess a common algorithm.

Ubuntu
1. 之前新加的那个Assert
2. Assert.False() Failure=>`Assert.False(ch.Finish());`

Copy link
Owner

@cuteant cuteant left a comment

Choose a reason for hiding this comment

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

非windows平台的测试还是要把 SslProtocols.Tls 去掉

@yyjdelete
Copy link
Collaborator Author

yyjdelete commented May 24, 2021

非windows平台的测试还是要把 SslProtocols.Tls 去掉

之前删if的时候没注意, 使用的协议版本必须要连续, 已经在新的提交里面修了, 换了一种方式验证握手的协议版本
只是不知道为啥, pipeline里面现在TlsHandleTest.TlsRead对应的所有测试项都显示同一个名字(还是已经删掉了的), 但raw log和Attachments里面又是正常的, 看着好难受...
我看别的像TlsHandleTest.TlsWrite的显示又是正常的啊, 而且是正常分组了的, 奇怪
image
image

现在主要剩下失败的应该就是新加的那个Assert, 和不知道为啥新触发的Assert.False(ch.Finish());

@yyjdelete
Copy link
Collaborator Author

  1. 查了下提交日志, 报错的那个Assert.False(ch.Finish());(好像是仅在握手成Tls13的情况下报错)还是我加的, 但想不起来是当时加这个是为了验证什么了, 可能当时看到返回的都是false, 这个Assert我先屏蔽掉吧.
    https://github.com/Azure/DotNetty/pull/366/files#diff-f34ff1ddf35c6af0420e796aa97007ec141fe8e7e7737c77a69b958472ef8776R107
  2. 我本机在windows上测试下列组合握手失败(最新提交的版本忘了应该优先测试新版Tls了, 我后面反下序), 本身未触发任何的Assert, 但原生的SslStream用同一组合可以正常握手成功, 需要调查下原因
  Message: 
    System.Security.Authentication.AuthenticationException : Authentication failed, see inner exception.
    ---- System.ComponentModel.Win32Exception : 因为算法不同,客户端和服务器无法通信。(The client and server cannot communicate, because they do not possess a common algorithm.)

  Stack Trace: 
    SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm)
    TaskUtil.WithTimeout(Task taskToComplete, TimeSpan timeout) line 428
    TlsHandlerTest.SetupStreamAndChannelAsync(Boolean isClient, IEventExecutor executor, IWriteStrategy writeStrategy, SslProtocols serverProtocol, SslProtocols clientProtocol, List`1 writeTasks) line 344
    TlsHandlerTest.TlsWrite(Int32[] frameLengths, Boolean isClient, SslProtocols serverProtocol, SslProtocols clientProtocol) line 233
    TlsHandlerTest.TlsWrite(Int32[] frameLengths, Boolean isClient, SslProtocols serverProtocol, SslProtocols clientProtocol) line 271
    --- End of stack trace from previous location ---
    ----- Inner Stack Trace -----

  Standard Output: 
    frameLengths: 1
    isClient: True
    serverProtocol: Tls13
    clientProtocol: Tls, Tls11, Tls12, Tls13
    writeStrategy: as-is
  1. TlsRead的问题, 好像在vs里面也分不了组, 只是不想pipeline一样会名称错乱, 可能是pipeline的名称缓存存在bug
    好像是MemberData参数包含不可序列化的对象的时候就会这样
    Debugging Theory with non-serializable data in DataMember launches debug of all tests cases xunit/xunit#1679

具体包含以下测试, 我看把TlsRead的writeStrategy改成个枚举传入
其他好改的改, 不好改的和TlsRead一样, 加个this.Output.WriteLine()输出实际参数好了, 反正别的基本上也不会失败
或者试下设置DisableDiscoveryEnumeration = true

DotNetty.Codecs.Http.Tests.HttpServerKeepAliveHandlerTest.ConnectionCloseHeaderHandledCorrectly
DotNetty.Codecs.Http.Tests.HttpServerKeepAliveHandlerTest.KeepAlive
DotNetty.Codecs.Http.Tests.HttpServerKeepAliveHandlerTest.PipelineKeepAlive
DotNetty.Codecs.Http2.Tests.HpackTest.Test
DotNetty.Handlers.Tests.SniHandlerTest.TlsRead
DotNetty.Handlers.Tests.TlsHandlerTest.TlsRead

@cuteant
Copy link
Owner

cuteant commented May 24, 2021

TlsRead最开始VS是可以分组的,后来随着VS的升级......
我的VPN挂了,现在能上github看Issue和PR要看几率:cold_sweat:

Copy link
Owner

@cuteant cuteant left a comment

Choose a reason for hiding this comment

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

要不 MqttServerAndClient 先这样忽略掉?

#if LOCALTEST
        [Fact]
#else
        [Fact(Skip = "Azure DevOps")]
#endif

@yyjdelete
Copy link
Collaborator Author

设了DisableDiscoveryEnumeration好像没用, 而且不知道从哪缓存了一个旧版测试的名字, 先不管这个了

测试的名字全显示成一样的是pipeline本身和xunit+vstest的兼容性问题, 可惜连接过期了看不到当初给的规避方案了, 而且估计短时间内也不会修, 虽然不知道为啥dotnet/runtime本身用到的测试好像都是对的...
https://developercommunity.visualstudio.com/t/xunit-theory-tests-using-complex-memberdata-have-d/1037085#T-N1068757


MqttServerAndClient 那个我会修的, 因为现在还剩下的基本上都是Azure/DotNetty#374那个问题, 就是我最初加的那个Assert, 但可能要周末才有空改, 而且这块本身的代码也是一团乱麻, 搞得我都想去抄system.io.pipelines的实现或者引用了...
其实需要的MediationStream基本上就是这么一个东西
https://github.com/AArnott/Nerdbank.Streams/blob/main/doc/FullDuplexStream.md

@cuteant
Copy link
Owner

cuteant commented May 25, 2021

😃 👍

@yyjdelete
Copy link
Collaborator Author

不知道是不是最近win10预览版的问题, 现在我一跑dotnet sdk就蓝屏重启, 但跑别的又没问题. 现在完全没法调, 先延下期...

@cuteant
Copy link
Owner

cuteant commented May 30, 2021

好的,预览版的东东少用,之前用过vs的预览版,做单元测试的时候差点把我搞崩溃
我等你这边完成后再推 #52

@yyjdelete
Copy link
Collaborator Author

yyjdelete commented Jun 9, 2021

Updated:

  1. Ub下测试通过(还没有PUSH)

  2. Tls13在windows系统上测试失败的原因已确认, 并可以在dotnetty以外独立重现:
    在windows上系统疑似会为同一域名+客户端协议缓存最终握手的协议, 因此更换服务端协议后无法重新谈判到Tls13
    疑似就是Some HTTP & TLS test are failing on Insider Preview Windows build dotnet/runtime#47378
    准备参考dotnet的解决方案, 生成随机(子)域名用于测试, 我研究下怎么重新生成上传一个带通配符(泛域名)的证书

  3. 还是TlsHandlerTest.TlsRead, 试了下在net6.0控制台下跑测试概率失败(数据错误), 但无法在调试中重现, 正在想办法打日志中看看是不是还存在什么别的问题...

DotNetty.Handlers.Tests.TlsHandlerTest.TlsRead(frameLengths: [100, 0, 1000], isClient: True, writeStrategy: as-is, serverProtocol: None, clientProtocol: None) [FAIL]
无法解密指定的数据

@cuteant
Copy link
Owner

cuteant commented Jun 9, 2021

😁 👍 等你的推送,我也测试下

@cuteant cuteant linked an issue Jun 26, 2021 that may be closed by this pull request
@cuteant
Copy link
Owner

cuteant commented Jul 15, 2021

@yyjdelete 我在 #65 中借用了 090b92d ,并做了些修改,重构了TlsHandler,做了些异常捕捉,确保 tlshandler 可以关闭,#46 可以杜绝了

@yyjdelete
Copy link
Collaborator Author

@cuteant 好的, 有时间的时候我看下然后rebase之后重新push下
现在的代码基本上没啥问题(虽然net6.0还是小概率失败)

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

Successfully merging this pull request may close these issues.

SSL Handshake failed on Ubuntu 18.04 using .NET 5.0
2 participants