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

PR#117 rebased on master #129

Merged
merged 5 commits into from Jun 18, 2021
Merged

PR#117 rebased on master #129

merged 5 commits into from Jun 18, 2021

Conversation

gselander
Copy link
Collaborator

No description provided.

@gselander
Copy link
Collaborator Author

Didn't manage to make a PR on #117, but this should just be rebasing on current master

@gselander
Copy link
Collaborator Author

I reviewed #117 and updated it with my proposed changes in the commit above. Pending resolution of comments in #117 and further quick comments I think we are ready to merge.

gselander added a commit that referenced this pull request Jun 13, 2021
@emanjon
Copy link
Collaborator

emanjon commented Jun 17, 2021

I updated the example message sizes, I think it makes most sense to describe EDHOC itselft without any transport parameters.

Two editorials

"may need ... no, they don't need anything special"

"For use of EDHOC in the XX protocol"

Otherwise I think this is ready to merge

@emanjon
Copy link
Collaborator

emanjon commented Jun 17, 2021

Christian had a comment that one of the hashes became useless. I am not sure it should be removed. New methods may specify things send in message_3.

@gselander
Copy link
Collaborator Author

Let's not do the editorials now, current text can be kept as placeholders.

Let's remove data_3 and the iterated hash until there is a concrete method that needs it, but let's merge first to avoid further conflicts.

@gselander gselander merged commit f97a5c5 into master Jun 18, 2021
gselander added a commit that referenced this pull request Jun 18, 2021
gselander added a commit that referenced this pull request Jun 18, 2021
Copy link
Contributor

@malishav malishav left a comment

Choose a reason for hiding this comment

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

A couple of editorial comments while going through the PR.


If the whole transport path provides a mechanism for correlating messages received with messages previously sent, then some of the connection identifiers may be omitted. There are four cases:
If a connection identifier is used with an application protocol for which EDHOC establishes keys (such as OSCORE) then the connection identifiers SHALL adhere to the requirements for that protocol. For OSCORE the choice of connection identifier results in the endpoint selecting its Recipient ID, see Section 3.1 of {{RFC8613}}), for which certain uniqueness requirements apply, see Section 3.3 of {{RFC8613}}).
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposal to rephrase for clarity to:

If EDHOC connection identifiers are used by an application protocol for which EDHOC established keys (such as OSCORE), then the selected connection identifiers SHALL adhere to the requirements for that protocol. For OSCORE, the choice of a connection identifier results in the endpoint selecting its Recipient ID (see Section 3.1 of {{RFC8613}}), for which the uniqueness requirements from Section 3.3 of {{RFC8613}} apply.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, included (with minor modification) as you can see.


For example, if the key exchange is transported over CoAP, the CoAP Token can be used to correlate messages, see {{coap}}.
The Initiator and the Responder need to have agreed on a transport to be used for EDHOC, see {{applicability}}. It is recommended to transport EDHOC in CoAP payloads, see {{coap}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

How come it is "recommended" to transport EDHOC in CoAP? The fact that CoAP is the most frequent use case for transporting EDHOC does not make it a "recommended" transport IMO.

Consequently, the application/edhoc media type does not apply to these messages;
their media type is unnamed.

An example of a successful EDHOC exchange using CoAP is shown in {{fig-coap1}}. In this case the CoAP Token enable correlation on the Initiator side, and the prepended C_R enables correlation on the Responder (server) side.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/enable/enables

@malishav
Copy link
Contributor

In general, I think that this approach is a cleaner way to solve the issue than what we had with CORR. Still, as discussed in #117, in order to implement this, interaction between three different protocol implementations is needed: EDHOC to negotiate connection identifiers, OSCORE/application for uniqueness requirements and CoAP/transport layer to transport them. The text is not very clear on how this interaction happens. While this is an implementation decision, it would still be good to organize the text in such a way that a clean separation of concerns is described.

One way of doing this would be to split "Connection Identifiers and Transport" into two separate sections:

  • "Connection Identifiers", and
  • "Transport".

I would further split the "Connection Identifiers" section into three subsections, e.g:

  1. "CID Negotiation",
  2. "CID Selection", and
  3. "CID Transport".

The goal is to describe a clear flow and who does what. For instance, EDHOC calls an OSCORE API which proposes the CID, EDHOC negotiates the CID by exchanging EDHOC messages, EDHOC sets the CIDs to the underlying transport as soon as the CIDs are available. Transport module decides when to send and when to omit the CIDs depending on its correlation properties. The text should be general of course to account for other applications and transports. Would that make sense?

P.S. I am missing normative text in the CoAP transport case mandating the transport of connection identifiers for different messages (in Section 7.2 ?). Am I missing the reason why there is no normative text at all in Section 7?

@gselander
Copy link
Collaborator Author

I forgot to respond to @malishav. The PR which was merged addressed much of these comments.

  • split "Connection Identifiers and Transport" into two separate sections
  • split the "Connection Identifiers" section into subsections
    • CID Selection
    • Use of CID in EDHOC
    • Use of CID in OSCORE
    • and expanded text about CID transport in the new Transport section

The second split was not exactly what was requested but it made more sense to me: The concept of "CID negotation" was redundant in the draft and replaced with "CID Selection" in a few occurences, wherefore a separate section of CID negotation was not needed. Having one section about CID Transport and then immediately followed by a section about Transport lead to duplicated text so I settled for a reference to the latter section.

I did not include any text about OSCORE API which seemed too implementation specific.

About normative text for CoAP transport of connection identifier, I don't know why that is needed. How EDHOC messages are transported is application dependent. If desirable an applicability statement can state that they use connection identifiers as described in A.3. I don't think adding normative text makes a difference.

If you think this is missing, or something else was missed in this comment, please make separate issues. Thanks.

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

3 participants