search

LinkTed / dns-message-parser

Rust libary to encode and decode DNS packets
BSD 3-Clause "New" or "Revised" License
24 stars 8 forks source link

Support SVCB and HTTPS Resource Records #14

Closed l0s closed 3 years ago

l0s commented 3 years ago

This adds support for servicing binding resource records: SVCB, type 64 and HTTPS, type 65. This adds two new resource record enum variants: SVCB and HTTPS. However, since the structure of those records are nearly identical, only a single ServiceBinding struct is introduced. Tests for encoding, decoding, and presentation formatting were added based on the test vectors here: https://github.com/MikeBishop/dns-alt-svc/blob/master/draft-ietf-dnsop-svcb-https.md .

References:

Resolves: #13

LinkTed commented 3 years ago

Thanks for your PR. I hope the architecture of the crate was easy to understand 😀.

Could you move the test, for example the tests in src/decode/rr/draft_ietf_dnsop_svcb_https.rs or src/rr/draft_ietf_dnsop_svcb_https.rs into the tests directory. Create new files, for example tests/encode.rs, tests/decode.rs or tests/rr.rs, if it does not fit in the current files.

Can you move the match in the file src/decode/rr/draft_ietf_dnsop_svcb_https.rs at line 32 into a separate function (rr_service_parameter).

Is a ServiceParameter::ALPN with the multiple of the same value correct, for example twice of vec!["h2".to_string(), "h2".to_string()], if not, then could you change it to a set?

l0s commented 3 years ago

Thanks for the feedback @LinkTed . I'll work on moving the tests and refactoring the service parameter match statement. I'll also look into the RFC requirements for ALPN ID uniqueness and ordering. Note that in that same vein, the RFC does require that service parameter keys be unique and sorted. I considered using a BTreeSet for this but opted to use a Vec instead. A Vec allows developers to create records declaratively. I sort the records (but do not check for duplicates) prior to serialising and prior to generating the presentation format. Should I change how service parameters are stored?

l0s commented 3 years ago

@LinkTed, section 6.1 of the RFC ("alpn" and "no-default-alpn") does not disallow duplicate ALPN IDs, although I cannot think of a legitimate use case for doing that. It does not mandate any specific order either. However, RFC7301 - TLS Application-Layer Protocol Negotiation Extension indicates that clients should present protocols in decreasing order of preference. Systems that adopt DNS service binding records for ALPN may wish to adopt the TLS Extension behaviour as a convention. I think it would make sense to retain a Vec for this use case. What do you think?

Regarding moving the unit tests into the tests directory, I found that I cannot move them without violating visibility constraints. For example, Header is not visible to the tests. I can work around this by testing full Dns records. They would not really be unit tests any more. Alternatively, I can keep them in the src tree, but follow the same pattern as src/decode/rr/tests.rs, breaking them into a separate file. Which would you prefer?

Lastly, I pushed a change to extract the rr_service_parameter method.

LinkTed commented 3 years ago

... the RFC does require that service parameter keys be unique and sorted. ... Should I change how service parameters are stored?

Yes, please change it to BTreeSet

... does not disallow duplicate ALPN IDs ... I think it would make sense to retain a Vec for this use case. What do you think?

Keep it as Vec if the RFC does not disallow duplicate ALPN IDs.

Regarding moving the unit tests into the tests directory, ... I can keep them in the src tree, but follow the same pattern as src/decode/rr/tests.rs, breaking them into a separate file. Which would you prefer?

Okay, move the tests into src/decode/rr/tests.rs or src/encode/rr/tests.rs, if possible, but change the prefix to test_service_binding_XXX

Thank you for your contribution.

l0s commented 3 years ago

@LinkTed, I made the changes you requested. Let me know if there is anything else I can do.

codecov-commenter commented 3 years ago

Codecov Report

Merging #14 (868c6c1) into master (aaef7ac) will increase coverage by 0.18%. The diff coverage is 91.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   91.03%   91.22%   +0.18%     
==========================================
  Files          86       91       +5     
  Lines        2812     3623     +811     
==========================================
+ Hits         2560     3305     +745     
- Misses        252      318      +66
Impacted Files Coverage Δ
src/decode/error.rs 5.74% <0.00%> (-0.14%) :arrow_down:
src/question.rs 100.00% <ø> (ø)
src/decode/rr/enums.rs 94.18% <50.00%> (-1.06%) :arrow_down:
src/encode/rr/enums.rs 90.90% <50.00%> (-1.55%) :arrow_down:
src/rr/draft_ietf_dnsop_svcb_https.rs 68.75% <68.75%> (ø)
src/rr/enums.rs 87.24% <75.00%> (-0.53%) :arrow_down:
src/encode/rr/draft_ietf_dnsop_svcb_https.rs 85.10% <85.10%> (ø)
src/decode/rr/draft_ietf_dnsop_svcb_https.rs 87.50% <87.50%> (ø)
src/encode/rr/tests.rs 99.55% <99.55%> (ø)
src/decode/rr/tests.rs 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update aaef7ac...868c6c1. Read the comment docs.

LinkTed commented 3 years ago

Super, all tests were passed. Could you squash your commits?

l0s commented 3 years ago

@LinkTed I squashed the commits. Thanks!