[WiP] api: annotations for fields that should be set for untrusted en… #11058

htuch · 2020-05-05T01:33:26Z

…vironments.

This PR is an early prototype of a new flow for indicating that fields
needs to be set in the presence of untrusted downstreams/upstreams.

Based on the (yet to be merged)
cncf/udpa#28, a worked example of a YAML example
in bootstrap.proto for overload_manager is provided. The new security
annotations are validated during docs build for correctness and used to
generate inline docs where fields are defined.

This is intended to supplement and eventually replace
https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#best-practices-edge.

Work still to be done:

Decide if this is the approach we want to take.
Split out the rules_python pip3 import support from WORKSPACE into a distinct PR that integrates with our envoy import deps macros.
Fix protoformat toolchain to work with fields inside the annotations.
Annotate remaining fields from
https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#best-practices-edge
Cleanup protodoc Python for style reasons and make error handling more robust.

Risk level: Low
Testing: Docs inspection

Part of #9087

Signed-off-by: Harvey Tuch htuch@google.com

…vironments. This PR is an early prototype of a new flow for indicating that fields needs to be set in the presence of untrusted downstreams/upstreams. Based on the (yet to be merged) cncf/udpa#28, a worked example of a YAML example in bootstrap.proto for overload_manager is provided. The new security annotations are validated during docs build for correctness and used to generate inline docs where fields are defined. This is intended to supplement and eventually replace https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#best-practices-edge. Work still to be done: - Decide if this is the approach we want to take. - Fix protoformat toolchain to work with fields inside the annotations. - Annotate remaining fields from https://www.envoyproxy.io/docs/envoy/latest/configuration/best_practices/edge#best-practices-edge - Cleanup protodoc Python for style reasons and make error handling more robust. Risk level: Low Testing: Docs inspection Part of envoyproxy#9087 Signed-off-by: Harvey Tuch <htuch@google.com>

repokitteh-read-only · 2020-05-05T01:33:31Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11058 was opened by htuch.

see: more, trace.

WORKSPACE

