Handle Oscore `partialIv`, `kidContext`, and `kid` explicitly by zalenskivolt · Pull Request #343 · JuulLabs/koap

zalenskivolt · 2025-03-31T22:06:54Z

No description provided.

codecov · 2025-03-31T22:10:26Z

Codecov Report

❌ Patch coverage is 78.84615% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.61%. Comparing base (6cbf889) to head (d818bb2).

Files with missing lines	Patch %	Lines
koap-core/src/commonMain/kotlin/Oscore.kt	79.16%	9 Missing and 1 partial ⚠️
koap-core/src/commonMain/kotlin/Message.kt	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   69.04%   69.61%   +0.56%     
==========================================
  Files           9       10       +1     
  Lines         798      849      +51     
  Branches      217      236      +19     
==========================================
+ Hits          551      591      +40     
- Misses        161      171      +10     
- Partials       86       87       +1

☔ 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 · 2025-04-22T08:36:34Z

@zalenskivolt apologies; I've been busy on other work projects, but I plan to review this soon.

cedrickcooke

Sorry for the delays, overall great work again. One small change and then this turns into an approve

koap-core/src/commonMain/kotlin/Message.kt

zalenskivolt · 2025-04-23T10:10:35Z

koap-core/src/commonMain/kotlin/Message.kt

+                val partialIvLength = partialIv.size
+                if (partialIvLength > 5) {
+                    throw IllegalArgumentException("Partial IV length of $partialIvLength is outside allowable range of 0..5")
+                }
+                val kidContextLength = if (kidContext != null) kidContext.size else 0
+                val kidFlag = if (kid != null) 0x08 else 0
+                val kidContextFlag = if (kidContext != null) 0x10 else 0
+                val flagBits = partialIvLength or kidFlag or kidContextFlag
+                if (flagBits == 0) {
+                    // If the OSCORE flag bits are all zero (0x00), the option value SHALL be empty
+                    return byteArrayOf()
+                }
+                var value = byteArrayOf(flagBits.toByte()) + partialIv
+                if (kidContext != null) {
+                    value += byteArrayOf(kidContextLength.toByte()) + kidContext
+                }
+                if (kid != null) {
+                    value += kid
+                }
+                return value


these are some 20+17 lines of fairly dense/complicated code, with magic flag constants, etc.
maybe they should go in their own .kt file to isolate the complexity, and using constants for flags and masks?
not too happy that kidFlag and kidContextFlag is a byte/int value in one place and boolean in the other, maybe they should at least be named differently… (kidMask/kidContextMask?)

also, as mentioned in #333

There's reserved values for partialIv length 6 and 7, and three flags reserved for future use, so there's some potential for future updates to the standard rendering the option unparseable, and such flags/values should be handled in specific ways as outlined in 8.2. Verifying the Request …

It could have a malformed flag in it, or maybe a nullable partialIv and use null there to indicate the option didn't parse? I guess these complex options are a bit trickier than the simple uint/string ones in basic CoAP.

not sure how likely the option is to change, and it could just be handled by updating the code. but som drawback compared to just a binary blob is that it might start failing if updated.

these are some 20+17 lines of fairly dense/complicated code, with magic flag constants, etc. maybe they should go in their own .kt file to isolate the complexity, and using constants for flags and masks? not too happy that kidFlag and kidContextFlag is a byte/int value in one place and boolean in the other, maybe they should at least be named differently… (kidMask/kidContextMask?)

added Oscore.kt with constants for flags, masks, and ranges, and updated to kid[Context]FlagBit

zalenskivolt · 2025-04-23T10:11:16Z

koap-core/src/commonMain/kotlin/Message.kt

+                    if (value.size == 0) {
+                        return Oscore(byteArrayOf(), null, null)
+                    }
+                    val flagBits = value[0].toInt()
+                    val partialIvLength = flagBits and 0x07
+                    val kidFlag = (flagBits and 0x08) != 0
+                    val kidContextFlag = (flagBits and 0x10) != 0
+                    val partialIv = value.sliceArray(1 until 1 + partialIvLength)
+                    val kidContextLength = if (kidContextFlag) value[1 + partialIvLength].toInt() else 0
+                    val kidContext = if (kidContextFlag) {
+                        value.sliceArray(2 + partialIvLength until 2 + partialIvLength + kidContextLength)
+                    } else {
+                        null
+                    }
+                    val kidOffset = (if (kidContextFlag) 2 else 1) + partialIvLength + kidContextLength
+                    val kid = if (kidFlag) value.sliceArray(kidOffset until value.size) else null
+                    return Oscore(partialIv, kidContext, kid)


