Conversation
a55fe9f to
1704afd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b07a855 to
98024ef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
98024ef to
72d4d11
Compare
15491f8 to
803ddbc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
+ Coverage 63.95% 64.81% +0.85%
==========================================
Files 9 9
Lines 774 790 +16
Branches 172 176 +4
==========================================
+ Hits 495 512 +17
+ Misses 220 218 -2
- Partials 59 60 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@twyatt rebased & fixed |
twyatt
left a comment
There was a problem hiding this comment.
Amazing job on this! 💯
I've added a unit test for "Test Vector 8", it'd be great to replicate the test for the remaining test vectors. Feel free to take that on, or I'll get to it next weekend.
|
|
||
| /** RFC 8613 2. OSCORE */ | ||
| data class Oscore( | ||
| val coseObject: ByteArray, |
There was a problem hiding this comment.
What was your thinking for settling on coseObject naming?
Hm. I thought in section 2 it stated that the option "contains a compressed COSE object", and I just wanted a descriptive name if I could find one.
But I realize now I have probably misinterpreted this. The option just "indicates that the CoAP message is an OSCORE message", and that the message contains a COSE object.
2. The OSCORE Option
The OSCORE option defined in this section (see Figure 3, which
extends "Table 4: Options" of [RFC7252]) indicates that the CoAP
message is an OSCORE message and that it contains a compressed COSE
object (see Sections 5 and 6). The OSCORE option is critical, safe
to forward, part of the cache key, and not repeatable.
What is actually in the option value described in section 6.1:
6.1. Encoding of the OSCORE Option Value
The value of the OSCORE option SHALL contain the OSCORE flag bits,
the 'Partial IV' parameter, the 'kid context' parameter (length and
value), and the 'kid' parameter as follows:
So I think it should be renamed, maybe just to value.
For the block options I did some handling of the uint value to expand it to the block number, more flag, and block size. I guess a semantically aware OSCORE option implementation should also extract all those flags, 'Partial IV' parameter and 'kid context' parameter things. But maybe that could be left out for now, with just an opaque value?
There was a problem hiding this comment.
I did a take on actually handling partialIv, kidContext and kid in
zalenskivolt#3
learning a lot about Kotlin nullables (since both kidContext and kid can have the flag set but be empty) :)
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.
There was a problem hiding this comment.
For upgradability/backwards compatibility, kotlin just says "Avoid using data classes in your API"
https://kotlinlang.org/docs/api-guidelines-backward-compatibility.html#avoid-using-data-classes-in-your-api
and here's one suggestion for mitigation: No data:
https://jakewharton.com/public-api-challenges-in-kotlin/#mitigation-no-data
but I'll leave these considerations for the library maintainers 😉
There was a problem hiding this comment.
zalenskivolt#3 looks great, but let's leave that for review after we get this PR in. So ya, I appreciate you keeping it as a separate PR for now. 👍
There was a problem hiding this comment.
zalenskivolt#3 looks great, but let's leave that for review after we get this PR in. So ya, I appreciate you keeping it as a separate PR for now. 👍
-> #343
a92af01 to
b2555ac
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b2555ac to
71903c3
Compare
Added test vectors 4-7. (test vector 1-3 is just key derivation, no OSCORE options) |
…and found a bug in zalenskivolt#3 because of them! :) |
1d235d9 to
f788b0e
Compare
might be good to merge the block-wise ones first, if they are getting ready now. they all conflict a lot, in all the imports, and in tests if they are added in the same location… I think this one is easier to rebase! 😄 |
Good thinking. I'll wait to merge this PR until after #335 is merged. 👍 |
|
@zalenskivolt please rebase/fix conflicts, when you have a chance. TY! |
f788b0e to
707f681
Compare
rebased & fixed |
707f681 to
292722d
Compare
QuantumRand
left a comment
There was a problem hiding this comment.
Awesome to see expanded support for more of the CoAP spec! Thanks @zalenskivolt!

see IANA CoAP Option Numbers