rasn icon indicating copy to clipboard operation
rasn copied to clipboard

Octet Encoding Rules

Open Nicceboy opened this issue 1 year ago • 24 comments

Resolves #129 Depends on the ~~#144 and #152~~ merged

I think rasn was quite well designed, and it made it straightforward to implement codec. Some areas might need a bit documentation, other than my questions, but I can tell about them later.

~~Currently only SET and SET-OF are missing from rasn supported types, but I guess i will finish them next week. It seems to require some thinking. When SET is finished, I will add the relevant personnel test for OER.~~ SET works with unconstrained personnel!

~~The most missing work is about improving Errors and some constraints checks on decoding side. From decoding side, I am currently mostly reusing Errors from per side.~~ Also, decoder might not note all possible "loose" OER specific variations, like many leading zeros on every case.

Would it be better to create one encoding error module and one decoding error module , which are used by all the codecs? This would reduce duplication and might make their use more efficient. I haven't put much effort for errors yet if this could be done.

There was a lot of common code when handling Sequences, with minor changes, so there is some duplication (from PER). But maybe it is better to not reuse/share them.

Another thing is, that currently I am using BigUint for length, since the ASN.1 supported max length is much more than u128, but I think it does not make sense. No hardware can handle such lengths so I might need to change it just for usize.

Any other feedback would be welcome at this point. Currently COER is basically same as OER. Canonical encoding is always used.

There are probably a lot of bugs, even while the half of the code is tests, but I will start doing more testing now. I am planning to fuzz test the decoder later, and might add CI integration as well.

