charts/nginz: Add restrictive CORS headers to all URLs and explicitly… #1630
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arianvp
force-pushed
the
allowlist_explicit
branch
2 times, most recently
from
208c7b5 to
4bc520b
Compare
|
Will have to test this, and write extensive CHANGELOG entry before merging this. We also have to update the wire installation docs so that new users are sure to not forget this! To be changed: |
arianvp
force-pushed
the
allowlist_explicit
branch
from
4bc520b to
b542f13
Compare
|
Needs rebase |
arianvp
force-pushed
the
allowlist_explicit
branch
from
ffd4ee9 to
26d73d0
Compare
|
@jmatsushita this is ready for another review |
…tly allowlist allowed origins There is no reason not to have restrictive CORS on all endpoints; given our API is currently only consumed by internal services that we know apriori. We can loosen this restriction again in the future (e.g. when we have something like OAuth integration). We also move to an explicit allowlist instead of trusting the entirety of `external_env_domain`. Especially on-prem people might have many more services running on their base domain and we as Wire have no control over it. We do not want other services not in our control to be a vector for e.g. XSS attacks. Hence we require the user of our charts to explicitly allowlist all the expected origins. We will update the docs to guide people into making this update.
If people needs to adjust the subdomains, they can override them in their values.yaml But this makes sure things just 'work' out of the box in common case
The previous commits adding CORS configuration set the access control header to unconditionally return the Origin header sent with the request. This would cause fail-open behaviour, where any Origin sent by a client would be allowed. Instead, the $cors_header variable is used, as this is specifically set based on the request Origin header so that only origins which are explicitly in the Helm chart's allow list configuration may make cross-origin requests to nginz API endpoints.
sysvinit
force-pushed
the
allowlist_explicit
branch
from
19a9865 to
e9a5516
Compare
sysvinit
requested review from
jschaul and
flokli
and removed request for
jmatsushita
jschaul
approved these changes
Jan 26, 2022
There was a problem hiding this comment.
Minor change: we use and have been using 'account' (without s) everywhere in docs.wire.com and our own environments (except for accounts.wire.com - here we can do an override)
Looks fine, let's merge and deploy this to some of our environments.
Could you, after merging this, also add this to wire-docs somewhere?
Merged
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add restrictive CORS headers to all origins and explicitly allowlist allowed origins
There is no reason not to have restrictive CORS on all endpoints; given
our API is currently only consumed by known services. We can loosen this
restriction again in the future (e.g. when we have something like OAuth
integration).
We also move to an explicit allowlist instead of trusting the entirety
of
external_env_domain. Especially on-prem people might have many moreservices running on their base domain and we as Wire have no control
over it. We do not want other services not in our control to be a vector
for e.g. XSS attacks. Hence we require the user of our charts to
explicitly allowlist all the expected origins. We will update the docs
to guide people into making this update if needed.
Checklist