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.

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

Fix dohpath parsing/stringify #1366

Merged
merged 3 commits into from May 10, 2022
Merged

Conversation

shane-kerr
Copy link
Contributor

@shane-kerr shane-kerr commented Apr 14, 2022

I noticed that the brand-new dohpath support has a similar problem to the one addressed in PR #1361 - non-printable characters are not handled properly.

Luckily the parameter is much simpler than the alpn parameter. I refactored the parse/stringify SVCBLocal out and use that for both types.

@shane-kerr shane-kerr requested review from miekg and tmthrgd as code owners Apr 14, 2022
svcb.go Outdated
s.Template = b
template, err := svcbParseParam(b)
if err != nil {
return fmt.Errorf("dns: svcblocal: svcb private/experimental key %w", err)
Copy link
Contributor

@ainar-g ainar-g Apr 14, 2022

Choose a reason for hiding this comment

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

This message should probably be about svcbdohpath?

Copy link
Contributor Author

@shane-kerr shane-kerr Apr 14, 2022

Choose a reason for hiding this comment

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

Whoops! Cut & paste error. I've fix this, thanks for spotting!

@miekg
Copy link
Owner

miekg commented Apr 14, 2022

@shane-kerr probably makes sense to rework #1363 to make that more generic to handle all this stuff with 1 or 2 functions? Or does this obsolete that PR?

@shane-kerr
Copy link
Contributor Author

shane-kerr commented Apr 14, 2022

@shane-kerr probably makes sense to rework #1363 to make that more generic to handle all this stuff with 1 or 2 functions? Or does this obsolete that PR?

@miekg when I looked at #1363 I considered using a single function for parsing the alpn and the local stuff, but the encoded comma-separated list means that the rules are actually quite different. On my first attempt, I used the SVCBLocal version and then tried decoding and splitting the result, but it was both inefficient and messy. I could possibly try that again, but I'm pretty happy with the resulting code. Similarly the stringify needs to encode any , and \ that it encounters, so it's not really directly usable.

This PR is different, since the handling for parsing and stringify is actually identical for the dohpath and the local stuff. I'm pretty sure the two PR don't collide, so can both be merged usefully.

If we really want to refactor code to remove redundancy, we could look at pulling out the escaping functionality in sprintTxt() into a separate routine that operates on a single string, and doesn't add " around it. Then we could eliminate the svcbParamToStr() function I introduced completely. There may also be some clever way to get the parsing from existing code, although that's likely more work. Let me know what you think.

miekg
miekg approved these changes May 10, 2022
@miekg
Copy link
Owner

miekg commented May 10, 2022

I'll hold off merging, until #1363 is in, might result in conflicts otherwise?

@shane-kerr
Copy link
Contributor Author

shane-kerr commented May 10, 2022

Yes, makes sense. I don't expect conflicts, but we'll see!

@miekg miekg merged commit bfcbf0f into miekg:master May 10, 2022
4 checks passed
aanm pushed a commit to cilium/dns that referenced this issue Jul 29, 2022
* dohpath escaped in String(), and parsed such values

* Update the test for dohpath with escaping

* Fix cut & paste error with svcdohpath error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants