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

Extension maturity checklist #12962

Closed
htuch opened this issue Sep 2, 2020 · 5 comments · Fixed by #12976
Closed

Extension maturity checklist #12962

htuch opened this issue Sep 2, 2020 · 5 comments · Fixed by #12976
Labels
area/security enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@htuch
Copy link
Member

htuch commented Sep 2, 2020

While we now have explicit extension security postures, there is no systematic way to govern how extensions are promoted from untrusted to trusted, i.e. when they are considered robust to downstream or upstream; we generally rely on Envoy maintainer intuition and burn time. We should probably have a checklist providing guidelines on how this can be done in a consistent way.

Some ideas:

  • Does the extension have fuzz coverage? If it's only receiving fuzzing courtesy of the generic listener/network/HTTP filter fuzzers, does it have a dedicated fuzzer for any parts of the code that would benefit?
  • Does the extension have unbounded internal buffering? Does it participate in flow control via watermarking as needed?
  • Does the extension have at least one deployment with live untrusted traffic for a period of time, N months?
  • Does the extension rely on dependencies that meet our extension maturity model?
  • Is the extension reasonable to audit by Envoy security team for obvious scary things, e.g. memcpy, does it have gnarly parsing code, etc?
  • Does the extension have active CODEOWNERS who are willing to vouch for the robustness of the extension?
  • Is the extension absent a low coverage exception?

Thoughts on any others? CC @envoyproxy/security-team

@htuch htuch added enhancement Feature requests. Not bugs or questions. triage Issue requires triage area/security help wanted Needs help! and removed triage Issue requires triage labels Sep 2, 2020
@mattklein123
Copy link
Member

This sounds like a great initial list to me. Should we codify this somewhere in the repo and iterate?

@junr03
Copy link
Member

junr03 commented Sep 3, 2020

extension reasonable to audit by Envoy security team for obvious scary things, e.g. memcpy

I, naively, wonder how much of this type of audits could be codified in linters instead of relying solely on the eyes of the sec team.

@asraa
Copy link
Contributor

asraa commented Sep 3, 2020

Does the extension have fuzz coverage? If it's only receiving fuzzing courtesy of the generic listener/network/HTTP filter fuzzers, does it have a dedicated fuzzer for any parts of the code that would benefit?

On that note, I'm working on a local refactor for the generic HTTP filters, so that the class framework only takes in a filter and then encodes/decodes the HTTP message, without any mock construction unless you specify it. I want to make the interface the library exposes easier for single-fuzz targets to adopt and extend. I'll be following the refactor with a dedicated ext_authz target.

@htuch
Copy link
Member Author

htuch commented Sep 3, 2020

@junr03 yes, good point, some of this should just be code format or CodeQL checks, but other parts are more subjective, e.g. "is this a scary parser?".

@jmarantz
Copy link
Contributor

jmarantz commented Sep 3, 2020

RE "memcpy" it should rarely be needed (and could be excluded with a check_format rule), and instead use MemBlockBuilder in source/common/common/mem_block_builder.h (#9328)

htuch added a commit to htuch/envoy that referenced this issue Sep 3, 2020
Fixes envoyproxy#12962.

Signed-off-by: Harvey Tuch <htuch@google.com>
htuch added a commit that referenced this issue Sep 9, 2020
Fixes #12962.

Signed-off-by: Harvey Tuch <htuch@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants