drogue-iot / embedded-tls

An Rust TLS 1.3 implementation for embedded devices.
Apache License 2.0
168 stars 21 forks source link

Improve record encoding code #97

Closed bugadani closed 1 year ago

bugadani commented 1 year ago

This PR is one step in the direction of a write buffer. The intention is to introduce a builder that can encode records in write memory, then encrypt them if necessary, in-place. This PR moves most of the record encoding and touch-up into a single place, not counting encrypted application data building. The PR also cleans up weird and unnecessary return values.

bugadani commented 1 year ago

I'm wondering what the original design decisions were and if I'm breaking something (even possible compatibility with earlier TLS) very subtly with my refactors. I understand that I'm moving stuff around a lot, but I promise it's not completely pointless, most likely, maybe :)

bugadani commented 1 year ago

I'm not noticing anything here that would break that, could you point me to to what piece of code you're thinking about?

Code organization in general seems to have special cases (like the post-processing of the client hello) that make me think of "this must have been done for some reason I don't understand". Nothing in particular, though.

with a lot of conflicts

Sorry about that 🙈

lulf commented 1 year ago

From what I can see, this particular special case (TLS-PSK) is still supported with the change. Wrt. the code organization, I think you're the first that actually are trying to refactor and clean stuff after the initial release, so as features and improvements have took place there was no refactoring to put things in a better structure. So, thanks a lot for that effort, I can see it improves by every PR!

bugadani commented 1 year ago

From what I can see, this particular special case (TLS-PSK) is still supported with the change

I do my best trying to not delete features :)