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

actual linux/non windows support? #9

Open
mitchcapper opened this issue Jul 23, 2017 · 13 comments
Open

actual linux/non windows support? #9

mitchcapper opened this issue Jul 23, 2017 · 13 comments

Comments

@mitchcapper
Copy link
Contributor

So using this in windows requires you adding the "textsecure-servicewhispersystemsorg.crt" certificate to "Trusted people" either at runtime (as the UWP app does) or manually otherwise.

The problem is there is no "Trusted People" equivalent in linux. You can add CA's to the locally approved list but "textsecure-servicewhispersystemsorg.crt" has no public CA certificate that signed it (as far as I can tell) that one could import if they wanted to.

dotnet/corefx#12038 addresses custom validation for ClientWebSocket but who knows how far out that is.

Open if anyone has additional thoughts on working around this issue to allow this code to work on other platforms:) The most obvious I see is moving to a 3rd party web socket library like https://github.com/sta/websocket-sharp that supports server certificate validation.

@Trolldemorted
Copy link
Contributor

Does the ServicePointManager's ServerCertificateValidationCallback work in dotnet core apps/wine/whereever?

Iirc i originally wanted to use that, but it didn't work on uwp because uwp uses different socket implementations under the hood.

@mitchcapper
Copy link
Contributor Author

@Trolldemorted
Correct that never has existed in .net core. The solution for httpClient is to init a HttpClientHandler and specify the callback handler there. It is more elegant than a global one as well. The problem is ClientWebSocket does not yet have anything similar. That is what dotnet/corefx#12038 addresses.

@Trolldemorted
Copy link
Contributor

Damn. So there is no official workaround for custom certificates until ManagedWebSockets arrive?

Can we alter .net cores certificate validation with reflection?

@mitchcapper
Copy link
Contributor Author

As far as I understand it there is no known work around and the WebSocket code is native (so just copying the .net core code would not work). In addition it seems officially it will not be making it in the next revision either.

Reflection may be a hacky option but im not sure would be best (and not sure of the reduced .net standard options for UWP compatability).

Moving to a 3rd party web socket library however could resolve this.

@Trolldemorted
Copy link
Contributor

I found a stackoverflow answer where someone made his own ClientWebSocket to send cookies when opening the connection. It uses a HttpWebRequest to establish a connection, which has a ServerCertificateValidationCallback that can be used to manually accept/reject certificates.

Now the next problem is, when you use the ServerCertificateValidationCallback in an UWP app, you get a PlatformNotSupportedException. We could catch and silently ignore this on UWP though (since UWP can still use the TrustedPeople workaround), and it should™ work on all other platforms as far as i know.

I am visiting my family and having exams this and the next week, but after that i will have a look.

@mitchcapper
Copy link
Contributor Author

I would agree with the proposed solution. Technically the work around of using the WinRT client instead is doable with a few #defines depending on the platform is doable. Given the existing work around for UWP of using the trusted people would simplify the code base to just use the native .net HTTPClient lib.

@mitchcapper
Copy link
Contributor Author

@Trolldemorted plans on this? My primary target is non-windows so cannot use the lib with this blocker.

@Trolldemorted
Copy link
Contributor

Sorry, i have an important exam in 2 weeks and thus been very busy.

Could you test https://github.com/signal-csharp/libsignal-service-dotnet/tree/certificates and see if the PushServiceSocket works?

mitchcapper added a commit to mitchcapper/libsignal-service-dotnet that referenced this issue Apr 23, 2018
mitchcapper added a commit to mitchcapper/libsignal-service-dotnet that referenced this issue May 29, 2018
@mitchcapper
Copy link
Contributor Author

Per #28 if you want I can create the default handlers for this, update the package to target netapp2.1 and 1.4 or we can leave it without default handlers and just have users write their own. I just don't want to submit a PR that has to be rebased as it just sits if you don't want it.

@Trolldemorted
Copy link
Contributor

Not sure if multitargeting is worth the hassle, but we could include a sample impl in the documentation (some documentation would be great anyway, now that I think of it). It could get outdated if we make changes to the interface and forget to update the doc, but I really hope we don't have to touch it every again.

Our previous ws impl automatically reconnected, do we want to maintain that behaviour? If so, addressing Turasa/libsignal-service-java#16 becomes more important.

@mitchcapper
Copy link
Contributor Author

I would assume we want to reconnect still.
I took an initial wack at this and have it working for .net core. It is a good bit of code and I am not sure where I would suggest a PR for it (as it doesn't belong in the windows lib either). We could add it here just commented out (or consider retargetting). If someone had to do it from scratch would be somewhat annoying to figure out (and required for .net core 2.1 and higher per above).
https://gist.github.com/mitchcapper/1a40fd1b008f60aaf7c27ffa48dd2432

It would be less of an issue maintaining parity (or for someone to do their own .net core version) but the windows library switched to using MessageWebSocket which there is no .net standard version or library for.

@mitchcapper
Copy link
Contributor Author

That gist also requires #31 I could just duplicate the cert (or make it an arg to the constructor like I had before) but this seemed cleaner than two copies.

@mitchcapper
Copy link
Contributor Author

As it is related to #34 lets discuss the overlap here.

tldr: We can avoid deadlock, personally I prefer the library code safely Waiting on a Task then relying on the user to properly Wait on their own tasks but the most straight forward approach is to leave it not to use Tasks anywhere (and just taskify it later if/when it switches to something async). Let me know your preference I can update.

full details:

Won't this deadlock if you call decrypt on a task with a synchronization context?

So it is a double edged sword, a user who calls it with an incorrect wait on UI would block but the callback would also happen on the UI thread for those who do not (potentially nice). The far safer approach is to use Task.Run to in decrypt to run in another context (or a fake delay/etc with ContinueWith(false) to get us out of that context).

Do we really need the asynchronity? I think it doesn't make that much sense that deep in the stack, especially with the locks around.

I don't think Decrypt calls have a benefit currently being Task based in nature (given the fact they are not actually async due to the locks). I do think there is benefit to DecryptionCallback being Task based. For one, we are already going to break clients by adding another arg, so might as well change the return signature too. If someone is using the callback support to make sure the message is written safely before it is acked on the server however this may include async operations. In fact the biggest reason I see to use the callback is for potential unreliable mechanisms that async is a good fit for.

Granted we could leave it as is, not have DecryptionCallback take tasks and the user can just .Wait their own async calls.
Making this change would do some future proofing as well so that if we switch to a async based locking mechanism (say semephores) it could be moved to be async quite easily.

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

No branches or pull requests

2 participants