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

Protocol Updates #83

Merged

Conversation

QuantumEntangledAndy
Copy link
Collaborator

@QuantumEntangledAndy QuantumEntangledAndy commented Oct 15, 2020

This contains updates to the protocol and is set as a draft for now as I think it should be discussed. In the PR #80 we analysed the messagse in more detail and discovered that xmls of the form.

<body>
...
</body>

Always went after in the binary block offset

And that xmls of the form

<Extension>
...
</Extension>

Always went before the binary block offset.

I believe this means that we were serde'ing incorrectly. It only worked before because neolink only ever sent Extension xml where it correctly set binary_offset to the message length. We also only every received either

  • Body xml: in which case it was assumed that the 0 in the binary_offset was a mistake (this is not the case),
  • Extentsion + Binary in which case we had special logic to parse it

This applies appropriate changes so that extension xml is always before the binary_offset which is renamed to payload offset, and that body xml and binary payloads are after the payload_offset

This simplifies the serde so we don't need to keep in_bin_mode in context which was a nice bonus

This also include the changes requred to send command and control messages to neolink that was required for the MQTT PR #78

P.s. Also applied some things to the readme: formatting, fixed some of my spelling mistakes and closed an unclosed backtick.

@QuantumEntangledAndy
Copy link
Collaborator Author

QuantumEntangledAndy commented Oct 17, 2020

Things to add/added

  • Rework message structure

  • Handle unencrypted/encrypted flag correctly

  • Add message handle

  • Use message handle in get stream (2 streams one connection)

Co-Authored-By: twistedddx <bits@live.com>
Conflicts:
	dissector/baichuan.lua
	src/bc_protocol.rs
	src/bc_protocol/media_packet.rs
Co-Authored-By: twistedddx <bits@live.com>
Using u8 for channel id throughtout
Using simpler map_err

Co-Authored-By: twistedddx <bits@live.com>
Co-Authored-By: twistedddx <bits@live.com>
@QuantumEntangledAndy
Copy link
Collaborator Author

QuantumEntangledAndy commented Apr 17, 2021

Now with added AES encryption protocol. See #149

@thirtythreeforty
Copy link
Owner

Is this ready to go? I had been mostly ignoring it while it was marked draft.

@QuantumEntangledAndy
Copy link
Collaborator Author

Yes it should be now. The only thing I might want to add is getting SD and HD streams together. Alot of checks are failing though need to see why

@QuantumEntangledAndy QuantumEntangledAndy marked this pull request as ready for review April 17, 2021 06:05
@QuantumEntangledAndy
Copy link
Collaborator Author

Ahh we need openssl now in the docker images

@thirtythreeforty
Copy link
Owner

thirtythreeforty commented Apr 17, 2021

Checks are failing because of the moving target that is GitHub runners... EDIT: ah you're right

If you wouldn't mind separating out HD+SD, that would be appreciated. This is taking a minute to digest :)

@thirtythreeforty
Copy link
Owner

Want to grab a pure Rust AES implementation? I had tentatively selected block-cipher.

@QuantumEntangledAndy
Copy link
Collaborator Author

Yes that was it all is fine again.. Sigh that was embarassing

@QuantumEntangledAndy
Copy link
Collaborator Author

Ok changes have been made but I want to try changing to pure rust AES. If you want to have a go at that please do else I'll work on it tomorrow

@thirtythreeforty
Copy link
Owner

thirtythreeforty commented Apr 17, 2021

Done changing to pure Rust. CI seems happy. See my branch.

I checked performance on a lark and the debug build now uses a lot more CPU (10% of my 5-year-old i5). But the release build is < 1%, so it's fine.

@QuantumEntangledAndy
Copy link
Collaborator Author

QuantumEntangledAndy commented Apr 18, 2021

Ok so it looks good now and all the checks pass except clippy. In the case of clippy it is because of issues with the lexical-core package (a dependency of nom). Bumping up nom's version may solve this.

@QuantumEntangledAndy
Copy link
Collaborator Author

Everything should now pass and I've even updated the tests. Please review when you can

Copy link
Owner

@thirtythreeforty thirtythreeforty left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for all the work.

@thirtythreeforty thirtythreeforty merged commit 5a62bb1 into thirtythreeforty:master Apr 18, 2021
@QuantumEntangledAndy
Copy link
Collaborator Author

Awesome. Next job is SD and HD in the same login.

@QuantumEntangledAndy
Copy link
Collaborator Author

Any comments on subscribing on msg number over message id

@thirtythreeforty
Copy link
Owner

thirtythreeforty commented Apr 18, 2021 via email

@QuantumEntangledAndy
Copy link
Collaborator Author

QuantumEntangledAndy commented Apr 18, 2021

I had a little look into the service as part of #66. I came up with two solutions. Either nssm which we would need to package the binary with neolink or windows-service-rs which would allow us to run neolink as the sevice Daemon itself at the cost of extra code and maintenance.

Maybe tackle an MSI too so we can close #1 too. If you do want to tackle the MSI maybe have a look at #66 as that has a go at it.

@QuantumEntangledAndy
Copy link
Collaborator Author

@twistedddx The current master version should now be able to handle unencrypted connections for the older cameras. Could you maybe try it when you get the chance?

@QuantumEntangledAndy QuantumEntangledAndy deleted the protocol_updates branch April 18, 2021 05:09
@twistedddx
Copy link
Contributor

Neolink now works great with the older cams using unencrypted XML.

@QuantumEntangledAndy
Copy link
Collaborator Author

Thanks for testing. I'm glad we can close this one.

entropin pushed a commit to entropin/neolink that referenced this pull request Aug 18, 2021
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.

None yet

3 participants