librasn / rasn

A Safe #[no_std] ASN.1 Codec Framework
Other
190 stars 45 forks source link

C/OER roadmap #129

Closed Nicceboy closed 3 months ago

Nicceboy commented 1 year ago

Hi,

I was planning to add the OER support, and I think I will start working on it quite soon. All development tips for the process are welcome!

XAMPPRocky commented 1 year ago

Thank you for working on this. I'd also like to collect progress and all questions here, and then from that we can generate a "how to write a codec" document that we can include in the documentation.

kurkpitaine commented 1 year ago

Hi,

I was planning to add the OER support, and I think I will start working on it quite soon.

All development tips for the process are welcome!

Hi, Need a no_std COER impl here, willing to help. I started some parts of OER on another crate but nothing working yet. I can test against a COTS C implementation if needed.

Nicceboy commented 1 year ago

Hi, Need a no_std COER impl here, willing to help. I started some parts of OER on another crate but nothing working yet. I can test against a COTS C implementation if needed.

That would be nice. I have been developing against Python asn1tools and and C asn1c codecs. I will later benchmark against rasn implementations of the ETSI TS 103 097, ETSI TS 102 941 and partial IEEE 1609.2, which I make public later on.

OER support is still in early phase. I will update the first comment later on. Currently only bool and Integer encoder works, but my plan is to finish this ASAP.

XAMPPRocky commented 1 year ago

@Nicceboy I would also recommend making a draft PR to rasn that adds it, that way people can test it and make PRs to it.

Nicceboy commented 1 year ago

@Nicceboy I would also recommend making a draft PR to rasn that adds it, that way people can test it and make PRs to it.

Sure, I would like to have a bit more stable functionality before that.

One question; currently on lib.rs, #![no_std] is commented. If it is uncommented, many types are not found in different places, since no fully type annotations have been used.

Should we fix this?

XAMPPRocky commented 1 year ago

Sure, I would like to have a bit more stable functionality before that.

I wouldn't worry about stable, it's supposed to be a draft after all. Better to open it now when it doesn't work and for you to get busy later and let another person pick up the work, than for you to get busy and then have to continue the PR with no progress. I know you think it'll be done soon, but my experience is that there's always little things that get in the way of getting it done as fast as you want, and life will get in the way at some point.


I'll fix the commented out no_std now.

Nicceboy commented 1 year ago

One worrying thing currently is the dependency of bitvec crate. The future of the project seems uncertain and I noticed the marked issues. I wonder if I should use just alloc vectors instead just in case. It adds some extra work, but might be possible. There are currently some functions in U/PER which can be directly used with PER otherwise.

Another thing is, that should we consider refactoring to use common functions between these two codecs on some cases, or should we keep them fully separate?

XAMPPRocky commented 1 year ago

One worrying thing currently is the dependency of bitvec crate. The future of the project seems uncertain and I noticed the marked issues. I wonder if I should use just alloc vectors instead just in case. It adds some extra work, but might be possible.

For now I would continue with the using bitvec, I have no intention right now to switch from it. It would be a lot of extra work for little benefit.

Another thing is, that should we consider refactoring to use common functions between these two codecs on some cases, or should we keep them fully separate?

Do you mean between PER and OER?

Nicceboy commented 1 year ago

For now I would continue with the using bitvec, I have no intention right now to switch from it. It would be a lot of extra work for little benefit.

My main concern is the amount of unsafe code in bitvec and if nobody is actively patching the issues, it might take too much time to get something critical fixed. I did not want to replace the existing code, but maybe creating the new code for OER codec without using bitvec. But I know it will add extra work, but from safety perspective it might be worth it.

Do you mean between PER and OER?

There will be some duplicate code otherwise I think. But maybe we can see that later how much there actually will be.

XAMPPRocky commented 1 year ago

My main concern is the amount of unsafe code in bitvec and if nobody is actively patching the issues, it might take too much time to get something critical fixed. I did not want to replace the existing code, but maybe creating the new code for OER codec without using bitvec. But I know it will add extra work, but from safety perspective it might be worth it.

I understand that concern, however I think that's acting too soon, it's only been a few months, and if bitvec becomes unmaintained there will likely be a community fork for it. There are a lot larger crates that depend on bitvec, and there's strength in numbers, there's no guarentee that what we would write would be safer or more performant.

Until we get a more definitive deprecation or CVE we should continue to use it. Rain is currently going through a security audit so if there is anything actually concerning, it should hopefully be caught by the auditors.

Nicceboy commented 1 year ago

Maybe you are right, I will use the bitvec for now.

Here is the comment (README.md) of the author about the situation: https://github.com/myrrlyn

Nicceboy commented 1 year ago

Seems like negative discriminant values for custom enumerated values are currently unsupported. OER encoding makes possible to also encode/decode these, and ASN.1 itself does not forbid these.

Only positive values are matched and negative values are dropped: https://github.com/XAMPPRocky/rasn/blob/e64391c49c34efdd3a342df81976a56cdfb487d2/macros/src/config.rs#L516

Might not be a big priority to change, but writing it here.

Nicceboy commented 1 year ago

Is it possible to declare BitString type as "NamedBitList"? Seems like encoding can depend on that in the case of OER. In my understanding it might not be currently possible.

XAMPPRocky commented 1 year ago

Is it possible to declare BitString type as "NamedBitList"? Seems like encoding can depend on that in the case of OER. In my understanding it might not be currently possible.

I don't believe so, no. We'd to be able to define the named values of the type at const time, which isn't currently possible for BitString.

Nicceboy commented 1 year ago

I have a problem with interpreting the standard if this is a problem for OER...

I might need to set "NamedBitList" as default behavior if it is. I don't have statistics in my hand, but usually in ASN.1 specifications they are almost always named.

I might then need to provide some option for codec, so custom override is possible for the defined type.

XAMPPRocky commented 1 year ago

I might need to set "NamedBitList" as default behavior if it is. I don't have statistics in my hand, but usually in ASN.1 specifications they are almost always named.

I think the easiest to do for now is to say "we don't support NamedBitList bit strings", so you'd only implement bit strings without NamedBitLists, leave a TODO in the code for it, and then once the rest of it is done, we can revisit this and try to build a type that would work for it, I believe PER also has different behaviour for those bit strings, so we'll need to update that codec as well once it is supported.

Nicceboy commented 12 months ago

One thing which took me a lot of time how to figure out, was how the field_bitfield actually worked in UPER.

On every trait method, there was call for self.set_bit(tag, true)?;. When I initially started doing the codec, I just thought it is related to the encoding, and left it out.

But apparently it tracks the presence of value in Sequence/Set. I was wondering how UPER got this automatically, and had to debug quite some time.

Nicceboy commented 12 months ago

Am I understanding correctly that

A ::= SEQUENCE {
    a BOOLEAN,
    ...
} 

Is identical to this?

#[derive(AsnType, Clone, Debug, Decode, Encode, PartialEq)]
#[non_exhaustive]
struct Sequence1 {
    a: bool,
}

Does #[non_exhaustive] always means ellipsis in the end?

XAMPPRocky commented 12 months ago

Does #[non_exhaustive] always means ellipsis in the end?

Yes.

Nicceboy commented 12 months ago

When encoding SET, did you try any other way than storing the output to another variable (set_output)?

I think I have finished everything but SET, but now it seems that I might redo the length determinant.

I was wondering would it be manually possible to adjust the order of general encoder, when it encodes the fields from the struct, so the total output is already correct.

XAMPPRocky commented 12 months ago

I was wondering would it be manually possible to adjust the order of general encoder, when it encodes the fields from the struct, so the total output is already correct.

Do you mean the derive implementation? That wouldn't help because you can't rely on the caller using the derive implementation.

While it adds an allocation having it in a separate field it means that we can guarantee that sets are always encoded correctly, even if people swap the order of fields.

Maybe we can implement an optimisation where it only moves to a separate allocation if you call it out of order, but I think for the initial implementation we should take the naive approach that we know will be correct, and then follow up with an optimisation.

Separately we need to add some profiling infrastructure so we can know if these kinds of optimisations provide tangible benefits.

Nicceboy commented 12 months ago

Do you mean the derive implementation? That wouldn't help because you can't rely on the caller using the derive implementation.

I was thinking that would it be possible to call function like this earlier, and use it to change order:

https://github.com/XAMPPRocky/rasn/blob/9dc64bd1d0806624b74eea69e052338cf445260b/src/per/enc.rs#L76-L87

Since on compile time it is known that struct is set and which tags are present, and which codec is being used.

But marcos are still all Greek to me, so I don't have actually no idea.

XAMPPRocky commented 12 months ago

But marcos are still all Greek to me, so I don't have actually no idea.

Right but even though it's guaranteed at compile time by the macro, the caller doesn't know that the order of fields are correct, because someone might not use the macro. We'd need a way in the type system to have an assert that guarantees the order.

Nicceboy commented 12 months ago

I see. I will use the similar method than in PER.

Nicceboy commented 11 months ago

Okay, SET seems to work now, also with personnel test!

I wasn't that big change after all, while the code looks a bit messier now.