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

Carl's in depth review #15

Closed
thomas-fossati opened this issue Dec 20, 2022 · 5 comments
Closed

Carl's in depth review #15

thomas-fossati opened this issue Dec 20, 2022 · 5 comments

Comments

@thomas-fossati
Copy link
Collaborator

thomas-fossati commented Dec 20, 2022

See https://mailarchive.ietf.org/arch/msg/rats/Bmlxnu0GmnRMsP6WEq3CBjLVm0k/

@thomas-fossati
Copy link
Collaborator Author

thomas-fossati commented Mar 6, 2023

Below are some suggestions/questions regarding the RATS Conceptual Messages Wrapper draft (draft-ftbs-rats-msg-wrap-00).

Abstract

  • Should the spec have a broader title/focus since there appears to be nothing to limit usage to RATS conceptual messages?

While the mechanism is generic -- at least in part, now that we added the CM indicator to the array serialisation -- we want to keep the scope limited to RATS messages.

  • How does this draft relate to the EAT Media Types draft? That draft states that is "defines media types to be used for Entity Attestation Token (EAT) payloads independently of the RATS Conceptual Message in which they manifest themselves." Does this mean a separate media-type would be used with the encapsulation mechanisms defined in this draft?

This encap is intended for tunnelling RATS messages in protocols that don't use media types as first class type identifiers themselves. The relation of this spec with the EAT media type I-D is one of "consumer": if EAT MT has a type that is good for the message, we can use it.

Section 1

  • In the first sentence of the second paragraph, what is being registered or used interoperably? This would clarify what "their" means in the second sentence. I think the media type notion might need to be introduced before this paragraph.

In #22, I hope I have explained a bit better the basic reason for providing a single encap, and what registrations we are amortising.

Section 3.1

  • Is there any reason to not use .det with indentation of the RFC6838 definition? I think that'd be easier to read.

To improve readability this has now been separated into its own section.

Section 3.2

  • What if a CoAP Content Format does not exist for the media type? I am assuming a media-type value is used, but it may be worth stating that.

Done in #23

  • Are tag definitions intended to adhere to the same limitations defined for the value field in the array, i.e., cbor-bytes / base64-string serialized per the tag number? If not, what would govern the type?

This is stated in §3.2: "[...] encoded as a CBOR byte string, to which the tag is prepended."

Section 4

  • I think it would be helpful to include ASCII hex of the CBOR encodings in addition to the diagnostic (i.e., 8219753144ABCDABCD and DA6374763244ABCDABCD).

Agreed; we have added the CBOR "pretty" output to the examples.

  • It may be worth including a CBOR example with a media-type value. This could possibly be as a second hypothetical for which no CoAP content format has been defined or as an example with parameters of the same type as current example.

Done in #23

  • It may be worth showing the CDDL definition of the resulting tag as it may appear in some protocol definition, i.e.. example_tag = #6.1668576818(bstr).

We have now used a brand new CDDL feature ("non-literal CBOR tag numbers") in §3.2 to do that in a generic way.

thomas-fossati added a commit that referenced this issue Apr 3, 2023
Partially addresses #15

Signed-off-by: Thomas Fossati <thomas.fossati@arm.com>
This was referenced Apr 3, 2023
thomas-fossati added a commit that referenced this issue Apr 18, 2023
Partially addresses #15

Signed-off-by: Thomas Fossati <thomas.fossati@arm.com>
@thomas-fossati
Copy link
Collaborator Author

hi Carl, we think we addressed all your points. See https://thomas-fossati.github.io/draft-ftbs-rats-msg-wrap/draft-ftbs-rats-msg-wrap.html

Could you please check when you have some spare time?

/cc @carl-wallace

@carl-wallace
Copy link

carl-wallace commented Apr 20, 2023 via email

thomas-fossati added a commit that referenced this issue Apr 24, 2023
apply Carl's suggestions and fixes at [1]

[1] #15 (comment)

Signed-off-by: Thomas Fossati <thomas.fossati@arm.com>
@thomas-fossati
Copy link
Collaborator Author

Yep. Thanks.

Cool, thanks for checking (and for the further comments).

A few nits/comments: [...]

done in #30

thomas-fossati added a commit that referenced this issue Jun 15, 2023
apply Carl's suggestions and fixes at [1]

[1] #15 (comment)

Signed-off-by: Thomas Fossati <thomas.fossati@arm.com>
@thomas-fossati
Copy link
Collaborator Author

Carl's original review was addressed in #22 and #23.

The follow-up re-review (#15 (comment)) has been addressed in #30

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

No branches or pull requests

2 participants