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.

Sign up for GitHub

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

Jump to bottom

Improves odohconfig parsing #7

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Improves odohconfig parsing #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "odoh-client-rs"
version = "0.1.5"
version = "0.1.6"
authors = [ "Tanya Verma <tverma@cloudflare.com>" ]
edition = "2018"
license = "BSD-2-Clause"
Expand Down
22 changes: 21 additions & 1 deletion src/dns_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ pub async fn fetch_odoh_config(target: &str) -> Result<Vec<u8>> {
/// parse keys.
fn odohconfig_from_https(data: &[u8]) -> Result<Vec<u8>> {
let data_hex = hex::encode(data);
let vec = data_hex.split(ODOH_VERSION).collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem correct. Consider the following config:

data = "00010000010003026832000400086810f8f96810f9f9000600202606470000000000000000006810f8f92606470000000000000000006810f9f98001002e002cff0300280020000100010020128dfb25d235708df7031ff036bd82ec061c0877835186321e0c03e24f93213aff04deadbeef";

When parsed, this would return:

002cff0300280020000100010020128dfb25d235708df7031ff036bd82ec061c0877835186321e0c03e24f93213aff04deadbeef

and not:

002cff0300280020000100010020128dfb25d235708df7031ff036bd82ec061c0877835186321e0c03e24f93213a

In general, I'm not sure converting from a [u8] to hex string, and then parsing as a hex string, is a robust way to implement this function. (And shouldn't this capability be part of the odoh-rs library?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the intention, because when the deserialization to ObliviousDoHConfigs happens inside of odoh-rs that, it will automatically discard the ff04deadbeef. The whole point of this parser isn't to be perfect (that I will soon get once I get the DNS library updated since the maintainer said he would be happy to add it in), it's to be good enough for 99.9% of cases.

Copy link
Contributor Author

@tanyav2 tanyav2 Dec 18, 2020 •
edited

Choose a reason for hiding this comment

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

The whole hex thing along with this entire parsing hack will soon be removed anyway once the update is made, but in any case that is a different PR. The current PR for the most part is just adding testing, not doing much new.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, why do we do any parsing here? Why not pass the entire blob to odoh-rs and let it figure things out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need to remove all the stuff prior to 002cff03 since that cannot be parsed to odohconfig (those are fields like ipv6hint or ipv4hint). So currently I'm just trying to remove all the stuff before 002cff03 and let odoh-rs handle the rest (that was what this change was about). But we won't have to deal with the fields ipv4hint etc once the DNS library can recognize HTTPS records.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

002c is what odoh-rs uses to figure out the length it needs to consider, it can't use the beginning of the blob 0001 for instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. So this is a fine hack for now, but let's file an issue to come back and fix this when the upstream changes.

let vec = data_hex.splitn(2, ODOH_VERSION).collect::<Vec<_>>();
let first_len = vec.first().unwrap().len();
let odohconfig_len = vec.first().unwrap().to_string().split_off(first_len - 4);
let odohconfig = format!(
Expand Down Expand Up @@ -136,3 +136,23 @@ fn get_qtype(qtype: &str) -> Result<RecordType> {
};
Ok(rtype)
}

#[cfg(test)]
mod test {
use super::*;

#[test]
fn test_odohconfig_parser() {
// if odohconfig string does not contain multiple ODOH_VERSION
let mut data = "00010000010003026832000400086810f8f96810f9f9000600202606470000000000000000006810f8f92606470000000000000000006810f9f98001002e002cff0300280020000100010020f150567d5bfd7ffbb20f52b73cce923f0654e37265d7065dc5d6bfc8912b3e5e";
let mut odohconfig = odohconfig_from_https(&hex::decode(data).unwrap()).unwrap();
let mut expected_odohconfig = "002cff0300280020000100010020f150567d5bfd7ffbb20f52b73cce923f0654e37265d7065dc5d6bfc8912b3e5e";
assert_eq!(expected_odohconfig, hex::encode(odohconfig));

// if odohconfig string contains multiple ODOH_VERSION
data = "00010000010003026832000400086810f8f96810f9f9000600202606470000000000000000006810f8f92606470000000000000000006810f9f98001002e002cff0300280020000100010020128dfb25d235708df7031ff036bd82ec061c0877835186321e0c03e24f93213a";
odohconfig = odohconfig_from_https(&hex::decode(data).unwrap()).unwrap();
expected_odohconfig = "002cff0300280020000100010020128dfb25d235708df7031ff036bd82ec061c0877835186321e0c03e24f93213a";
assert_eq!(expected_odohconfig, hex::encode(odohconfig));
}
}