Easily mask sensitive headers from request logs by adding `HeadersSanitizer` #5188

seonWKim · 2023-09-16T03:32:21Z

Motivation:

Mask sensitive headers when logging request and response headers.

Modifications:

Allow users to configure maskHeaders, maskRequestHeaders, maskResponseHeaders when using AbstractFormatterBuilder implementations. (e.g. TextLogFormatterBuilder, JsonLogFormatterBuilder)
Use values from maskRequestHeaders and maskResponseHeaders to mask request and response headers

Result:

Users are able to easily mask request and response headers:

HeadersSanitizer
  .builderForText()
  .sensitiveHeaders("Authorization"...)
  .maskingFunction((name, value) -> "....")
  .build()

Closes Easily mask sensitive headers from request logs #5175

codecov · 2023-09-16T04:06:47Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (9568d56) 0.00% compared to head (a837197) 74.01%.
Report is 137 commits behind head on main.

Files	Patch %	Lines
...ia/common/logging/JsonHeadersSanitizerBuilder.java	66.66%	2 Missing and 1 partial ⚠️
...p/armeria/common/logging/TextHeadersSanitizer.java	94.11%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5188       +/-   ##
===========================================
+ Coverage        0   74.01%   +74.01%     
- Complexity      0    20744    +20744     
===========================================
  Files           0     1799     +1799     
  Lines           0    76422    +76422     
  Branches        0     9728     +9728     
===========================================
+ Hits            0    56561    +56561     
- Misses          0    15258    +15258     
- Partials        0     4603     +4603

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2023-09-16T04:47:10Z

🔍 Build Scan® (commit: `a837197`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/qqeaeheiqdsfm
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/ttkaebndl4yum
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/zcs57yxrh4zak
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/u5iprdibspk3s
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/oakzf2faufc7s
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/uczhpypzzisom
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/dd622kkrsoxyw
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/jypihrt4t4kik

jrhee17

Left a preliminary comment 🙇

core/src/main/java/com/linecorp/armeria/common/logging/AbstractLogFormatterBuilder.java

core/src/main/java/com/linecorp/armeria/common/logging/TextLogFormatterBuilder.java

core/src/main/java/com/linecorp/armeria/common/logging/AbstractLogFormatterBuilder.java

core/src/main/java/com/linecorp/armeria/common/AbstractHeadersSanitizerBuilder.java

core/src/main/java/com/linecorp/armeria/common/JsonHeadersSanitizerBuilder.java

core/src/main/java/com/linecorp/armeria/common/TextHeadersSanitizer.java

core/src/main/java/com/linecorp/armeria/common/TextHeadersSanitizerBuilder.java

core/src/main/java/com/linecorp/armeria/common/TextHeadersSanitizer.java

seonWKim · 2023-12-01T15:22:12Z

Sorry for the late reply. I will work on the issue again 🙇

ikhoon · 2023-12-04T07:45:28Z

The Git history looks messed up. Changes of +14,386 −3,985 are not made by in this PR.
Could you clean the Git history of this PR with squash, reset, etc?

- fix comments - change method order

core/src/main/java/com/linecorp/armeria/common/JsonHeadersSanitizer.java

core/src/main/java/com/linecorp/armeria/common/AbstractHeadersSanitizerBuilder.java

core/src/main/java/com/linecorp/armeria/common/JsonHeadersSanitizer.java

core/src/main/java/com/linecorp/armeria/common/TextHeadersSanitizer.java

ikhoon · 2024-01-03T02:47:03Z

core/src/main/java/com/linecorp/armeria/common/AbstractHeadersSanitizerBuilder.java

+    /**
+     * Sets the {@link Set} which includes headers to mask before logging.
+     */
+    public AbstractHeadersSanitizerBuilder<T> maskHeaders(String... headers) {


The headers defined in HttpHeaderNames should be compatible without additional casting.

maskingHeaders or sensitiveHeaders is preferred over maskHeaders because this function sets the headers to be masked, not the actual action.

Suggested change

public AbstractHeadersSanitizerBuilder<T> maskHeaders(String... headers) {

public AbstractHeadersSanitizerBuilder<T> maskingHeaders(CharSequence... headers) {

Great point. I'll apply as below

maskHeaders -> maskingHeaders

mask -> maskingFunction

core/src/main/java/com/linecorp/armeria/common/AbstractHeadersSanitizerBuilder.java

core/src/main/java/com/linecorp/armeria/common/HeadersSanitizer.java

seonWKim · 2024-01-06T08:39:54Z

@ikhoon , should the masking function be case insensitive? For example, if user set maskingHeaders("set-cookie"), should we mask "Set-Cookie" header(which is uppercase)?

- Fix naming - Mask sensitive headers by default

ikhoon · 2024-01-18T07:36:28Z

HTTP header names are case-insensitive as per https://www.rfc-editor.org/rfc/rfc2616#section-4.2

core/src/main/java/com/linecorp/armeria/common/JsonHeadersSanitizer.java

minwoox · 2024-01-24T09:20:46Z

I pushed changes that fix some style issued. PTAL. 😉

seonWKim · 2024-01-24T12:42:26Z

I pushed changes that fix some style issued. PTAL. 😉

@minwoox Thanks for cleaning up the mess I've made 🙇 . The PR seems ready 👍

jrhee17

Looks nice! Thanks @seonwoo960000 🙇 👍 🙇

ikhoon · 2024-01-29T10:23:05Z

@jrhee17 Sorry, I've made some changes after your review. PTAL tomorrow. 🙇‍♂️

minwoox

Still, looks good to me. Thanks! 😉

- A header name is necessary to dynamically mask header values. - Handle a null value returned by masking functions. - Replace Function<String,String` with `HeaderMaskingFunction`. - `HeaderSanitizer` their builders are moved to `common.logging` from `common`.

jrhee17

Looks good! Thanks @ikhoon 🙇 👍 🙇

core/src/test/java/com/linecorp/armeria/common/logging/TextLogFormatterTest.java

core/src/main/java/com/linecorp/armeria/common/logging/AbstractHeadersSanitizerBuilder.java

…tHeadersSanitizerBuilder.java

ikhoon

Nice work! Thanks, @seonwoo960000 🙇‍♂️🚀

Motivation: `HeadersSanitizer` was added in line#5188. If no `HeadersSanitizer` is set, the default santizer masks `Authorization`, `Cookie` `Set-Cookie` and `Proxy-Authorization` with `****`. In addition to `HeadersSantizer`, other new features are added to `LoggingDecoratorBuilder` and the code became messy to maintain backward compatibility. `LoggingDecoratorBuilder`, `LogWriterBuilder`, and `LogFormatterBuilder` each have their own properties, so it was not easy to make the overall operation consistent. As a result, while `LoggingClient.newDecorator()` santizes sensitive headers, LoggingClient.builder().requestLogLevel(...).newDecorator()` did not work. Neither code set a sanitizer, so a default sanitizer should have been set, but it didn't. Modifications: - Refactor `LoggingDecoratorBuilder` to delegate all builder properties to either `LogWriterBuilder` or `LogFormatterBuilder`. - Add missing `@Deprecated` annotation to `LoggingClientBuilder` and `LoggingRpcClientBuilder` and `LoggingServiceBuilder`. - `@Deprecated` in the super method aren't inspected by IntelliJ line#5395 - Breaking) Change the signature of `LogWriterBuilder.responseCauseFilter()` to have `RequestContext` as the first parameter. - It may be a bug or a mistake when adding the API. Our APIs usually provide `RequestContext` for predicates or handlers. Result: `LoggingClient` and `LoggingService` now correctly mask sensitive headers by default.

…ng decorators (#5519) Motivation: `HeadersSanitizer` was added in #5188. If no `HeadersSanitizer` is set, the default sanitizer masks `Authorization`, `Cookie` `Set-Cookie`, and `Proxy-Authorization` with `****`. In addition to `HeadersSantizer`, other new features were added to `LoggingDecoratorBuilder` and the code became messy to maintain backward compatibility. `LoggingDecoratorBuilder`, `LogWriterBuilder`, and `LogFormatterBuilder` each have their own properties, so making the overall operation consistent was not easy. As a result, while `LoggingClient.newDecorator()` sanitizes sensitive headers, LoggingClient.builder().requestLogLevel(...).newDecorator()` did not work. Neither code set a sanitizer, so a default sanitizer should have been set, but it didn't. Modifications: - Refactor `LoggingDecoratorBuilder` to delegate all builder properties to either `LogWriterBuilder` or `LogFormatterBuilder`. - Add missing `@Deprecated` annotation to `LoggingClientBuilder` and `LoggingRpcClientBuilder` and `LoggingServiceBuilder`. - `@Deprecated` in the super method isn't inspected by IntelliJ #5395 - Breaking) Change the signature of `LogWriterBuilder.responseCauseFilter()` to have `RequestContext` as the first parameter. - It may be a bug or a mistake when adding the API. Our APIs usually provide `RequestContext` for predicates or handlers. Result: `LoggingClient` and `LoggingService` now correctly mask sensitive headers by default.

seonWKim requested review from ikhoon, jrhee17, minwoox and trustin as code owners September 16, 2023 03:32

jrhee17 added the new feature label Sep 18, 2023

jrhee17 added this to the 1.26.0 milestone Sep 18, 2023

jrhee17 reviewed Sep 18, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/logging/AbstractLogFormatterBuilder.java Outdated Show resolved Hide resolved

seonWKim force-pushed the mask-sensible-headers branch from 76e2dcc to 6e5df41 Compare October 4, 2023 16:38

seonWKim changed the title ~~Add maskHeaders, maskRequestHeaders, maskResponseHeaders for LogFormatter~~ Easily mask sensitive headers from request logs by adding HeadersSanitizer Oct 5, 2023

minwoox reviewed Oct 11, 2023

View reviewed changes

minwoox modified the milestones: 1.26.0, 1.27.0 Oct 11, 2023

0x1306e6d reviewed Nov 13, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/TextHeadersSanitizer.java Outdated Show resolved Hide resolved

seonWKim force-pushed the mask-sensible-headers branch from e690150 to e8ee73d Compare December 4, 2023 23:38

seonWKim closed this Dec 4, 2023

seonWKim reopened this Dec 4, 2023

Add HeadersSanitizer to allow users to easily mask headers

3c163cb

seonWKim force-pushed the mask-sensible-headers branch from e8ee73d to 3c163cb Compare December 6, 2023 14:45

Nit

a52c45e

- fix comments - change method order

seonWKim mentioned this pull request Dec 7, 2023

Mask well known HTTP headers that contain sensitive information by default #5286

Closed

ikhoon reviewed Jan 3, 2024

View reviewed changes

Apply comments

0904f65

- Fix naming - Mask sensitive headers by default

seonWKim force-pushed the mask-sensible-headers branch from f35a31e to 0904f65 Compare January 7, 2024 01:47

Fix HashMap to LinkedHashMap to maintain order of headers

b3dfd7d

ikhoon reviewed Jan 18, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/JsonHeadersSanitizer.java Outdated Show resolved Hide resolved

Fix style issues

b4cb62b

Address the comment from @ikhoon

c5b8961

jrhee17 approved these changes Jan 29, 2024

View reviewed changes

BiFunction was replaced with HeadersSanitizer

d7dbffc

ikhoon requested a review from jrhee17 January 29, 2024 10:22

ikhoon added 3 commits January 29, 2024 19:34

Update the default sensitive headers

d2bd0a7

add reference

cecbc3b

final.final.final

c1161fc

minwoox approved these changes Jan 29, 2024

View reviewed changes

ikhoon added 4 commits January 29, 2024 20:38

Javadoc for the default sensitive headers

042141a

Rename maskingHeaders to sensitiveHeaders

ad0ab4f

Fix test failures

6690425

jrhee17 approved these changes Jan 30, 2024

View reviewed changes

core/src/test/java/com/linecorp/armeria/common/logging/TextLogFormatterTest.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/logging/AbstractHeadersSanitizerBuilder.java Outdated Show resolved Hide resolved

Remove public

c15e7bf

ikhoon reviewed Jan 30, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/logging/AbstractHeadersSanitizerBuilder.java Outdated Show resolved Hide resolved

Update core/src/main/java/com/linecorp/armeria/common/logging/Abstrac…

a837197

…tHeadersSanitizerBuilder.java

ikhoon approved these changes Jan 30, 2024

View reviewed changes

ikhoon merged commit 2400845 into line:main Jan 30, 2024
16 of 17 checks passed

ikhoon mentioned this pull request Mar 19, 2024

Fix a bug where the default HeadersSanitizer does not work in logging decorators #5519

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Easily mask sensitive headers from request logs by adding `HeadersSanitizer` #5188

Easily mask sensitive headers from request logs by adding `HeadersSanitizer` #5188

seonWKim commented Sep 16, 2023 •

edited by ikhoon

Loading

codecov bot commented Sep 16, 2023 •

edited

Loading

github-actions bot commented Sep 16, 2023 •

edited

Loading

jrhee17 left a comment

seonWKim commented Dec 1, 2023

ikhoon commented Dec 4, 2023 •

edited

Loading

ikhoon Jan 3, 2024

seonWKim Jan 6, 2024

seonWKim commented Jan 6, 2024 •

edited

Loading

ikhoon commented Jan 18, 2024

minwoox commented Jan 24, 2024

seonWKim commented Jan 24, 2024 •

edited

Loading

jrhee17 left a comment

ikhoon commented Jan 29, 2024

minwoox left a comment

jrhee17 left a comment

ikhoon left a comment

	public AbstractHeadersSanitizerBuilder<T> maskHeaders(String... headers) {
	public AbstractHeadersSanitizerBuilder<T> maskingHeaders(CharSequence... headers) {

Easily mask sensitive headers from request logs by adding HeadersSanitizer #5188

Easily mask sensitive headers from request logs by adding HeadersSanitizer #5188

Conversation

seonWKim commented Sep 16, 2023 • edited by ikhoon Loading

codecov bot commented Sep 16, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 16, 2023 • edited Loading

🔍 Build Scan® (commit: a837197)

jrhee17 left a comment

Choose a reason for hiding this comment

seonWKim commented Dec 1, 2023

ikhoon commented Dec 4, 2023 • edited Loading

ikhoon Jan 3, 2024

Choose a reason for hiding this comment

seonWKim Jan 6, 2024

Choose a reason for hiding this comment

seonWKim commented Jan 6, 2024 • edited Loading

ikhoon commented Jan 18, 2024

minwoox commented Jan 24, 2024

seonWKim commented Jan 24, 2024 • edited Loading

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon commented Jan 29, 2024

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Easily mask sensitive headers from request logs by adding `HeadersSanitizer` #5188

Easily mask sensitive headers from request logs by adding `HeadersSanitizer` #5188

seonWKim commented Sep 16, 2023 •

edited by ikhoon

Loading

codecov bot commented Sep 16, 2023 •

edited

Loading

github-actions bot commented Sep 16, 2023 •

edited

Loading

🔍 Build Scan® (commit: `a837197`)

ikhoon commented Dec 4, 2023 •

edited

Loading

seonWKim commented Jan 6, 2024 •

edited

Loading

seonWKim commented Jan 24, 2024 •

edited

Loading