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

Add Unassigned, Reserved, and ExperimentalUse options holding opaque binary values #328

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

zalenskivolt
Copy link

@zalenskivolt zalenskivolt commented Mar 19, 2025

Add option classes to hold any option number encountered, with an opaque value.
Decode any unknown options into an Unassigned, Reserved or ExperimentalUse option instead of returning an error.
Format the value in hex in OptionSerializer.

This is to support handling any new or experimental option, for examples new entries in this list
https://www.iana.org/assignments/core-parameters/core-parameters.xhtml#option-numbers
or any custom or experimental one.

The options are classified as:

  • Reserved for the reserved option numbers: 0, 128, 132, 136, and 140
  • ExperimentalUse in the range 65000-65535
  • Unassigned if otherwise unknown

See IANA CoAP Option Numbers

Closes #182

@twyatt twyatt added the minor Changes that should bump the MINOR version number label Mar 20, 2025
@twyatt
Copy link
Member

twyatt commented Mar 21, 2025

Let me mull on this for a bit.

I'm not eager to implement a "catch-all" type — [assuming not a huge lift] being that CoAP Option Numbers (link you provided) has an exhaustive list of options, it seems it would be better to instead implement those types?

twyatt added a commit that referenced this pull request Mar 21, 2025
@zalenski
Copy link

zalenski commented Mar 21, 2025

That list also has wide ranges providing for new extensions and experimental use, so I think you would need some way to capture a general option number. Is essence, all numbers could potentially get used

0-255 IETF Review or IESG Approval
256-2047 Specification Required
2048-64999 Expert Review
65000-65535 Experimental use (no operational use)

Of course, it would be great to cover all options already defined with specific implementations. There might possibly be some upgrade issues if first the "Unknown" or general catch-all is added, and later on more specific implementations get added, for anyone using such options. On the other hand, there's no easy way to implement specific option handling before that option is added, and implementations could still pass on all options as is, and things like that. So maybe it is not too bad if a general catch-all is added first, and more specific implementations get added later?

For my personal use case as an example, we are using the No-Response option that is in the list, which is how I noticed the issue. Now we are also about to add a custom option for internal use, and I wish the online KoAP parser to still be useful for all of our CoAP messages!

Please mull over all this. Maybe some other naming is better, and maybe considerations for future upgrades are important. I'm still pushing for any encountered option to be handled in some way! 😄

@twyatt
Copy link
Member

twyatt commented Mar 21, 2025

My thinking is that we simply implement the handful of additional types defined in CoAP Option Numbers (e.g. No-Response, Echo, etc) and then also add:

  • Unassigned
  • Reserved
  • ExperimentalUse

So, I guess Unassigned essentially is Unknown, but it will fall under well defined option numbers. 🙃

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 59.87%. Comparing base (753f4a7) to head (6e1dcdd).

Files with missing lines Patch % Lines
koap-core/src/commonMain/kotlin/Message.kt 37.50% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
+ Coverage   58.52%   59.87%   +1.34%     
==========================================
  Files           8        8              
  Lines         639      648       +9     
  Branches      145      148       +3     
==========================================
+ Hits          374      388      +14     
+ Misses        228      210      -18     
- Partials       37       50      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@twyatt
Copy link
Member

twyatt commented Mar 21, 2025

@zalenskivolt let me know if you want to tackle adding the few remaining options, or if you'd like me to take that on?

@zalenski
Copy link

Sounds like a plan!

This would make future allocated option numbers parsed as "unassigned", but I guess that makes perfect sense until the code gets updated.

Maybe both Reserved and ExperimentalUse could inherit from Unassigned? They are all unassigned too…

Interesting that the reserved ones are just 5 numbers. The option 0 (zero), and four reserved for "more Location-*" options.

From RFC7252(CoAP) 5.10.7. Location-Path and Location-Query

Beyond Location-Path and Location-Query, more Location-* options may be defined in the future
and have been reserved option numbers 128, 132, 136, and 140.

@zalenski
Copy link

@zalenskivolt let me know if you want to tackle adding the few remaining options, or if you'd like me to take that on?

I'll see if I can make a start!

@twyatt
Copy link
Member

twyatt commented Mar 21, 2025

Maybe both Reserved and ExperimentalUse could inherit from Unassigned? They are all unassigned too…

If you can make the class hierarchy play nice, then that would be ideal. I'm also fine w/ them living alongside Unassigned if them inheriting causes issues.

Interesting that the reserved ones are just 5 numbers. The option 0 (zero), and four reserved for "more Location-*" options.

From RFC7252(CoAP) 5.10.7. Location-Path and Location-Query

Ya, the scattering of reserved options number is quite strange. I wonder if it has something to do w/ the bitfield? 🤷

Integer value Hex Bits
128 80 1000_0000
132 84 1000_0100
136 88 1000_1000
140 8C 1000_1100

@zalenski
Copy link

zalenski commented Mar 21, 2025

If you can make the class hierarchy play nice, then that would be ideal. I'm also fine w/ them living alongside Unassigned if them inheriting causes issues.

from a quick google:

stackoverflow/extend-data-class-in-kotlin

The truth is: data classes do not play too well with inheritance. We are considering prohibiting or severely restricting inheritance of data classes. For example, it's known that there's no way to implement equals() correctly in a hierarchy on non-abstract classes.

So, all I can offer: don't use inheritance with data classes.

@zalenskivolt zalenskivolt changed the title Add UnknownOption holding an opaque binary value Add Unasigned, Reserved, and ExperimentalUse options holding opaque binary values Mar 21, 2025
@zalenskivolt zalenskivolt changed the title Add Unasigned, Reserved, and ExperimentalUse options holding opaque binary values Add Unassigned, Reserved, and ExperimentalUse options holding opaque binary values Mar 21, 2025
@twyatt
Copy link
Member

twyatt commented Mar 21, 2025

Looks like a great start! I'll try to find time this weekend to look it over.

@twyatt twyatt self-requested a review March 21, 2025 23:25
@zalenskivolt
Copy link
Author

zalenskivolt commented Mar 22, 2025

@zalenskivolt let me know if you want to tackle adding the few remaining options, or if you'd like me to take that on?

I'll see if I can make a start!

Started with 258 No-Response in #332, that I had already started on.

Feel free to comment on that one, and I'll try to apply changes to any others…
Maybe they could be added in some blocks, like OSCORE+EDHOC, the different block-wise ones, hop-limit+echo+request-tag?

Missing ones are

# Option RFC
9 OSCORE [RFC8613 OSCORE (using COSE)]
16 Hop-Limit [RFC8768 Hop-Limit]
19 Q-Block1 [RFC9177 Block-Wise robust]
21 EDHOC [RFC9668 Ephemeral Diffie-Hellman Over COSE]
23 Block2 [RFC7959 Block-Wise][RFC8323 TLS+][RFC8613 OSCORE]
27 Block1 [RFC7959 Block-Wise][RFC8323 TLS+][RFC8613 OSCORE]
28 Size2 [RFC7959 Block-Wise][RFC8613 OSCORE]
31 Q-Block2 [RFC9177 Block-Wise robust]
252 Echo [RFC9175 Echo, Request-Tag, and Token Processing]
258 No-Response [RFC7967 No Server Response][RFC8613 OSCORE]
292 Request-Tag [RFC9175 Echo, Request-Tag, and Token Processing]

TLS+ = TCP, TLS, and WebSockets

and these are also listed without RFCs:

# Option Reference
2049 OCF-Accept-Content-Format-Version [Michael_Koster]
2053 OCF-Content-Format-Version [Michael_Koster]
2055 SCP82-Params [GPC_SPE_207]

@zalenskivolt
Copy link
Author

@zalenskivolt let me know if you want to tackle adding the few remaining options, or if you'd like me to take that on?

I'll see if I can make a start!

Started with 258 No-Response in #332, that I had already started on.

Feel free to comment on that one, and I'll try to apply changes to any others… Maybe they could be added in some blocks, like OSCORE+EDHOC, the different block-wise ones, hop-limit+echo+request-tag?

PRs for all 11 now:

PR Options
#332 No-Response
#333 OSCORE, EDHOC
#334 Hop-Limit, Echo, Request-Tag
#335 Size2, Block1, Block2, QBlock1, QBlock2

All of them update the comments in Message.kt, I planned to rebase that away from all but one of them (or add annother PR?).

@twyatt
Copy link
Member

twyatt commented Mar 22, 2025

PRs for all 11 now:

PR Options
#332 No-Response
#333 OSCORE, EDHOC
#334 Hop-Limit, Echo, Request-Tag
#335 Size2, Block1, Block2, QBlock1, QBlock2
All of them update the comments in Message.kt, I planned to rebase that away from all but one of them (or add annother PR?).

Thanks so much for all your work on all this! Really incredible effort! ❤️

I'll follow up on this PR after #329 and #332 are merged.

Apologies for all the rebasing that will likely ensue. 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Changes that should bump the MINOR version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse unknown options as opaque instead of failing the whole message
3 participants