Improves odohconfig parsing #7
Conversation
tanyav2
force-pushed
the
tanya/better-parser
branch
2 times, most recently
from
f5e2948
to
213551a
Compare
tanyav2
force-pushed
the
tanya/better-parser
branch
from
213551a
to
84b9b1e
Compare
| @@ -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<_>>(); | |||
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Adds tests for
odohconfigparsing, so when thetrust-dns-clientlibrary adds support for HTTPS, we can reuse these tests.Also improves the parsing logic to only consider the first instance of
ODOH_VERSIONin the HTTPS string, hence avoiding splits ifff03(akaODOH_VERSION) shows up again in the randomly generated public key.