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.

Sign up for GitHub

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

Jump to bottom

bootstrap and xDS should have separate support for allow unknown fields #6651

Closed
fuqianggao opened this issue Apr 18, 2019 · 6 comments · Fixed by #7857
Closed

bootstrap and xDS should have separate support for allow unknown fields #6651

fuqianggao opened this issue Apr 18, 2019 · 6 comments · Fixed by #7857
Assignees
@htuch
Labels
area/security enhancement Feature requests. Not bugs or questions.

Comments

@fuqianggao
Copy link
Contributor

Currently, --allow-unknown-fields is false by default. Any unknown fields in bootstrap will be caught early and xds response sent by management server with unknown fields will be rejected.

It would be helpful if we can have separate control of allow unknown fields for bootstrap and xds. This way, typos in bootstrap can still be caught early by not allowing unknown fields on bootstrap; while management server can set new fields in the xds and not worry that an Envoy running in an older build will reject the config by allowing unknown fields on xds.
@htuch
Copy link
Member

htuch commented Apr 18, 2019

+1, this is really important when dealing with security fix releases as well.

@htuch htuch added enhancement Feature requests. Not bugs or questions. help wanted Needs help! area/security labels Apr 18, 2019
@mattklein123
Copy link
Member

+1 makes sense to me.

@htuch htuch self-assigned this May 22, 2019
This was referenced May 24, 2019
Closed
Merged
htuch added a commit to htuch/envoy that referenced this issue May 30, 2019
@htuch
config: dependency injection of message validation.
2d1cd91 
This is the plumbing to support envoyproxy#6651 and envoyproxy#6818, where we remove the global control over message
validation type and put it under the control over a new injectable interface. It's pretty horrible
IMHO that there is this much churn; the closest major context object that we could associate with is
Api, but it's not part of the filter API as such and we will want to later inject different
validators at different points, depending on whether we are reading the static bootstrap or on xDS.

No new behavior should be added in this PR.

Risk level: Low.
Testing: bazel test //test/...

Signed-off-by: Harvey Tuch <[email protected]>
htuch added a commit that referenced this issue May 31, 2019
@htuch
config: dependency injection of message validation. (#7072)
8d1ad35 
This is the plumbing to support #6651 and #6818, where we remove the global control over message
validation type and put it under the control of a new injectable interface. It's pretty horrible
IMHO that there is this much churn; the closest major context object that we could associate with is
Api.

The main from the new dependency is that we can:

1. Inject different validators, e.g. one for static config when ingesting bootstrap resources, another for xDS.
2. Validation state, so that we can reach back to the server stats for the purpose of tracking unknown fields.
This PR introduces a visitor pattern for the validator and starts to do the threading needed for dependency injection here.

It's possible that we could extend this visitor for deprecated fields in the future to provide finer grained control over these alongside unknown, if there is a use case.

No new behavior should be added in this PR.

Risk level: Low.
Testing: bazel test //test/...

Signed-off-by: Harvey Tuch <[email protected]>
@htuch htuch mentioned this issue Jun 7, 2019
Closed
@stale
Copy link

stale bot commented Jun 21, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 21, 2019
@htuch
Copy link
Member

htuch commented Jun 24, 2019

Still in progress on a low priority thread.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 24, 2019
@stale
Copy link

stale bot commented Jul 24, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 24, 2019
@stale
Copy link

stale bot commented Jul 31, 2019

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

@stale stale bot closed this as completed Jul 31, 2019
@htuch htuch reopened this Jul 31, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 31, 2019
htuch added a commit to htuch/envoy that referenced this issue Aug 7, 2019
@htuch
config: distinct CLI options for strict/permissive checking of static…
f923a18 
…/dynamic config.

As per envoyproxy#6651, this PR plumbs in CLI options to allow independent control over static/dynamic unknown
field validation.

The defaults are the same for static as today (strict) and for dynamic we are by default permissive.
This permits easy rollout of new API minor versions, including those related to security fixes.

Fixes a regression that occurred in envoyproxy#7200 where strict/permissive checking CLI options were
inverted.

Risk level: Low (strictly more permissive by default)
Testing: additional unit and integration tests added, exercising both permissive/strict checking
  over various parts of the API (bootstrap, listeners, clusters, xDS, network filters, etc).

Fixes envoyproxy#6651

Signed-off-by: Harvey Tuch <[email protected]>
@htuch htuch mentioned this issue Aug 7, 2019
Merged
htuch added a commit that referenced this issue Aug 19, 2019
@htuch
config: distinct CLI options for strict/permissive checking of static…
0418a85 
…/dynamic config. (#7857)

As per #6651, this PR plumbs in CLI options to allow independent control over static/dynamic unknown
field validation.

The defaults are the same for static as today (strict) and for dynamic we are by default permissive.
This permits easy rollout of new API minor versions, including those related to security fixes.

Fixes a regression that occurred in #7200 where strict/permissive checking CLI options were
inverted.

As per #6818, added stats/warning for any unknown fields encountered.

Risk level: Low (strictly more permissive by default)
Testing: additional unit and integration tests added, exercising both permissive/strict checking
over various parts of the API (bootstrap, listeners, clusters, xDS, network filters, etc).

Fixes #6651
Fixed #6818

Signed-off-by: Harvey Tuch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@htuch htuch

Labels
area/security enhancement Feature requests. Not bugs or questions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

config: distinct CLI options for strict/permissive checking of static… htuch/envoy
3 participants
@mattklein123 @htuch @fuqianggao