htuch · 2020-05-05T01:36:20Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+        configure_for_untrusted_upstream: true
+        example_configs {
+          example:
+            "  refresh_interval: 0.25s\n"


Protobuf strings don't support multi-line, so you have this ugliness with "\n". The alternative is we have a text proto fragment here buried inside an Any, but when I tried that, it caused the protobuf compiler to hang, so we'd need to debug that. The other downside of text proto is that it's an unfamiliar syntax to most folks.

@lizan @mattklein123 WDYT about the best approach to handling this? I'm almost starting to think that putting examples inline might not be the greatest idea, but in this prototype it does seem to work reasonably well, see the generated docs. From an end user UX, it's helpful to have these directly attached to the fields.

I would start with annotations and possibly a comment to sec config docs. The issue is 1 size does not fit all limits and timeouts when operating with untrusted downstreams and upstreams. Typical example is H2 flow control windows; setting them too small will result in poor performance when the remote endpoint has high RTT as is typical for a downstream client somewhere in the internets (or a cruise ship).

@antoniovicente what do you mean by annotations here? Does this example constitute an annotation from your view point?

I was referring to things like configure_for_untrusted_upstream. Those are great. I think example config needs to be reconsidered; we can always introduce it later if we decide it is necessary.

Yeah, I think we can drop the experiment here on inline examples due to size and poor proto representation capabilities. I'll switch to the simpler configure_foo annotations.

sgtm. I do like the idea of baking in defaults for different "profiles" but maybe we can figure that out separately.

I have an alternative idea on how to achieve the programmatic association of examples or defaults with API protos. We could use a similar approach to what I want to do for proxy-specific docs comments. We start with the normal protos as defined today. We then add a manifest file, which is a map from to comment or example, e.g.

envoy.config.listener.v3.Listener.foo_field: "foo isn't implemented yet in super-duper proxy. envoy.config.listener.v3.Listener.bar: "bar is alpha in super-duper proxy.

Protodoc can then glue this manifest as it generates docs for each proto.

We can do a similar thing for these secure default examples, getting the same end results as in this PR, but without modifying any of the protos, and also by using natural YAML syntax.

WDYT? I'm hoping I can kill two birds with one stone and address @alyssawilk ask here.

Yeah that looks neat and a great step in the right direction! As I said offline to @htuch I think also with this Python close we are very close to having a Python sphinx plugin that can check examples inline. It would be really awesome to be able to add those directly in the RST files and/or link out to include things in the manfiest.

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2020-05-05T10:54:51Z

For reference, see the overload_manager field definition at https://342632-65214191-gh.circle-artifacts.com/0/generated/docs/api-v3/config/bootstrap/v3/bootstrap.proto.html for an example of output.

antoniovicente · 2020-05-05T16:48:04Z

/assign @antoniovicente

htuch · 2020-05-07T20:21:57Z

WORKSPACE

+
+# Load the central repo's install function from its `//:requirements.bzl` file,
+# and call it.
+load("@protodoc_deps//:requirements.bzl", "pip_install")


@lizan I'm trying to roll this into envoy_dependency_imports() and envoy_dependencies(). Unfortunately, I think this requires a 3 stage load(..)/invoke function from load sequence. The core issue is that we can't do the pip3_import in envoy_dependencies(), since it needs @rules_protobuf, which itself is only loaded from repository_locations.bzl in repositories.bzl.

Any ideas on how to do this in 2 stages? If not, I think we need to add a envoy_dependencies_extra() stage to this flow.

htuch · 2020-05-11T22:24:40Z

Closing this out as it's replaced by #11151.

This PR replaces envoyproxy#11058, taking a slightly different approach. We utilize field options to annotate fields that should be set for untrusted environments with [configure_for_untrusted_downstream, configure_for_untrusted_downstream]. Defaults are provided out-of-band, in a manifest files in docs/edge_defaults_manifest.yaml. Protodoc glues the manifest and options together when generating field documentation, providing an additional notice for sensitive fields. This PR depends on envoyproxy#11108 first merging to provide the pip3 build infrastructure. Risk level: Low (docs only). Testing: Inspection of generated docs. Signed-off-by: Harvey Tuch <htuch@google.com>

This PR replaces #11058, taking a slightly different approach. We utilize field options to annotate fields that should be set for untrusted environments with [configure_for_untrusted_downstream, configure_for_untrusted_downstream]. Defaults are provided out-of-band, in a manifest files in docs/edge_defaults_manifest.yaml. Protodoc glues the manifest and options together when generating field documentation, providing an additional notice for sensitive fields. This PR depends on #11108 first merging to provide the pip3 build infrastructure. Risk level: Low (docs only). Testing: Inspection of generated docs. Signed-off-by: Harvey Tuch <htuch@google.com>

repokitteh-read-only bot added the api label May 5, 2020

htuch commented May 5, 2020

View reviewed changes

Flexible YAML loaders.

8fe6338

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch mentioned this pull request May 5, 2020

annotations: introduce security annotations. cncf/udpa#28

Merged

htuch requested review from mattklein123 and antoniovicente May 5, 2020 01:42

htuch assigned mattklein123 May 5, 2020

htuch mentioned this pull request May 5, 2020

Jinja2 dependency upgrade: 2.10.3 -> 2.11.2 #11023

Closed

repokitteh-read-only bot assigned antoniovicente May 5, 2020

mattklein123 added the waiting label May 5, 2020

antoniovicente mentioned this pull request May 5, 2020

[API] Breaking change with RFC header constraints #10318

Closed

3 tasks

alyssawilk mentioned this pull request May 6, 2020

Deprecated extension documentation link is broken #11078

Closed

htuch mentioned this pull request May 6, 2020

Manifest based documentaton extensions #11085

Closed

htuch commented May 7, 2020

View reviewed changes

htuch mentioned this pull request May 11, 2020

api: manifest based edge default documentation. #11151

Merged

htuch closed this May 11, 2020

htuch deleted the security-annotations branch May 11, 2020 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WiP] api: annotations for fields that should be set for untrusted en… #11058

[WiP] api: annotations for fields that should be set for untrusted en… #11058

htuch commented May 5, 2020 •

edited

Loading

repokitteh-read-only bot commented May 5, 2020

htuch May 5, 2020

htuch May 5, 2020

antoniovicente May 5, 2020

htuch May 5, 2020

antoniovicente May 5, 2020

htuch May 5, 2020

mattklein123 May 5, 2020

htuch May 5, 2020

mattklein123 May 5, 2020

htuch May 6, 2020

htuch commented May 5, 2020

antoniovicente commented May 5, 2020

htuch May 7, 2020

htuch May 7, 2020

htuch commented May 11, 2020

[WiP] api: annotations for fields that should be set for untrusted en… #11058

[WiP] api: annotations for fields that should be set for untrusted en… #11058

Conversation

htuch commented May 5, 2020 • edited Loading

repokitteh-read-only bot commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented May 5, 2020

antoniovicente commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented May 11, 2020

htuch commented May 5, 2020 •

edited

Loading