For the most part, the presentation language (somewhat informally) described in
section 3 and its use throughout the document is clear, but the use doesn't
always match the description and some things are written in different ways. I
have a few examples below of the issues I've noticed. After the examples, I
make concrete suggestions. To keep the following text as uncluttered as
possible but still provide easy reference, I've copied the relevant structures
definitions from Appendix B at the bottom of this message.
Array lengths have four different formats.
- Literal constants. For example, `opaque Random[32];` or `uint8
CipherSuite[2];`.
- Named, implied constants. For example `coordinate_length` in
`UncompressedPointRepresentation` which doesn't appear anywhere else, but is
informally described in the following paragraphs. A variant of this is
`Hash.length` in the `Finished` struct.
- Qualified field names. I.e., `struct.field`. For example
`TLSPlaintext.length` in the `TLSPlaintext` and `TLSInnerPlaintext` structs
- Unqualified field names. I.e., `field` where `field` is in the same structure
as the array. For example `length` in `TLSCiphertext`.
I think the unqualified field names use should be removed. We should write
`TLSCiphertext.length` instead. I think this is the only place that the
unqualified field name appears.
`Hash.length` and `coordinate_length` are both doing the same thing. I think
they should be written in the same way. `Hash` isn't a defined structure that
has fields so it makes sense to me to go with `hash_length`. (This would also
simplify parsing the presentation language.)
Next, most uses of `select` don't match the presentation language's definition.
Here's the general definition.
struct {
T1 f1;
T2 f2;
....
Tn fn;
select (E) {
case e1: Te1;
case e2: Te2;
....
case en: Ten;
} [[fv]];
} [[Tv]];
The definition isn't explicit, but it appears that the `Te1`, ..., `Ten` should
be types but most uses outside of section 3 are _not_ types. The only use of
`select` that appears to match the example is in the `Handshake` struct.
`KeyShare` and `PreSharedKeyExtension` have additional fields in their `case`
statements and, somewhat confusingly, `EarlyDataIndication`'s `case` statements
contain both fields and types.
In addition, the only one of those structs to use the optional label on the
`select` is `Handshake`.
I think we should make the presentation language definition actually conform to
its use which means we either need to change the definition or change the uses.
Options include
1. Add anonymous structs around all of the fields in each `case` statement.
2. Add auxiliary structure definitions (like we have for `struct {} Empty;`
already) and use the type names for each `case` statement.
2. Change the definition of the `case` statements from types to a sequence of 0
or more fields. Something like
struct {
T1 f1;
T2 f2;
....
Tn fn;
select (E) {
case e1:
Te1_1 fe1_1;
Te1_2 fe1_2;
....
Te1_m fe1_m;
case e2:
Te2_1 fe2_1;
Te2_2 fe2_2;
....
Te2_m fe2_m;
....
case en:
Ten_1 fen_1;
Ten_2 fen_2;
....
Ten_m fen_m;
} [[fv]];
} [[Tv]];
Option 1 adds a bunch of otherwise-useless `struct`s inline with the case
statements making them harder to read. Option 2 is going to be slightly easier
to read but still adds a bunch of structs. Option 3 complicates the definition
of `select` but better conforms with what we actually do.
Two additional benefits of option 3 are that we could remove the optional
`select` field label (the `[[fv]]`) and we can get rid of the `Empty`
structure. The only `select` that actually uses that label is `Handshake` and I
don't think that label is ever referred to.
Concretely, I think we should make the following changes.
1. Replace `length` with `TLSCiphertext.length` in the definition of
`TLSCiphertext`.
2. Replace `Hash.length` with `hash_length` throughout (9 instances).
3. Change the definition of `select`'s `case` statements to have 0 or more
fields (types and names) and remove the optional label.
4. Change the `select` example to match the new definition.
5. Change `Handshake` by adding field names to each `case` statement. (These
could all be `body` or they could be unique.) Remove the `body` label.
6. Delete the `Empty` structure and replace both current uses with the comment
`/* Empty */`.
Thoughts?
Copied from Appendix B for reference.
struct {
uint8 legacy_form = 4;
opaque X[coordinate_length];
opaque Y[coordinate_length];
} UncompressedPointRepresentation;
struct {
opaque verify_data[Hash.length];
} Finished;
struct {
ContentType type;
ProtocolVersion legacy_record_version;
uint16 length;
opaque fragment[TLSPlaintext.length];
} TLSPlaintext;
struct {
opaque content[TLSPlaintext.length];
ContentType type;
uint8 zeros[length_of_padding];
} TLSInnerPlaintext;
struct {
ContentType opaque_type = 23; /* application_data */
ProtocolVersion legacy_record_version = 0x0301; /* TLS v1.x */
uint16 length;
opaque encrypted_record[length];
} TLSCiphertext;
struct {
HandshakeType msg_type; /* handshake type */
uint24 length; /* bytes in message */
select (Handshake.msg_type) {
case client_hello: ClientHello;
case server_hello: ServerHello;
case end_of_early_data: EndOfEarlyData;
case hello_retry_request: HelloRetryRequest;
case encrypted_extensions: EncryptedExtensions;
case certificate_request: CertificateRequest;
case certificate: Certificate;
case certificate_verify: CertificateVerify;
case finished: Finished;
case new_session_ticket: NewSessionTicket;
case key_update: KeyUpdate;
} body;
} Handshake;
struct {
select (Handshake.msg_type) {
case client_hello:
KeyShareEntry client_shares<0..2^16-1>;
case hello_retry_request:
NamedGroup selected_group;
case server_hello:
KeyShareEntry server_share;
};
} KeyShare;
struct {
select (Handshake.msg_type) {
case client_hello:
PskIdentity identities<7..2^16-1>;
PskBinderEntry binders<33..2^16-1>;
case server_hello:
uint16 selected_identity;
};
} PreSharedKeyExtension;
struct {
select (Handshake.msg_type) {
case new_session_ticket: uint32 max_early_data_size;
case client_hello: Empty;
case encrypted_extensions: Empty;
};
} EarlyDataIndication;
--
Stephen Checkoway
_______________________________________________
TLS mailing list
TLS@ietf.org
https://www.ietf.org/mailman/listinfo/tls
