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

Encryption support #15

Merged
merged 6 commits into from
Mar 29, 2024
Merged

Encryption support #15

merged 6 commits into from
Mar 29, 2024

Conversation

seime
Copy link
Owner

@seime seime commented Mar 10, 2024

I need some help on this one.

ESPHome device reports "invalid key".

  • Initial hello -> rsp (OK)
  • initial noise handshake -> "bad key" (ERR)

@seime seime added the help wanted Extra attention is needed label Mar 10, 2024
@seime
Copy link
Owner Author

seime commented Mar 11, 2024

@smolbo or @amandel - anyone of you that can assist? :)

@smolbo
Copy link
Contributor

smolbo commented Mar 11, 2024

Do you need review or do you need actual code help? I'm pretty deep with light handler currently (for past month+), as it is current thing I need to be kinda working. :)

@seime
Copy link
Owner Author

seime commented Mar 11, 2024

Actual code help, I've gotten blind on what the problem might be ;)

If you could try flash an ESP with an encryption key, add encryptionKey as a thing parameter and try to connect.

For me it always fails at private void handleHandshake(byte[] packetData) throws ProtocolException { in EncryptedFrameHelper. If you're able to make it past the initial if check, I can take over again and complete it.

Been stuck at it for way too long ;)

@amandel
Copy link
Contributor

amandel commented Mar 11, 2024

I do not want to promise anything but I try to have a look during this or next week.

@seime
Copy link
Owner Author

seime commented Mar 11, 2024

@smolbo

Do you need review or do you need actual code help? I'm pretty deep with light handler currently (for past month+), as it is current thing I need to be kinda working. :)

I added some initial light support a couple of weeks ago - did you test it? I only have a simple W2812B LED ring to test with, so the current support might not be very comprehensive.

@amandel
Copy link
Contributor

amandel commented Mar 26, 2024

I'm on it a bit. For me it looks like the NNpsk0 pattern is not supported via the used noise-java library.

Some links I've collected_

In noise/functions/patterns.py:49 there is handling for psk0 modifier, in https://github.com/rweather/noise-java/blob/master/src/main/java/com/southernstorm/noise/protocol/HandshakeState.java#L161 there is no such handling. Which could be reason that we get Handshake MAC failure errors.

I don't know much details - so I try to reach out to the author of the noise-java library. I did not find other alternatives for now.

@seime
Copy link
Owner Author

seime commented Mar 26, 2024

By examining the Noise java source code, I do think the variant used in the OH code NoisePSK_NN_25519_ChaChaPoly_SHA256 is identical toNoise_NNpsk0_25519_ChaChaPoly_SHA256, but just to be sure I've tried to make explicit support for Noise_NNpsk0_25519_ChaChaPoly_SHA256 in the java library. But it still doesn't work.

Thanks for looking into this @amandel !

@amandel
Copy link
Contributor

amandel commented Mar 26, 2024

I did not try to analyze the handshake fully, but the difference in the noise spec in 9.4. Pattern modifiers triggered me:

NN:
  -> e
  <- e, ee

vs. 	

NNpsk0:
  -> psk, e
  <- e, ee

do you have the branch with psk0 support somewhere? In your tests - do you think it fails even before this stage?

@seime
Copy link
Owner Author

seime commented Mar 26, 2024

Take a look at https://github.com/seime/noise-java/tree/nn_psk0

I think it fails in the first exchange. Maybe the code to init the library is insufficient, but I cannot see the difference from the other language implementations

@smolbo
Copy link
Contributor

smolbo commented Mar 26, 2024 via email

@seime
Copy link
Owner Author

seime commented Mar 26, 2024

recovering from minor surgery.

Hope you're doing ok!

@amandel
Copy link
Contributor

amandel commented Mar 26, 2024

I've reached // TODO untested branch, after that the communication stops. I had to do a lot of additional changes in your fork of noise-java which I'll check and commit as next step. Unfortunately the changes also break the tests 🤔 . Now I need a rest ;)

@seime
Copy link
Owner Author

seime commented Mar 27, 2024

Awesome @amandel !

@amandel
Copy link
Contributor

amandel commented Mar 27, 2024

I've pushed the changes to https://github.com/amandel/noise-java - this now supports Noise_NNpsk0_25519_ChaChaPoly_SHA256 - at least the test cases work.
In EncrptedFrameHelper I changed
private final static String NOISE_PROTOCOL = "NoisePSK_NN_25519_ChaChaPoly_SHA256";
to
private final static String NOISE_PROTOCOL = "Noise_NNpsk0_25519_ChaChaPoly_SHA256";

I'm still unsure, after receiving the handshake response, we try to send client_info but do not get a response back....

Edit: My commit to nn_psk0 missed some parts - fixed.

@seime
Copy link
Owner Author

seime commented Mar 27, 2024

Great work @amandel - I'm travelling ATM but I'll test this over the weekend!

@seime
Copy link
Owner Author

seime commented Mar 28, 2024

@amandel With your updated noise-java I managed to get the encrypted connection working using a Wokwi emulator.

Core remaining step: Getting the updated noise-java library published - or library source code inlined in this project. WDYT?

@amandel
Copy link
Contributor

amandel commented Mar 28, 2024

Great! This sounds cool.

I'm not sure - I did not get response from the original author to my inquiry. But I can create a PR and ask if there is interest.

For the encryption part this definitely needs some additional review - I followed the spec but with little understanding of the details.

I don't know how openHAB policies are. Might be you can include it to be not blocked until the PR gets merged. BTW: If it helps, I can create a GitHub action based build pipeline so that the binaries are at least available on GitHub. (not maven central)

@seime
Copy link
Owner Author

seime commented Mar 28, 2024

BTW: If it helps, I can create a GitHub action based build pipeline so that the binaries are at least available on GitHub. (not maven central)

Yes this would be sufficient if it is publicly available and in a stable repo.

@amandel
Copy link
Contributor

amandel commented Mar 29, 2024

On "https://maven.pkg.github.com/amandel/noise-java" there is now:

<dependency>
  <groupId>com.southerstorm</groupId>
  <artifactId>noise-java</artifactId>
  <version>1.0-ESPHOME-SNAPSHOT</version>
</dependency>

for now this is just meant as a temporary solution.

@seime
Copy link
Owner Author

seime commented Mar 29, 2024

@amandel I hit the Github auth requirement maven artifacts, so instead I just inlined the source code.

If your PR gets merged, I'll redo.

@seime seime merged commit 282ff61 into master Mar 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants