custom DoH request validation

[balboah]

This is a POC for #4323

[johnbelamaric]

So the idea then would be some sort of plugin that installs a new validation function? Like: doh path /my-weird-path with possible extensions like:

doh {
  path /foo
  read-timeout 100
}

Or something like that? The (completely made up) read-timeout would need more than a validator function of course but from a syntax point of view, is this what you are thinking?

If so, it seems reasonable to me. The doh plugin in this case would then publish the configured path as metadata by implementing the appropriate interface.

John

[balboah]

My idea is more like http middleware, but only for reading the requests and only one global middleware :)
I may have two clients on DoH where each uses their own dynamic uuid like https://example.com/<uuid>

Then my plugin (which doesn't have to be included in this repo) would extract this ID and possibly pass it along using the metadata plugin.
It may for example be for things such as using rewrite plugin to set this uuid in edns0 for special handling upstream:
rewrite edns0 local set 0xffee {uuid}

Others could potentially use this in their plugin for collecting HTTP specific stats such as which user agent is used.
It's not possible do to this afaik without something like these changes

[balboah]

Not exactly sure of the details in these other issues, but it might be possible to use this as a first step for hooking up plugins that supports #3460 and #4259 as well

[johnbelamaric]

Why do they use UUID in the path? Wouldn't it make more sense to use the standard path and put the UUID in a header?

In the latter case, this change would no longer be the same; instead of a validator, it would just be some sort of getter for the original HTTP request or some sort of inline parser function.

[balboah]

The reason why it can’t be a header is that the clients aren’t modified or under my control. They can be any client, as long as they can specify a custom path. Which most can

[johnbelamaric]

Ah, those pesky clients.

Ok. So in your case you will install a custom validator function that just checks a regex or something. So that's why my description above is not accurate for you, and we don't need the doh options plugin right now. Got it. You just want access to this in your external plugin.

This approach is fine with me.

[balboah]

Yes. This sounds spot on 🙂

[johnbelamaric]

Unless @miekg has some strong objections, I would merge this but it needs the following:

1. Add a comment that although this override is not used in-tree today, external plugins depend upon it. Otherwise it may get pulled later...
2. Add a test

[balboah]

@johnbelamaric up for grabs!

[codecov-io]

Codecov Report

Merging #4329 (5a60e52) into master (aac422f) will increase coverage by 0.50%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4329      +/-   ##
==========================================
+ Coverage   54.96%   55.46%   +0.50%     
==========================================
  Files         223      223              
  Lines        9918     9923       +5     
==========================================
+ Hits         5451     5504      +53     
+ Misses       4022     3963      -59     
- Partials      445      456      +11     

Impacted Files	Coverage Δ
core/dnsserver/config.go	0.00% <ø> (ø)
core/dnsserver/server_https.go	48.52% <100.00%> (+48.52%)	⬆️
core/dnsserver/server.go	26.71% <0.00%> (+14.50%)	⬆️
core/dnsserver/https.go	50.00% <0.00%> (+50.00%)	⬆️

---

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 aac422f...5a60e52. Read the comment docs.

[johnbelamaric]

/lgtm

[corbot]

Approved by johnbelamaric