formatter: print request header without query string

[tsaarni]

Signed-off-by: Tero Saarni [email protected]

Commit Message:
Adds formatter extension implementing a command operator that writes HTTP request header to access log without including the query string. Resolves #7583.

Additional Description:
Motivation for this change: Query string may contain sensitive information introducing potential vulnerability since access logs may unintentionally expose this information. If the problem cannot be fixed in the proxied application itself, the vulnerability can be mitigated by removing query string from the access logs.

This PR is similar to #7847 but implemented as extension and it is first in-tree formatter extension (xref #14512).

It implements command %REQ_WITHOUT_QUERY(X?Y):Z% which works in the same way as existing command operator %REQ(X?Y):Z%. The proposed implementation closely follows SubstitutionFormatParser and reuses SubstitutionFormatParser::parseCommandHeader() for parsing the command arguments.

In order to get check_format.py to pass, I needed to add entry to CODEOWNERS but since I don't know who should be added, I added @dio and myself as a placeholder for now.

Risk Level: Low
Testing: unit test
Docs Changes: New extension category added to docs
Release Notes: formatter: command operator to log request headers without query string.
Platform Specific Features: N/A

[rgs1]

FWIW, we've achieved the equivalent of %REQ_WITHOUT_QUERY(X?Y):Z% by using the header-to-metadata filter, e.g.:

- typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.http.header_to_metadata.v3.Config
    request_rules:
      - header: ":path"
        on_header_present:
          metadata_namespace: com.example.sanitized_headers
          key: path
          regex_value_rewrite:
            pattern:
              google_re2: {}
              regex: "^(/[^?]*).*$"
            substitution: "\\1"

and then from the access log format string:

%DYNAMIC_METADATA(com.example.sanitized_headers:path)%

though I do still think it'll be nice to have an extension with REQ_WITHOUT_QUERY.

cc: @sc0ttbeardsley

[rgs1]

Generally LGTM, left two comments plus let's get the build fixed and I'll make another pass.

[tsaarni]

FWIW, we've achieved the equivalent of %REQ_WITHOUT_QUERY(X?Y):Z% by using the header-to-metadata filter, e.g.:

Thanks! I did not know about this filter.

In general, I have some doubts about about this PR. Adding a complete extension for the single purpose of removing ?querystring seems bit wrong. Do people see it as problematic if we have a lot of small extensions that do really trivial things?

I've also done some experiments with more general purpose formatter, inspired by your example with Header-To-Metadata, which allows following

formatters:
  - name: envoy.formatter.regex_substitute
    typed_config:
      "@type": type.googleapis.com/envoy.extensions.formatter.regex_substitute.v3.RegexSubstituteConfig
      command_name: REQ_WITHOUT_QUERY
      regex_value_rewrite:
        pattern:
          google_re2: {}
          regex: "^(/[^?]*).*$"
        substitution: "\\1"

but would this then be duplication of functionality?

[repokitteh-read-only]

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

🐱

Caused by: #15711 was synchronize by tsaarni.

see: more, trace.

[dio]

@tsaarni sorry I was out for a health issue. Sorry was not able to update this earlier. But generally, the PR is good. If you resolve the conflict I'll take a look at it as soon as I can. And it is nice to have that REQ_WITHOUT_QUERY shorthand.

[dio]

Simply FormatterProviderPtr doesn't work here? Sorry for nitpicking.

[tsaarni]

Same as above, it did not work...

[tsaarni]

@tsaarni sorry I was out for a health issue. Sorry was not able to update this earlier. But generally, the PR is good. If you resolve the conflict I'll take a look at it as soon as I can. And it is nice to have that REQ_WITHOUT_QUERY shorthand.

@dio No problem with delays, glad to see you back! I've now resolved the conflict.

I wonder if you have opinion about #15711 (comment), that is: more general purpose formatter (regex manipulation) vs this one (dedicated for query string alone)? I'm OK with anything at the end, but just wanted to hear your thoughts :-)

[tsaarni]

Check envoy-presubmit (check linux_x64 gcc) is failing for me due to too long argument list (full log here)

2021-04-24T19:45:27.1433253Z ERROR: /source/test/exe/BUILD:98:14: C++ compilation of rule '//test/exe:win32_scm_test' failed (Exit 1): gcc failed: error executing command 
...
2021-04-24T19:45:27.2269979Z gcc: fatal error: cannot execute ‘/usr/lib/gcc/x86_64-linux-gnu/9/cc1plus’: execv: Argument list too long

and in another case in different target (full log here)

2021-04-22T18:21:55.0427418Z ERROR: /source/test/exe/BUILD:62:14: C++ compilation of rule '//test/exe:main_common_test' failed (Exit 1): gcc failed: error executing command 
...
2021-04-22T18:21:55.1126016Z gcc: fatal error: cannot execute ‘/usr/lib/gcc/x86_64-linux-gnu/9/cc1plus’: execv: Argument list too long

I saw @rgs1 mentioning this in #14855 (comment) but there it was config_fuzz_test_lib. I don't understand why it is not always the same target that fails but it seems to hit more compilation targets as just a specific lib.

[tsaarni]

Could I kindly ask help in reviewing of this PR? I think @dio might be still unavailable, so I was hoping maybe others could be able to help and look at this also?

[rgs1]

Left one nit, otherwise generally LGTM. My only remaining question is whether this should be included/built by default (it is right now, maybe it shouldn't be).

Over to @dio for another round. Thanks!

[tsaarni]

I think it's OK to make this one trusted for downstream and upstream.

Thanks @mattklein123. I've now added entry to release notes and changed the security posture.

[mattklein123]

Oops looks like it needs a main merge.

/wait

[tsaarni]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15711 (comment) was created by @tsaarni.

see: more, trace.

[tsaarni]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15711 (comment) was created by @tsaarni.

see: more, trace.

[tsaarni]

There has been quite many flakes but these tests now seem to be failing quite consistently and I don't seem to get clean CI run anymore

• IpHttpVersions/HttpHealthCheckIntegrationTest.SingleEndpointImmediateHealthcheckFailHttp/IPv4_Http2Upstream (logs)
• IpHttpVersions/HttpHealthCheckIntegrationTest.SingleEndpointImmediateHealthcheckFailHttp/IPv6_Http2Upstream (logs)

I saw some other PRs have had same failures, so I wonder if this is a flake as well, or could it have something to do with the changes in this PR?

[zuercher]

Yeah, it's happening on master as well.