cloudflare / odoh-client-go

Oblivious DoH client
MIT License
80 stars 19 forks source link

make TARGET_HTTP_MODE and PROXY_HTTP_MODE configurable from command line #12

Closed kimbo closed 3 years ago

kimbo commented 3 years ago

When I tried running this locally with odoh-server, it failed because it sends requests with https and my odoh-server was running with plain http. It'd be nice to be able to set the scheme (http/https) for the target and proxy without having to change commands/common.go and rebuild.

chris-wood commented 3 years ago

This would be a good thing to add, yeah. @kimbo, do you want to throw up a PR? If not, I'll take it.

kimbo commented 3 years ago

I'm good either way, though I may not get to it for a bit. Here are a few thoughts I had:

--proxy and --target could accept the full URL including the scheme (and the path), and then default to https and /dns-query (or whatever the default path is for the given command) when it's ommitted. Personally I'm used to curl's default of http when you don't specify the scheme, but in his case I think https is a fine default. (lmk if you have a different opinion about that)

Examples values/test cases for --proxy or --target flag for odoh command:

The above would apply to all the commands (doh, odoh, and odoconfig-fetch) with whatever default path they each have.

Not sure how the odoh config URL would look in an odoh lookup with a custom url (and their config isn't in a DNS record). Probably would need to be an additional flag for that.

Anyway, there are my thoughts. Not trying to make this change bigger than it has to be, just thinking out loud.

chris-wood commented 3 years ago

--proxy and --target could accept the full URL including the scheme (and the path), and then default to https and /dns-query (or whatever the default path is for the given command) when it's ommitted. Personally I'm used to curl's default of http when you don't specify the scheme, but in his case I think https is a fine default. (lmk if you have a different opinion about that)

That's a good suggestion. Defaulting to https would be my preference here.

Not sure how the odoh config URL would look in an odoh lookup with a custom url (and their config isn't in a DNS record). Probably would need to be an additional flag for that.

The config is under a .well-known, which is always at the path root (https://tools.ietf.org/html/rfc5785). If a custom URL was uploaded, I'd imagine the code would just construct the .well-known as + "/.well-known/odohconfigs".

kimbo commented 3 years ago

The config is under a .well-known, which is always at the path root (https://tools.ietf.org/html/rfc5785). If a custom URL was uploaded, I'd imagine the code would just construct the .well-known as + "/.well-known/odohconfigs"

That makes sense. I was imagining a scenario where someone had their odoh config stored under a different path, but maybe it'd be better not to support that 🤷‍♂️

chris-wood commented 3 years ago

That makes sense. I was imagining a scenario where someone had their odoh config stored under a different path, but maybe it'd be better not to support that 🤷‍♂️

We could always add that in the future. But yeah, for now, it seems like YAGNI.