these are some 20+17 lines of fairly dense/complicated code, with magic flag constants, etc.
maybe they should go in their own .kt file to isolate the complexity, and using constants for flags and masks?

zalenskivolt · 2025-04-23T10:32:51Z

@zalenskivolt apologies; I've been busy on other work projects, but I plan to review this soon.

no worries!

I had a bit of a break in my current work, which caused this flurry of 10 PRs 😄 but I'm also more back to other work projects now…

zalenskivolt · 2025-11-12T01:45:28Z

Another take: instead of breaking up the option value into parts, just do that in the toString method,
and optionally construct an Oscore option from the different parts, using Oscore.fromParts(partialIv, kidContext, kid)

zalenskivolt requested review from a team and twyatt as code owners March 31, 2025 22:06

zalenskivolt requested a review from cedrickcooke March 31, 2025 22:06

zalenskivolt force-pushed the oscore-partial-iv-kid-context-kid branch from 608948e to 6e82787 Compare March 31, 2025 22:07

zalenskivolt mentioned this pull request Mar 31, 2025

Add Oscore and Edhoc options #333

Merged

zalenskivolt changed the title ~~Handle Oscore partialIv, kidContext, and kid explicitly~~ Handle Oscore partialIv, kidContext, and kid explicitly Apr 1, 2025

zalenskivolt changed the title ~~Handle Oscore partialIv, kidContext, and kid explicitly~~ Handle Oscore partialIv, kidContext, and kid explicitly Apr 1, 2025

zalenskivolt force-pushed the oscore-partial-iv-kid-context-kid branch 2 times, most recently from c7b37d9 to 8728335 Compare April 6, 2025 12:42

cedrickcooke reviewed Apr 22, 2025

View reviewed changes

koap-core/src/commonMain/kotlin/Message.kt Outdated Show resolved Hide resolved

zalenskivolt commented Apr 23, 2025

View reviewed changes

koap-core/src/commonMain/kotlin/Message.kt Outdated Show resolved Hide resolved

zalenskivolt commented Apr 23, 2025

View reviewed changes

zalenskivolt force-pushed the oscore-partial-iv-kid-context-kid branch from 7c21fbb to 2c43ac5 Compare April 23, 2025 11:25

zalenskivolt and others added 3 commits November 12, 2025 01:46

Handle Oscore partialIv, kidContext, and kid explicitly

2f8558f

Use contentHashCode for all ByteArray hashes

633e113

Move oscore parse code to Oscore.kt, add constants

fa319d1

zalenskivolt force-pushed the oscore-partial-iv-kid-context-kid branch 3 times, most recently from 9fd8b11 to a47f626 Compare November 12, 2025 01:44

zalenskivolt force-pushed the oscore-partial-iv-kid-context-kid branch from a47f626 to 78c5ac5 Compare November 12, 2025 01:51

Make Oscore split by parts optional

d818bb2

zalenskivolt force-pushed the oscore-partial-iv-kid-context-kid branch from 78c5ac5 to d818bb2 Compare November 12, 2025 02:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle Oscore `partialIv`, `kidContext`, and `kid` explicitly#343

Handle Oscore `partialIv`, `kidContext`, and `kid` explicitly#343
zalenskivolt wants to merge 4 commits intoJuulLabs:mainfrom
zalenskivolt:oscore-partial-iv-kid-context-kid

zalenskivolt commented Mar 31, 2025

Uh oh!

codecov bot commented Mar 31, 2025 •

edited

Loading

Uh oh!

twyatt commented Apr 22, 2025

Uh oh!

cedrickcooke left a comment

Uh oh!

Uh oh!

Uh oh!

zalenskivolt Apr 23, 2025

Uh oh!

zalenskivolt Apr 23, 2025

Uh oh!

zalenskivolt Apr 23, 2025

Uh oh!

zalenskivolt Apr 23, 2025

Uh oh!

zalenskivolt commented Apr 23, 2025

Uh oh!

zalenskivolt commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

zalenskivolt commented Mar 31, 2025

Uh oh!

codecov bot commented Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

twyatt commented Apr 22, 2025

Uh oh!

cedrickcooke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zalenskivolt Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

zalenskivolt Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

zalenskivolt Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

zalenskivolt Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

zalenskivolt commented Apr 23, 2025

Uh oh!

zalenskivolt commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 31, 2025 •

edited

Loading