Known issues

  • Enumerated type does not support custom negative values. rasn overwrites them with positive ones. (Will be resolved on https://github.com/XAMPPRocky/rasn/pull/188)
  • BitString does not support named bits. ("NamedBitList")

Nicceboy avatar Aug 27 '23 20:08 Nicceboy

Thank you for your PR! Before I review this, I noticed that there's some stuff that's in main in the changelog, would you be able to rebase this onto latest?

XAMPPRocky avatar Aug 28 '23 00:08 XAMPPRocky

It should be rebased. Those changes should be related to those two other PRs, since codec depends on them.

Nicceboy avatar Aug 28 '23 00:08 Nicceboy

Those should be merged now.

Currently COER is basically same as OER. Canonical encoding is always used.

What about basic and canonical decoding?

XAMPPRocky avatar Aug 28 '23 07:08 XAMPPRocky

What about basic and canonical decoding?

Basic decoding is default, but should be still improved.

If we want strict canonical decoding, that should be easy to add. There isn't that much difference.

I was also wondering would it make sense to add additional "strict" option to throw error if there is still data in buffer, even after everything needed was succesfully decoded.

Nicceboy avatar Aug 28 '23 07:08 Nicceboy

I was also wondering would it make sense to add additional "strict" option to throw error if there is still data in buffer, even after everything needed was succesfully decoded.

Perhaps, I know people want the opposite, because there are protocols that are stream based, so you may have a buffer which contains a message + seven bytes of the next message for example, so you need to get those leftover seven bytes to include in the next received buffer.

If we want strict canonical decoding, that should be easy to add. There isn't that much difference.

Yeah, if we're going to have a coer module, the decode should also be canonical, otherwise we should just have a oer module that has basic decode and canonical encode.

XAMPPRocky avatar Aug 28 '23 08:08 XAMPPRocky

Perhaps, I know people want the opposite, because there are protocols that are stream based, so you may have a buffer which contains a message + seven bytes of the next message for example, so you need to get those leftover seven bytes to include in the next received buffer.

Yeah, maybe I will leave it out. It can be added as option later if there is demand.

What do you think about making common error modules and type of the length determinant?

Nicceboy avatar Aug 28 '23 12:08 Nicceboy

Ehh.. I just noticed that I used BigUint for length determinant on decoding size but usize on encoding... 🤦 I guess usize is the best way,

Nicceboy avatar Aug 28 '23 18:08 Nicceboy

What do you think about making common error modules and type of the length determinant?

I have no problem with it, but I'd need to review the errors themselves.

XAMPPRocky avatar Aug 29 '23 07:08 XAMPPRocky

I can try to make a suggestion on separe PR which combines all current errors from the main for encode error/decode error modules.

Nicceboy avatar Aug 29 '23 11:08 Nicceboy

I can try to make a suggestion on separe PR which combines all current errors from the main for encode error/decode error modules.

That would be helpful, then I can review just that part, and once approved, we can just rebase this one to use those changes.

XAMPPRocky avatar Aug 29 '23 12:08 XAMPPRocky

Hey, just want to note I haven't forgotten about this PR, but since you mentioned making a separate PR for errors and the title still mentions draft I have not reviewed it, let me know how you want to continue and when I should review it.

XAMPPRocky avatar Sep 14 '23 20:09 XAMPPRocky

Hey.

I have almost done the separate PR. I had to do some other things over the past weeks. I will get back to it next week.

Nicceboy avatar Sep 15 '23 06:09 Nicceboy

No worries, there's no rush, just wanted to clarify the status 🙂

XAMPPRocky avatar Sep 15 '23 07:09 XAMPPRocky

I will have also 3 standard implementations which tests this codec further (needs 4 separate crates to implement them, as one workspace for now, due to cyclic dependencies), and I wonder should those be merged to rasn as well in some point. For now, I think I will publish them on my separate repository, but eventually I wonder what might the best practice.

Nicceboy avatar Nov 14 '23 17:11 Nicceboy

As long as they're open standards I don't see a problem with having them upstream myself. Though let's make that a separate PR.

What's the status of this PR in your opinion? Is it still a draft?

XAMPPRocky avatar Nov 14 '23 17:11 XAMPPRocky

As long as they're open standards I don't see a problem with having them upstream myself. Though let's make that a separate PR

They are open standards (about Vehicular Public Key Infrastructure). I am also trying to write a scientific publication about adaption of Rust for this ecosystem, and I think I also need to ask also you as co-author for Rust and this codec part, if you want to 😁

Also, if you want to mention (but no means necessary), that work for this OER codec was funded by European Union’s Horizon Europe Marie Skłodowska-Curie Actions (MSCA), as a part of RE-ROUTE project.

What's the status of this PR in your opinion? Is it still a draft?

It is quite ready, but I haven't fuzztested it yet properly, so there are likely issues on decoder side.

I think I will reduce the usage of bitvec for this codec, since it does not make sense to use it the current amount. As the name says, it is octet encoding rules. There will be likely less cloning and other performance benefits for just using Vec<u8> as much as possible. I make that change at first.

Nicceboy avatar Nov 14 '23 18:11 Nicceboy

Also, if you want to mention (but no means necessary), that work for this OER codec was funded by European Union’s Horizon Europe Marie Skłodowska-Curie Actions (MSCA), as a part of RE-ROUTE project.

That's very cool, I definitely will, I was thinking of doing a round-up release post of the past while.

I am also trying to write a scientific publication about adaption of Rust for this ecosystem, and I think I also need to ask also you as co-author for Rust and this codec part, if you want to 😁

I would be honoured to 🙂

XAMPPRocky avatar Nov 14 '23 20:11 XAMPPRocky

Also, if you want to mention (but no means necessary), that work for this OER codec was funded by European Union’s Horizon Europe Marie Skłodowska-Curie Actions (MSCA), as a part of RE-ROUTE project.

That's very cool, I definitely will, I was thinking of doing a round-up release post of the past while.

I am also trying to write a scientific publication about adaption of Rust for this ecosystem, and I think I also need to ask also you as co-author for Rust and this codec part, if you want to 😁

I would be honoured to 🙂

Cool 😎 I don't have the exact timeline yet, but if you can send your email to email address in my profile, I can contact when I am closer to draft version and tell you a bit more, and you can still decline afterwards.

Nicceboy avatar Nov 15 '23 18:11 Nicceboy

Just an update, I haven't forgot this PR yet 😄 I have been busy on other things. This should be overall quite completed already, I just need to work on fuzzing side a bit more. I have tried to implement overall ASN.1 type bindings for fuzzing which can be used for other codecs too.

Nicceboy avatar Jan 31 '24 18:01 Nicceboy

Sounds good, but if all that's left is fuzzing, would you be okay with making ready to review now? We can add fuzzing with fixes in follow-up prs

XAMPPRocky avatar Jan 31 '24 20:01 XAMPPRocky

I will double check the code and maybe it can be reviewed then. I also need to move WIP fuzzing code into different branch.

Nicceboy avatar Jan 31 '24 21:01 Nicceboy

@Nicceboy Friendly ping 🙂

XAMPPRocky avatar Mar 12 '24 17:03 XAMPPRocky

@Nicceboy Friendly ping 🙂

I have very high hopes that I can get back to this even in this week!

Nicceboy avatar Mar 12 '24 17:03 Nicceboy

@XAMPPRocky I guess it is finally time to review this 😁 Fuzzing is not completed, so there are probably still quite some bugs. But at this point, might be good for someone else to review and see if there are any other major issues.

Nicceboy avatar Apr 14 '24 03:04 Nicceboy

Going to merge this now, and we can improve it in followup PRs.

Thank you for your PR! 🎉

XAMPPRocky avatar May 08 '24 09:05 XAMPPRocky

Thanks! :tada: Was it okay, if I make a PR about adding mention of EU funding for the bottom of sponsorship section, about the codec?

Nicceboy avatar May 08 '24 19:05 Nicceboy