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

Changed DTLSCiphertext to DTLSCIDCiphertext for easier readability #82

Closed
wants to merge 1 commit into from

Conversation

hannestschofenig
Copy link
Collaborator

Doing so I introduce ciphertext_length, as the length of the AEAD-encrypted form of the serialized DTLSInnerPlaintext structure.

Doing so I introduce ciphertext_length, as the length of the AEAD-encrypted form of the serialized DTLSInnerPlaintext structure.
opaque enc_content[DTLSCiphertext.length];
} DTLSCiphertext;
opaque enc_content[ciphertext_length];
} DTLSCIDCiphertext;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I may be wrong.
For me, the old defines a DTLSCiphertext, which contains a length field, which is referenced by DTLSCiphertext.length. the new one now has still that field, but introduces a "ciphertext_length", which may be considered as "maverick".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would DTLSCIDCiphertext.length be a replacement for "ciphertext_length"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using DTLSCIDCiphertext.length is IMHO incorrect because the length of the encrypted content is different from the length of the entire DTLSCIDCiphertext structure. Do I see that wrong?

Copy link
Collaborator

@boaks boaks Feb 5, 2021

Choose a reason for hiding this comment

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

My understanding:
DTLSCIDCiphertext.length is not the length of DTLSCIDCiphertext, it's the value of the field uint16 length of that structure.

e.g.

   uint16 encLength;
   opaque enc_content[DTLSCIDCiphertext.encLength];
} DTLSCIDCiphertext;

RFC5246 contains

 struct {
          ContentType type;       /* same as TLSPlaintext.type */
          ProtocolVersion version;/* same as TLSPlaintext.version */
          uint16 length;
          opaque fragment[TLSCompressed.length];
      } TLSCompressed;

the TLSCompressed.length can't be the length of the whole TLSCompressed structure, but instead the value of the field uint16 length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this interpretation is correct.

Comment on lines +275 to +276
: The AEAD-encrypted form of the serialized DTLSInnerPlaintext structure of
length ciphertext_length.

Choose a reason for hiding this comment

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

Why is this just for AEAD cipher types?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jsalowey

FMPOV, there are currently two interpretations of that definition.

Hannes:

Using DTLSCIDCiphertext.length is IMHO incorrect because the length of the encrypted content is different from the length of the entire DTLSCIDCiphertext structure. Do I see that wrong?

So xxx.length is the length of xxx.

My understanding:

DTLSCIDCiphertext.length is not the length of DTLSCIDCiphertext, it's the value of the field uint16 length of that structure.

So xxx.length is the value of the field xxx.length.

I think, my interpretation is right.

If I'm right, then the changes for "easier readability" will change the term (field name) length in more places, not only there.

   ...
    uint16 ciphertext_length;
   opaque enc_content[DTLSCIDCiphertext.ciphertext_length];
} DTLSCIDCiphertext;

with that, also the term lengthin the text, not only in the structure, must be exchanged with ciphertext_length. And, of course, for the other cipher types as well.

With that, I'm not sure, if exchange that term really pays off, this style/expression is used in many other RFCs, and so should be common to the most readers.

(I guess, if @hannestschofenig finds the time to revisit this, he will agree on my interpretation and with that, it may either "change much more", or this PR may get withdrawn.)

@ekr
Copy link
Contributor

ekr commented Feb 27, 2021

It seems to me that there are two changes here:

  1. The change to DTLSCIDCiphertext -- innocuous but maybe unnecessary
  2. The change to encrypted_record, which is, I believe, wrong. You'll note that RFC8446 uses the style pre-PR.

I suggest we either close this PR or just do 1.

@boaks
Copy link
Collaborator

boaks commented Feb 28, 2021

Just for information:

I assume, that the irritation about DTLSCiphertext.length is mainly caused by very common implicit definitions of ???.length in in programing languages, e.g. Java, where

byte[] data = new byte[0124];
...
if (data.length > 0) ...
...

To escape such accidentally misinterpretations using a name different from "length" (and please not "size" nor "sizeof") may help. But I have my doubts, if the effect is worth the work. And potentially, all others, which are common to the notation within the RFC may then get mixed up by such new names.

@hannestschofenig
Copy link
Collaborator Author

I have been trying to address the review from Ben and, reading through your comments, my attempt to improve the readability failed. This means that I will withdraw this PR.

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

4 participants