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

CryptoFormatter #1

Open
HavenDV opened this issue Jan 9, 2020 · 34 comments
Open

CryptoFormatter #1

HavenDV opened this issue Jan 9, 2020 · 34 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@HavenDV
Copy link
Owner

HavenDV commented Jan 9, 2020

A wrapper for other formatter that encrypts/decrypts data

At the moment, my knowledge is enough only to implement the simplest XOR encryption, which makes little sense

@HavenDV HavenDV added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 9, 2020
@zbalkan
Copy link
Contributor

zbalkan commented Dec 19, 2021

I use some encryption with my software to encrypt license keys. I am working on using the same encryption methods to encrypt every message. If I manage to make it work stable, I'll create a PR.

@zbalkan zbalkan mentioned this issue Dec 19, 2021
@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

I have merged your code and will adapt it a little according to my vision of the project. I need a little explanation about KeyPair.ValidatePublicKey:
The current implementation does not refer to class fields and can be a static method. Or you have an error, because the out variable and the publicKey field have the same name, and it is the out variable that is visible inside the method.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 20, 2021

Thank you for the review. I will update my repo and work on that part.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

Please wait a little. I have already included your code in the main branch, I will change it a little and you will just provide a new PR on top of the master branch.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

I rewrote your code a bit, simplifying the end-use to:

await using var server = new PipeServer<MyMessage>(pipeName, formatter: new InfernoFormatter(new SystemTextJsonFormatter()));
server.EnableEncryption();

await using var client = new PipeClient<MyMessage>(pipeName, formatter: new InfernoFormatter(new SystemTextJsonFormatter()));
client.EnableEncryption();

Please also study the code https://github.com/HavenDV/H.Pipes/tree/master/src/libs/H.Formatters.Inferno for errors in encryption.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

There is still a potential problem here:
If the client sends messages immediately after ConnectAsync, then the message may arrive unencrypted.
So far, I see a solution to this problem in changing the InfernoFormatter to IAsyncFormatter and waiting for the key via while (Key == null) await Task.Delay;

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

There is also a problem when the server has more than 1 connection, since the Formatter is common for all connections.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 20, 2021

My take was if the sent message is not a public key, ignore it. If it's a public key, then use it to calculate shared key. Afterwards, go on encrypted. But it's better to handle it in the library, you are right.

In the Validation, I'll add a try catch and keep the length as a check step. I plan to ensure the sent message is a valid public key by using either dotnet core Cryptography library or Inferno. Since try-catch is costly, the precheck of length is still a valid step.

And I think, client can send its public key only after it gets the server public key, or vice versa.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

I have changed the way for key exchange. Now the client and the server create a separate byte[] channel for this using the SingleConnection server and client.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 20, 2021

There is also a problem when the server has more than 1 connection, since the Formatter is common for all connections.

You are absolutely right. An instance of format is needed for each connection. I only created one KeyPair for each, but a Formatter is also needed for each. A dictionary might not suffice, I believe, there might be another class with Client Id, KeyPair and Formatter. There may be more ways to improve the quality.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

I have simplified the end-use to:

await using var server = new PipeServer<MyMessage>(pipeName, formatter: new SystemTextJsonFormatter());
server.EnableEncryption();

await using var client = new PipeClient<MyMessage>(pipeName, formatter: new SystemTextJsonFormatter());
client.EnableEncryption();

solving the problem with multiple connections at the same time - I just create an InfernoFormatter for each connection after I get the key.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

I added PipeConnectionExtensions.WaitExchangeAsync. It applies to any connection and will wait for the InfernoFormatter to be active.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 20, 2021

I am going to sleep. It will be great if you test this and provide feedback. Thank you again for the your work and the provided PRs

@zbalkan
Copy link
Contributor

zbalkan commented Dec 20, 2021

I'll test it this evening.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 20, 2021

There's a Heisenbug. It happens when not debugging but when you search for it, it does not occur.

Currently there are two errors I could see but could not diagnose. First one is about serialization / deserialization. There's a byte array sent to a JSON converter, that I could not spot. The second is premature cancelation of a task.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

The current challenge is to recreate the problems in the tests so that they can be fixed.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

The first one is probably this:

var client = new SingleConnectionPipeClient<byte[]>(pipeName, args.Connection.ServerName, formatter: args.Connection.Formatter);

Or this:
var response = await client.WaitMessageAsync(cancellationToken: cancellationToken).ConfigureAwait(false);

It sends a byte array but uses Formatter. Byte array cannot be (de)serialized as json. The exception was something like "0x17 is not a valid JSON".

And the cancelation is caused by the same thing because here it has 10 seconds to communicate but could not succeed for reasons.

using var source = new CancellationTokenSource(TimeSpan.FromSeconds(10));

Since the other one did not succeed for non-alphanumeric characters in byte array that could not be (de)serialized, this line will wait and probably cause the timeout.

await Task.Delay(TimeSpan.FromMilliseconds(1), cancellationToken).ConfigureAwait(false);

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

The timeout is needed to know about problems, instead of the method just waiting forever.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

I am using serialization byte[] to json in tests. This works correctly for Newtonsoft.Json/System.Json.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

I can use BinaryFormatter, but wouldn't that be another problem?

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

WaitExchangeAsync will wait forever unless you pass a CancellationToken. I can add an extra timeout there if needed.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

I am using serialization byte[] to json in tests. This works correctly for Newtonsoft.Json/System.Json.

It's probably the byte[] here has unreadable characters. I used a simple LINQ call chain that casts each byte as char and concatenate as a string. But Inferno has its own method for this, converting it to a Base16, Base32 or Base64 string. Probably, that's for the best and allow a safe JSON conversion.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

WaitExchangeAsync will wait forever unless you pass a CancellationToken. I can add an extra timeout there if needed.

Since the attempt to send a public key fails, it will always hit the timeout. So yes, having a timeout here is the correct thing. Without it, it would be hard to understand the problem and also it would hang forever.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

I have another question. Think of TCP handshake. Would it be better to have this key share like that?

  1. Client sends the public key, waits until it gets a public key. Ignores any other message. Client shuts down if no response is received.
  2. When server receives a public key, it sends its newly created public key for that pipe. If no valid public key received in a timely manner, disposes the pipe.
  3. When both successfully shared, then they start communicating.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

It's probably the byte[] here has unreadable characters. I used a simple LINQ call chain that casts each byte as char and concatenate as a string. But Inferno has its own method for this, converting it to a Base16, Base32 or Base64 string. Probably, that's for the best and allow a safe JSON conversion.

Can you grab the data that is throwing the exception?
I suspect that this is still a problem with the Formatter you are using - MessagePack

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

byte[] serialization should not depend on content in any way

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

I have another question. Think of TCP handshake. Would it be better to have this key share like that?

  1. Client sends the public key, waits until it gets a public key. Ignores any other message. Client shuts down if no response is received.
  2. When server receives a public key, it sends its newly created public key for that pipe. If no valid public key received in a timely manner, disposes the pipe.
  3. When both successfully shared, then they start communicating.

Yes, I think we need to add Disconnect if an error occurs or timed out while retrieving the key. Both client-side and non-server-side.

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

It's probably the byte[] here has unreadable characters. I used a simple LINQ call chain that casts each byte as char and concatenate as a string. But Inferno has its own method for this, converting it to a Base16, Base32 or Base64 string. Probably, that's for the best and allow a safe JSON conversion.

Can you grab the data that is throwing the exception? I suspect that this is still a problem with the Formatter you are using - MessagePack

Well, I only used the sample project you created. I did not want to make it more complicated.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 21, 2021

I cannot repeat the behavior you are talking about. Perhaps you have sent special characters or are you using not the most recent code from the master branch?

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

Last night, I rebased my repo with the - then- latest state of the repo. It happened twice. But when I tried to debug it, Iit did not reoccur. I'll rebase with the latest version and try again. But for now, it seems it's OK. If I manage to find it, I'll create a new issue with steps to reproduce. If it cannot be reproduced, then I'll assume it does not exist 👍

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

Hi,

I managed to find it. Here are the steps to reproduce:

  1. Start an instance and type server.
  2. Start an instance and type client.
  3. Start another instance and type client.
  4. Voila! Get an exception: Exception: System.Text.Json.JsonException: '...' is an invalid start of a value.
Enter mode('server' or 'client'):
client
Running in CLIENT mode. PipeName: named_pipe_test_server
Enter 'q' to exit
Client connecting...
Connected to server
Exception: System.Text.Json.JsonException: '0xEF' is an invalid start of a value. Path: $ | LineNumber: 0 | BytePositionInLine: 0.
 ---> System.Text.Json.JsonReaderException: '0xEF' is an invalid start of a value. LineNumber: 0 | BytePositionInLine: 0.
   at System.Text.Json.ThrowHelper.ThrowJsonReaderException(Utf8JsonReader& json, ExceptionResource resource, Byte nextByte, ReadOnlySpan`1 bytes)
   at System.Text.Json.Utf8JsonReader.ConsumeValue(Byte marker)
   at System.Text.Json.Utf8JsonReader.ReadFirstToken(Byte first)
   at System.Text.Json.Utf8JsonReader.ReadSingleSegment()
   at System.Text.Json.Utf8JsonReader.Read()
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   --- End of inner exception stack trace ---
   at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, JsonReaderException ex)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at H.Formatters.SystemTextJsonFormatter.DeserializeInternal[T](Byte[] bytes) in D:\Repos\H.Pipes\src\libs\H.Formatters.System.Text.Json\SystemTextJsonFormatter.cs:line 27
   at H.Formatters.FormatterBase.Deserialize[T](Byte[] bytes) in D:\Repos\H.Pipes\src\libs\H.Formatters\FormatterBase.cs:line 39
   at H.Pipes.PipeConnection`1.<Start>b__37_0(CancellationToken cancellationToken) in D:\Repos\H.Pipes\src\libs\H.Pipes\PipeConnection.cs:line 124

@zbalkan
Copy link
Contributor

zbalkan commented Dec 21, 2021

I believe some unit tests are needed for this library but I cannot decide what is a unit.

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 22, 2021

image

@HavenDV
Copy link
Owner Author

HavenDV commented Dec 22, 2021

In addition, as I understand it, the exception occurs after the keys are received and the encrypted connection is established.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants