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

JER BIT STRING quoted hex #133

Merged
merged 2 commits into from
Oct 6, 2023
Merged

JER BIT STRING quoted hex #133

merged 2 commits into from
Oct 6, 2023

Conversation

v0-e
Copy link

@v0-e v0-e commented Jul 24, 2023

JER BIT STRING currently encodes to:

  • an unquoted JSON string;
  • and a binary string, instead of a hex string (as per X.697).

This PR fixes both of these issues.

Current limitation: this contribution assumes a BIT STRING of fixed size. X.697 provides a variant encoding for BIT STRING of variable size, however, from my understanding, the function in question (BIT_STRING_encode_jer()) may not have enough context to know if the BIT STRING element is constrained in size or not.

@mouse07410
Copy link
Owner

My apologies - by now I managed to forget what the intended effect of this PR would be, compared with the current behavior. Could you please refresh my memory, and maybe give a couple of examples?

@v0-e
Copy link
Author

v0-e commented Sep 6, 2023

Sorry about that, I could have added more details.
Consider the following definition:

Test ::= SEQUENCE {
    number INTEGER,
    bsFixed BIT STRING (SIZE(16)),
    bsVar BIT STRING
}

With number=122 and bsFixed=bsVar=1001111001000100, this currently encodes to:

{
"Test":{

    "number": 122,
    "bsFixed":
        1001111001000100
    ,
    "bsVar":
        1001111001000100
    }
}

However, X.697 (Section 24 - Encoding of bitstring values) mandates to use an hex encoding for the BIT STRING.
Moreover, if you set any of these BIT STRING fields to 000... the final field encoding will be something like "bsFixed":00000, which is an invalid JSON according to RFC 8259.

This PR fixes both of these issues. The encoding of the above becomes:

{
"Test":{

    "number": 122,
    "bsFixed": "9e44"
    ,
    "bsVar": "9e44"
    }
}

A bit awkward with the displaced comma, but still a valid JSON while partially following X.697.
I say partially because both BIT STRING fields are encoded in the same way while one's length is fixed and the other is variable.

I am unsure if the function responsible for encoding a JER BIT STRING (BIT_STRING_encode_jer()) is aware if the field is constrained in size or not to implement the above distinction.

Let me know if there is anything I can do to improve this PR.

@kentkost
Copy link

kentkost commented Sep 7, 2023

Pretty sure that this provides a more precise implementation of x.697. But I am not sure if using sprintf is the correct fastest way to go converting binary to hex.
If both of you don't mind. Then I would like to test the behavior this coming weekend.
I'll also try to add some tests.

@v0-e
Copy link
Author

v0-e commented Sep 7, 2023

Indeed, sprintf is not the best approach. I'll try to change it before the weekend.

@v0-e
Copy link
Author

v0-e commented Sep 7, 2023

I've switched to the lookup approach instead of using sprintf. I've also removed code related to adding line breaks in the middle of the encoded BIT STRING.

Looks like the encoding of a JER OCTET STRING for large enough lengths also produces an invalid JSON due to the line breaks. For example,

Test ::= SEQUENCE {
    os OCTET STRING
}

with os=000... 32 bytes in length, encodes to:

{
"Test":{

    "os":
        00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
        "00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00"
    }
}

Not sure if this PR can be used to fix this or should a new one be created.

Copy link

@kentkost kentkost left a comment

Choose a reason for hiding this comment

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

Looks alright to me.

This line though seems a bit odd to me. What's the reason for this roundabout way of creating an array with a size of 52? And why was it reduced from 128?

char scratch[16 * 3 + 4];

@mouse07410
Copy link
Owner

Not sure if this PR can be used to fix this or should a new one be created.

A separate PR, please. One issue - one PR. ;-)

Thanks!

@mouse07410
Copy link
Owner

Looks alright to me.

This line though seems a bit odd to me. What's the reason for this roundabout way of creating an array with a size of 52? And why was it reduced from 128?

char scratch[16 * 3 + 4];

I've no clue. :-(

@v0-e
Copy link
Author

v0-e commented Sep 7, 2023

Looks alright to me.

This line though seems a bit odd to me. What's the reason for this roundabout way of creating an array with a size of 52? And why was it reduced from 128?

char scratch[16 * 3 + 4];

I copied some code of the JER OCTET STRING encoder implementation:
https://github.com/mouse07410/asn1c/blob/vlm_master/skeletons/OCTET_STRING_jer.c#L17
I just left it because we don't need 128 bytes here.

@kentkost
Copy link

kentkost commented Sep 8, 2023

I am unsure if the function responsible for encoding a JER BIT STRING (BIT_STRING_encode_jer()) is aware if the field is constrained in size or not to implement the above distinction.

I don't see how it would be possible to comply with X.697 in regards to variable lengths.
Per @v0-e example.

Test ::= SEQUENCE {
    number INTEGER,
    bsFixed BIT STRING (SIZE(16)),
    bsVar BIT STRING
}

Should actually encode to

{
"Test":{
    "number": 122,
    "bsFixed": "9e44",
    "bsVar": {
        "length": 14,
        "value": "9e44"
    }
}

I just don't see how that is possible under the current implementation. The constraints "belong" to the PDU itself. But the encoding has long since happened. There is just no way to check beforehand if the BIT STRING that is being encoded has any size constraints.
It would require that as you encode know where you are in the tree ie. which PDU is being encoded and subsequent primitives. It would also require the name of the element being encoded. And then you would have to be able to have some kind of lookup on the PDU itself to get the result of whether the element currently being encoded has any SIZE constraints.

I think it is best to just deviate from the specification in this case. At least until this has been properly discussed on how this should be solved.

@mouse07410 mouse07410 merged commit 989bb0c into mouse07410:vlm_master Oct 6, 2023
1 check passed
@v0-e v0-e deleted the dev branch October 10, 2023 21:34
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.

3 participants