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

Updates following Marco's review #199

Merged
merged 7 commits into from Dec 14, 2021
Merged

Conversation

gselander
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@emanjon emanjon left a comment

Choose a reason for hiding this comment

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

"By default, the message flow is as follows:"

  • Text misses error as a result from 4, that is not very default, but none of the error are....

"but intended to enable the use short identifiers in combination with simplifying the retrieval of the right security context in the application protocol."

  • of in "the use short" is missing
  • Why would "enable" be better than "simplify"? Don't remember what EDHOC says, but simplify is absolutely a better word than enable for the use in COSE/OSCORE.
  • Retrieval of the right security context is also for EDHOC. This change remove that by specifying "application protocol".

"identities to verify the 'sub' (subject) claim of the CWT."

  • Should use the same text for X509 and CWT.
  • Not added in this PR, but "sub" claim is just an example. CTW might add AltSub in the future.

"message_4 MUST be supported and the Responder MUST send message_4."

  • Good change, but the "Responder MUST send message_4" is still strange, would about I? Maybe just "message_4 MUST be used"?

@gselander
Copy link
Collaborator Author

"By default, the message flow is as follows:"

Text misses error as a result from 4, that is not very default, but none of the error are....

Done

"but intended to enable the use short identifiers in combination with simplifying the retrieval of the right security context in the application protocol."

of in "the use short" is missing
Why would "enable" be better than "simplify"? Don't remember what EDHOC says, but simplify is absolutely a better word than enable for the use in COSE/OSCORE.
Retrieval of the right security context is also for EDHOC. This change remove that by specifying "application protocol".

Marco's comment was about interpretation of "security context", I reverted and bring up that in the response to his review mail.

"identities to verify the 'sub' (subject) claim of the CWT."

Should use the same text for X509 and CWT.
Not added in this PR, but "sub" claim is just an example. CTW might add AltSub in the future.

Marco's comment was an editorial, which I kept. Your comment is a separate issue which i opened as #215

"message_4 MUST be supported and the Responder MUST send message_4."

Good change, but the "Responder MUST send message_4" is still strange, would about I? Maybe just "message_4 MUST be used"?

Done

@gselander gselander merged commit 777b155 into master Dec 14, 2021
@gselander gselander deleted the updates-following-Marco's-review branch April 18, 2022 12:43
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.

None yet

2 participants