-
Notifications
You must be signed in to change notification settings - Fork 10
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
AUTH48 #199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my initial review of the RFC Editor's changes. Mostly minor stuff, a few rewordings, no major disagreements. I have made TODO(RLB)
notations for things that couldn't be handled in GitHub suggestions, and will come back to implement those shortly.
draft-ietf-sframe-enc.md
Outdated
<!--[rfced] May we make the title more descriptive? We note that a web search on "SFrame" returns pages describing different technologies. | ||
|
||
Current: Secure Frame (SFrame) | ||
|
||
Perhaps: Secure Frame (SFrame): an Encryption and Authentication Mechanism for Media Frames | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to add a colon-subtitle, I would use something like:
Secure Frame (SFrame): Lightweight Authenticated Encryption for Real-Time Media
@eomara @juberti @murillo128 @youennf What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown edits FTW.
Co-authored-by: Martin Thomson <[email protected]>
Latest commit addresses comments from @martinthomson, including reworking the audio frame table, removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read over all the changes and overall the edits all look great, the text is clear and easy to follow. Noted a couple minor nits.
4. Independence from the underlying transport, including use in non-RTP | ||
transports, e.g., WebTransport {{?I-D.ietf-webtrans-overview}}. | ||
4. Decouple the media encryption framework from the underlying transport, | ||
allowing use in non-RTP, e.g., WebTransport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowing use in non-RTP, e.g., WebTransport | |
allowing use in non-RTP transports, e.g., WebTransport |
@@ -391,7 +414,7 @@ aspects of the AEAD and the hash algorithm below: | |||
size of a "tag" that is added to the plaintext) | |||
|
|||
* `AEAD.Nka` - For cipher suites using the compound AEAD described in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally read Nka
as "size-of-key-for-AEAD", which made sense to me, but then I realized that since we're always using AEAD, it's really "size-of-key-for-compound-AEAD". Maybe Nkc
?
policies, that new key frame could take some time to be generated. | ||
|
||
If the sender sends a key frame after the new E2EE key is in use, the time | ||
required for the new participant to display the video is minimized. | ||
|
||
Note that this issue does not arise for media streams that do not have | ||
dependencies among frames, e.g., audio streams. In these streams, each frame is | ||
independently decodeable, so there is never a need to process two frames | ||
together which might be on two sides of a key rotation. | ||
independently decodable, so there is never a need to process together two frames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I preferred the previous text, or perhaps "process two frames together that are on either side of a key rotation".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a better rewording.
3.3.2 of ?RFC3711}} specifies a counter-based anti-replay mitigation, which | ||
could be adapted to use with SFrame, using the CTR field as the counter. | ||
|
||
## Metadata | ||
|
||
The `metadata` input to SFrame operations is pure application-specified data. As | ||
such, it is up to the application to define what information should go in the | ||
The `metadata` input to SFrame operations an opaque byte string specified by the application. As |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `metadata` input to SFrame operations an opaque byte string specified by the application. As | |
The `metadata` input to SFrame operations is an opaque byte string specified by the application. As |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, although i think that we are still inconsistently using Key Id
an KID
and Counter
and CTR
. We also use Counter (CTR)
in several places.
I think we should either introduce the terms once and then use the abbreviations everywhere, or not use the abbreviations at all.
@eomara did you mean to merge this? There are still outstanding discussions. |
Agree that this should not have been merged quite yet. I'll start up a new PR to handle the remaining comments. |
This PR contains the RFC Editor's markdown as an initial commit. The author team will review this PR as with any other changes, and make any responses to the RFC Editor as comments or further commits on this PR.
Once this PR is merged, we will indicate to the RFC Editor that the Markdown is approved for publication.