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

Unitytls integration #784

Merged
merged 72 commits into from
Jan 17, 2018
Merged

Unitytls integration #784

merged 72 commits into from
Jan 17, 2018

Conversation

AndreasReich
Copy link

Provides an alternative TLS binding that will allow us to use TLS functionality on all unity platforms.

Use/Activation
Binding must be set from outside via native call (mono_unity_install_unitytls_interface). If this is done before Mono registers its TLS providers, unitytls will be the only available provider. Note that this registration mechanism provides only unitytls functionality that is actually used.
The unity side of this binding can be found in the unity branch platform/foundation/tls-module-monointegration

Support
Binding itself is fairly straight-forward and should provide support for everything that the two other main tls implementions (btls, appletls) support as well. However, there are most likely differences in cipher support, error reporting and robustness of loading data.

Testing
Black-box tests via the user facing SslStream are part of the change on the unity sided binding since currently that is the only place where unitytls is available. These tests show that with our TLS implementation, we have at least better TLS support than mono's current AppleTls implementation (in terms of TLS support and behavior being more similar to .Net)
Earlier versions that relied on a different mechanism for native calls, passed Mono's very shallow SslStreamTest.
There are currently no tests against Mono.Security which should work as well but has low priority for us.

Changed namespaces from UnityTls to Unity, change casing from 'TLS' to 'Tls'
…tr to Int8=byte ptr since char has a different meaning in C# than in C
…pdated UnityTls interface with trace methods
CheckAndThrow for verify results forwards now to the one without verify result.
Development up until this point happened on unity-2017-02-staging
Note that previous versions of mono would directly pass on whatever exception the implementation throws.
Now, not only is TlsException wrapped with the correct AuthentificationException but - since it is run form a task - it is passed on as an AggregateException
public override void FinishHandshake ()
{
// Query some data. Ignore errors on the way since failure is not crucial.
var cipherSuite = UnityTls.NativeInterface.unitytls_tlsctx_get_ciphersuite(m_TlsContext, null);
Copy link
Author

Choose a reason for hiding this comment

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

passing null for the errorstate is soon no longer supported. need to change this

Copy link
Author

Choose a reason for hiding this comment

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

[fixed]

…retrieving information about the connection from tlsctx.

Passing null for the errorstate is no longer supported in an upcoming unitytls version.

unsafe internal static partial class UnityTls
{
private const string DLLNAME = "MacStandalonePlayer_TLSModule_Dynamic.dylib";
Copy link
Member

Choose a reason for hiding this comment

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

leftover


public override void StartHandshake ()
{
// TODO: Client->Server authentification is not supported by UnityTls as of writing
Copy link
Member

Choose a reason for hiding this comment

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

should we fix this before pushing to master?

Copy link
Author

Choose a reason for hiding this comment

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

Is this feature fully functional and tested in UnityTls by now. If so, clearly I should add the necessary wiring and another integration test on the unity side of this PR!

Copy link
Member

Choose a reason for hiding this comment

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

it is

Copy link
Member

Choose a reason for hiding this comment

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

Together with SNI. I wonder how SNI is supported in Mono..

Copy link
Author

Choose a reason for hiding this comment

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

Done and tested.


// .Net implementation gives the server a verification callback (with null cert) even if AskForClientCertificate is false.
// We stick to this behavior here.
if (IsServer && !AskForClientCertificate) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have decided or even confirmed what the behavior is for unitytls in that regard. Feels like we at least need a conformance test to establish behavior.

Copy link
Author

Choose a reason for hiding this comment

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

I can look into that while wiring Client->Server auth to mono

Copy link
Author

Choose a reason for hiding this comment

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

done

}

return bufferLen;
} catch { // handle all exceptions since we don't want to let them go through native code.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is really okay... I imagine this is a fairly common error. Like dropping a network connection or some other io failure. In which case we will hide the real reason for the failure.

Copy link
Author

Choose a reason for hiding this comment

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

Don't think it is great either. But if we jump out via exception here, the unitytls impl ends up in an invalid state. I'd call that worse. I guess we could maybe always "cache" the last exception and throw it when we're outside of the read/write/verify block (whenever cached exception is non zero and we registered an error in handshake/read/write). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

How do the other TLS implementations handle this?

Copy link
Author

Choose a reason for hiding this comment

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

Turns out AppleTls does exactly what I described. So I better get on with it :)

Copy link
Author

Choose a reason for hiding this comment

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

Done. Also added tests for user exception forwarding

Copy link
Author

Choose a reason for hiding this comment

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

done

namespace Mono.Unity
{
using UInt8 = Byte;
using Int8 = Byte;
Copy link
Member

Choose a reason for hiding this comment

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

You like want SByte here.

Copy link
Author

Choose a reason for hiding this comment

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

I intentionally put Byte here since what we get on the C# side from encoding and marshaling functions are always byte[]
Unitytls uses UInt8 to denote raw buffers and Int8/char for strings. Since we need to push all in and outgoing strings through Encoding it is easier to pass byte* along. The aliases here are just there to keep the semantic in the interface and make it more similar to the c original.

I guess I should add a comment explaining that!

using Int8 = Byte;

[StructLayout (LayoutKind.Sequential)]
internal struct size_t
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking, but does this size_t provide enough value over just using IntPtr?

Copy link
Author

Choose a reason for hiding this comment

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

It's mostly about the implicit cast to and from int. Since size_t is used for buffer sizes / array length, it would require quite a few explicit casts

try {
if (m_WriteBuffer == null || m_WriteBuffer.Length < bufferLen)
m_WriteBuffer = new byte[bufferLen];
Marshal.Copy ((IntPtr)data, m_WriteBuffer, 0, bufferLen);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be thread safe?

Copy link
Author

Choose a reason for hiding this comment

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

There will be always only a single WriteCallback per context. So this should be safe :)

@AndreasReich
Copy link
Author

Quite happy with the latest improvements yesterday. I ramped up test coverage on the Unity side (in platform/foundation/tls-module-monointegration) and I'm quite happy how this turns out.
unity-tls api is stabilizing as well, so if none of you has concerns we're good to go.
Until the unity sided call is in place, nothing can break anyways ;)

public static bool IsSupported()
{
try {
return NativeInterface != null;
Copy link
Member

Choose a reason for hiding this comment

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

It seems wasteful to try/catch in here, although I guess this is only called once. Any reason this can't be a little cleaner? Perhaps NativeInterface can check result of mono_unity_get_unitytls_interface() and return null if that is null rather than always doing the Marshal.PtrToStructure call and raising exception?

Copy link
Author

Choose a reason for hiding this comment

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

My idea here was that I would also catch any exception thrown from Marshal.PtrToStructure. Looking at its possible exceptions again now I realize however that this should never happen so I agree with your suggestion. Will change accordingly!

Copy link
Author

Choose a reason for hiding this comment

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

(done)

}

[DllImport("__Internal")]
private static extern IntPtr mono_unity_get_unitytls_interface();
Copy link
Member

Choose a reason for hiding this comment

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

This should likely be an internal call and connected via the normal mono icall mechanism for class libraries.

Copy link
Member

Choose a reason for hiding this comment

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

As is, we need to be sure to export this symbol across all platforms which is not always easy.

Copy link
Author

Choose a reason for hiding this comment

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

Switched it to icall: 65af3e7
Completely fresh build(.zip) from katana still passes my tests so I guess I did everything right there? :)

@AndreasReich AndreasReich merged commit cdaebd8 into unity-master Jan 17, 2018
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.

3 participants