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

Add correct language for --rest-api-host-allowlist #610

Merged
merged 20 commits into from
Oct 24, 2024

Conversation

joaniefromtheblock
Copy link
Contributor

Fixes #599

Copy link

vercel bot commented Oct 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
doc-teku ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 23, 2024 11:19pm

@joaniefromtheblock
Copy link
Contributor Author

@rolfyone Please review

@joaniefromtheblock joaniefromtheblock self-assigned this Oct 4, 2024
Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

a couple of little things but close :)

docs/reference/cli/index.md Outdated Show resolved Hide resolved
docs/reference/cli/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM


:::tip

To allow all hostnames, use "*". We don't recommend allowing all hostnames for production environments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To allow all hostnames, use "*". We don't recommend allowing all hostnames for production environments.
To allow all hostnames, use "*". However, we don't recommend this for production environments.

Copy link
Contributor Author

@joaniefromtheblock joaniefromtheblock Oct 22, 2024

Choose a reason for hiding this comment

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

Other parts of the doc use the original wording, so I repeated it for consistency. I think being more specific instead of using 'this' could be more helpful.

This flag restricts the server's responding addresses, but not the client access.

By default, Teku's REST API server responds only to requests where the `Host` header matches `localhost` or `127.0.0.1`.
If you specify values, the server will only respond to requests where the `Host` header matches one of the specified hosts or IP addresses.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have question: not yet clear to me whether the example above is necessary, i.e. the moment you add a specified value, you then override both defaults and need to provide these explicitly (as per the example line 2772 above) OR whether the example is providing unnecessary data which are served by default...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m4sterbunny if they provide the option without values, they should get an error

|---------------|-----------|-----------|--------|
| Listen on all IP addresses and allow all hosts | `rest-api-interface="0.0.0.0"` | `rest-api-host-allowlist=["*"]` | Enables connections from any address, such as `localhost` (`127.0.0.1`) or `10.0.0.1`. |
| Listen on a specific IP address (`10.0.0.1`) and allow all hosts | `rest-api-interface="10.0.0.1"` | `rest-api-host-allowlist=["*"]` | Only the specified IP (`10.0.0.1`) can connect, and attempts from `localhost` (`127.0.0.1`) will fail. |
| Listen on all IP addresses but allow only `localhost` | `rest-api-interface="0.0.0.0"` | `rest-api-host-allowlist=["127.0.0.1"]` | Only `localhost` (`127.0.0.1`) can connect; other IP addresses (e.g., `10.0.0.1`) will receive a 403 error. |
Copy link
Contributor

Choose a reason for hiding this comment

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

personally, I believe that e.g. is a win here vs longer options and would disregard vale, however, most reviewers seem very determined to see all these gone. WDYT?

docs/reference/rest.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bgravenorst bgravenorst left a comment

Choose a reason for hiding this comment

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

Minor nit. Otherwise lgtm

docs/reference/cli/index.md Show resolved Hide resolved
@joaniefromtheblock joaniefromtheblock merged commit a800f3c into main Oct 24, 2024
11 of 13 checks passed
@joaniefromtheblock joaniefromtheblock deleted the update-allowlist-flag branch October 24, 2024 13:57
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.

--rest-api-host-allowlist description discrepancy
4 participants