Conversation
|
hi @chifflier, would you be available to review this pull request when you have a moment? |
chifflier
left a comment
chifflier
left a comment
There was a problem hiding this comment.
Hi,
overall the pull request is interesting and I'm in favor in adding the support for fragmented message.
I have some questions about how this should be reflected in the API and/or structs. There are several choices, but even if we keep the current API we should probably add documentation to show how the caller can handle different cases (all fragments received, or not yet received, etc.)
| false => ext_len as usize, | ||
| true => min(ext_len as usize, i.len()), | ||
| }; | ||
| let (i, ext) = opt(complete(take(ext_len)))(i)?; |
There was a problem hiding this comment.
I see the logic here, but since ext_len is lost when returning, how will the caller be aware that the message is partial?
Not sure how if this can cause problems.
Shouldn't we either return something, or maybe store ext_len in the CH?
src/dtls.rs
Outdated
| TlsHandshakeType::Certificate => match allow_partial { | ||
| false => parse_dtls_handshake_msg_certificate(raw_msg), | ||
| true => parse_partial_dtls_handshake_msg_certificate(raw_msg), | ||
| }, |
There was a problem hiding this comment.
comment/discussion:
The match here could be avoided be directly calling parse_tls_certificate_inner, something like:
let (rem,cert) = parse_tls_certificate_inner(raw_msg, allow_partial)?;
Ok((rem, DTLSMessageHandshakeBody::Certificate))This reduces the number of if/sub-functions and makes the allow_partial explicit as argument, at the cost of adding code to map and add the Ok. Maybe ? after the match should be moved in each branch to avoid that. The code would become
let (_, body) = match msg_type {
//...
let (rem,cert) = parse_tls_certificate_inner(raw_msg, allow_partial)?;
(rem, DTLSMessageHandshakeBody::Certificate)
// ...
};Could be done in a later PR
src/dtls.rs
Outdated
| /// DTLS Client Hello allow partial | ||
| // Section 4.2 of RFC6347 | ||
| fn parse_partial_dtls_client_hello(i: &[u8]) -> IResult<&[u8], DTLSMessageHandshakeBody<'_>> { | ||
| parse_dtls_client_hello_inner(i, true) | ||
| } |
There was a problem hiding this comment.
If I'm correct, the client hello can only be fragmented in the extensions. This should be added to the function documentation
| parse_dtls_record_with_header_inner(i, hdr, false) | ||
| } | ||
|
|
||
| pub fn parse_partial_dtls_record_with_header<'i>( |
There was a problem hiding this comment.
Same as above: how can be caller know the parsed message is partial?
|
Indeed, the caller cannot know whether the message is partial. Or we could keep the original API and record whether the message is partial in the returned structs. Is this better? |
At the moment I'd be tempted to:
|
|
@traceflight just wondering, do you want to update your PR according to the last comments ? Or submit a new one, if you think this is more appropriate? |
Yes, I will update the PR later according to the last comments. |
|
I have added a flag to some types of handshake message, but i'm not sure if it is appropriate. |
2 participants
Related issue #84