Skip to content

Restrict Host header values for TequilAPI - #3985

Merged
Snawoot merged 4 commits into
masterfrom
restrict_api_host
Sep 27, 2021
Merged

Restrict Host header values for TequilAPI#3985
Snawoot merged 4 commits into
masterfrom
restrict_api_host

Conversation

@Snawoot

@Snawoot Snawoot commented Sep 24, 2021

Copy link
Copy Markdown
Contributor

Closes https://github.com/mysteriumnetwork/environment/issues/728

@Waldz this PR introduced potentially breaking change. Requests with Host header not in whitelist will be rejected. If you invoke Tequilapi using some domain name, you may need to adjust tequilapi.allowed-hostnames config parameter or use CLI argument --tequilapi.allowed-hostnames.

Value examples:

  • ".localhost, localhost, .localdomain" - allow Host header values with localhost domain, all localhost subdomains and localdomain subdomains (but not localdomain itself!).
  • "." - allows all subdomains of root domain (virtually all domains).

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #3985 (db52fb9) into master (c097398) will increase coverage by 0.03%.
The diff coverage is 64.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3985      +/-   ##
==========================================
+ Coverage   43.45%   43.48%   +0.03%     
==========================================
  Files         325      326       +1     
  Lines       17100    17148      +48     
==========================================
+ Hits         7430     7457      +27     
- Misses       8915     8935      +20     
- Partials      755      756       +1     
Impacted Files Coverage Δ
config/flags_node.go 9.40% <0.00%> (-0.17%) ⬇️
tequilapi/middlewares/http_middlewares.go 11.76% <0.00%> (-88.24%) ⬇️
tequilapi/http_api_server.go 78.04% <100.00%> (+0.54%) ⬆️
utils/domain/whitelist.go 100.00% <100.00%> (ø)
core/policy/oracle.go 89.18% <0.00%> (-2.71%) ⬇️
core/discovery/discovery.go 63.49% <0.00%> (-1.59%) ⬇️

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 c097398...db52fb9. Read the comment docs.

Comment thread config/flags_node.go
Value: "127.0.0.1",
}
// FlagTequilapiAllowedHostnames Restrict hostnames in requests' Host header to following domains.
FlagTequilapiAllowedHostnames = cli.StringFlag{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this be Hidden: true as we already have a bunch of flags that are printed on --help and this seems like an average person should not care about it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Average person doesn't run node CLI!

There are two options:

  • We expose this parameter visible and it just adds another line in help output.
  • We hide it and whoever uses it will have to open source code to enable remote access by hostname to TequilAPI, otherwise receiving 403 for unknown reason.

I picked second option.

@Snawoot
Snawoot merged commit ec39f71 into master Sep 27, 2021
@Snawoot
Snawoot deleted the restrict_api_host branch September 27, 2021 08:15
Comment thread config/flags_node.go
FlagTequilapiAllowedHostnames = cli.StringFlag{
Name: "tequilapi.allowed-hostnames",
Usage: "Comma separated list of allowed domains. Prepend value with dot for wildcard mask",
Value: ".localhost, localhost, .localdomain",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks complicated compared to controlling with bind address: --tequilapi.address=0.0.0.0:4050.
And now i need to to know hostname thru which I'm gonna call TequiAPI. --tequilapi.address=0.0.0.0 --tequilapi.allowed-hostnames=myst-tunnel-0.

Talking about Docker case here, maybe --tequilapi.allowed-hostnames=. should be default inside Docker?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks complicated compared to controlling with bind address

because it's not a bind address, it's list of domains and suffixes to allow. This syntax is identical to similar list in Firefox proxy settings (list domains to bypass). Pretty much common I'd say.

Talking about Docker case here, maybe --tequilapi.allowed-hostnames=. should be default inside Docker?

Usually docker deployments of node do not expose TequilAPI directly, but there is no way to check it from the inside. Any assumptions on that subject are rather unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants