Introduce log maskers for `AnnotatedService` #6232

jrhee17 · 2025-05-07T14:05:10Z

Motivation:

This PR includes changes for #6231 .

While logging request/response logs provides developers more tools for debugging, logging sensitive data runs the risk of exposing certain information outside of the intended scope.
For this reason, there have been multiple attempts to add sanitization at #3795 and #4267.

This PR attempts to provide users an easier way to mask logging data by recursively introspecting fields for cross-cutting concerns. In detail, users may determine whether to mask a POJO field or not depending on annotations.
The following illustrates a sample usage where all fields with a @ShouldMask annotation are nullified:

@Retention(RetentionPolicy.RUNTIME)
public @interface ShouldMask {}

val contentSanitizer = ContentSanitizer.builder()
                .fieldMaskerSelector(FieldMaskerSelector.ofBean(info -> {
                    final ShouldMask maskerAnn = info.getAnnotation(ShouldMask.class);
                    if (maskerAnn == null) {
                        return FieldMasker.fallthrough();
                    }
                    return FieldMasker.nullify();
                }))
                .buildForText()

LoggingService.builder()
              .logWriter(LogWriter.of(LogFormatter.builderForText()
                                                 .contentSanitizer(contentSanitizer)
                                                 .build()))
              .newDecorator()

Note that the default serializers for AnnotatedRequest and AnnotatedResponse have been removed.
I believe this wouldn't be an issue since

Users will be using ContentPreviewingService for annotated services anyways
JsonLogFormatter naively applies ObjectMapper - even now Thrift, gRPC content probably wasn't correctly serialized anyways
It's probably worth tackling this issue when renovating JsonLogFormatter rather than in this PR

Modifications:

Serializers for AnnotatedRequest and AnnotatedResponse have been added
- Special logic is added to unwrap CF or ignore armeria internal types that don't support Jackson.
Log masking related APIs are introduced
- BeanFieldMaskerSelector is introduced so users can determine how to mask a field depending on the annotation information
- BeanFieldInfo is introduced which contains annotation information of a bean field.
  - JacksonBeanFieldInfo is created while recursively iterating fields via Jackson. AnnotatedBeanFieldInfo is created from AnnotatedService for parameters or return values.
- FieldMasker is introduced which actually performs the masking of a field value.
  - As it may be complicated to implement FieldMasker directly, FieldMaskerBuilder is introduced so users can easily create a FieldMasker in a type-safe manner.
- FieldMaskerSelectorProvider is introduced to allow FieldMaskerSelector customization for different protocols (thrift, gRPC)
  - BeanFieldMaskerSelectorProvider is implemented which customizes an ObjectMapper to correctly handle log masking for AnnotatedService
- MaskingBeanDeserializerModifier and MaskingBeanSerializerModifier are added, which applies the appropriate FieldMasker during serde.

Result:

Users can determine whether to mask a POJO field via annotations

codecov · 2025-05-07T14:26:43Z

Codecov Report

Attention: Patch coverage is 86.46154% with 44 lines in your changes missing coverage. Please review.

Project coverage is 74.71%. Comparing base (8150425) to head (abc2164).
Report is 102 commits behind head on main.

Files with missing lines	Patch %	Lines
...orp/armeria/common/logging/FieldMaskerBuilder.java	70.17%	15 Missing and 2 partials ⚠️
...ternal/server/annotation/JacksonBeanFieldInfo.java	63.15%	7 Missing ⚠️
...rnal/server/annotation/AnnotatedBeanFieldInfo.java	64.70%	6 Missing ⚠️
...rmeria/common/logging/ContentSanitizerBuilder.java	92.85%	2 Missing and 1 partial ⚠️
...rmeria/common/logging/BeanFieldMaskerSelector.java	66.66%	1 Missing and 1 partial ⚠️
...ecorp/armeria/common/logging/ContentSanitizer.java	33.33%	2 Missing ⚠️
.../linecorp/armeria/common/logging/FieldMaskers.java	80.00%	2 Missing ⚠️
...er/annotation/MaskingBeanDeserializerModifier.java	91.30%	1 Missing and 1 partial ⚠️
...rver/annotation/MaskingBeanSerializerModifier.java	93.54%	1 Missing and 1 partial ⚠️
...a/internal/server/annotation/AnnotatedRequest.java	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6232      +/-   ##
============================================
+ Coverage     74.46%   74.71%   +0.25%     
- Complexity    22234    22537     +303     
============================================
  Files          1963     1998      +35     
  Lines         82437    83227     +790     
  Branches      10764    10801      +37     
============================================
+ Hits          61385    62186     +801     
+ Misses        15918    15896      -22     
- Partials       5134     5145      +11

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-05-07T15:07:58Z

🔍 Build Scan® (commit: `4ca67f6`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/rtp6uljhydoby
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/ltr3y23brydjm
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/gwctwvmqthaq2
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/se4s3lef3cpzm
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/a2w425ifgxft6
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/nilwex4bomzji
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/4j6s2wx7be2nm

jrhee17 · 2025-05-13T02:42:04Z

Ready for review

jrhee17 · 2025-05-20T08:13:56Z

Bump) Ready for review

ikhoon

Overall, looks good. Left some questions.

ikhoon · 2025-05-28T05:44:28Z

...in/java/com/linecorp/armeria/internal/server/annotation/AnnotatedResponseJsonSerializer.java

@@ -45,6 +53,8 @@ public void serialize(AnnotatedResponse value, JsonGenerator gen, SerializerProv
            return;
        }

+        final FieldMasker fieldMasker = fieldMaskerCache.fieldMasker(value.beanFieldInfo());


Question) Are there any plans to add an extension point for maybeUnwrapFuture()? If not, should users implement their own JsonSerializer to handle the case?

Related: #6231 (comment)

Sure, I've added a AnnotatedRequest#parameter(int), AnnotatedResponse#value() method which unwraps automatically. These methods will be called for both Annotated[Request|Response]JsonSerializer and toString methods.

I've also added an internal utility class AnnotatedServiceLogUtil which serializes Annotated[Request|Response]JsonSerializer given that Annotated[Request|Response] is not public.

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

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

ikhoon · 2025-05-28T06:23:37Z

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

+
+                @Retention(RetentionPolicy.RUNTIME)
+                public @interface Masker {}
+                @Masker


Suggestion) Should we provide built-in annotations to let users take to use log masker without customizing ContentSanitizer in the f/u PR?

public class Foo { @Masking(Masker.NULL) private String field1; @Masking(Masker.ASTERISK) private String field2; @Masking(value = Masker.PARTIAL_LEFT, keep = 4) private String field3; } // LoggingService.newDecorator() applies the maskers by default.

Sounds like a good idea.

It may be more convenient if the new interface is used as the default contentSanitizer since users won't have to worry about setting contentSanitizer at all and only has to add annotations.

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedBeanFieldInfo.java

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

ikhoon

Thanks, @jrhee17! Looks good to me. 🙇‍♂️🙇‍♂️

minwoox

Mostly looks good 👍
Left some style comments and questions. 😉

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

minwoox · 2025-06-18T07:14:01Z

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

+     * Delegates {@link FieldMasker} selection to a different {@link BeanFieldMaskerSelector}
+     * if the current {@link BeanFieldMaskerSelector} returns {@link FieldMasker#fallthrough()}.
+     */
+    default BeanFieldMaskerSelector orElse(BeanFieldMaskerSelector other) {


Should this be in FieldMaskerSelector?

This was intentional since it doesn't make sense to do a BeanFieldMaskerSelector .orElse(ThriftFieldMaskerSelector).
Let me know if you still feel like this should be in FieldMaskerSelector though

BeanFieldMaskerSelector.orElse(ThriftFieldMaskerSelector)
Isn't it impossible because FieldMaskerSelector has type T? 🤔

Good point. Done

I forgot about this, but the type of FieldMaskerSelector is checked when determining how to use them from the provider.

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

Line 136 in 5c29fdd

selectors.stream().filter(selector -> provider.supportedType().isInstance(selector))

I prefer that BeanFieldMaskerSelector is a functional interface. Even if a marker is added, I think the functional style may not work in some cases.

e.g.

public interface BeanFieldMaskerSelector extends ... { @Override default Class<?> supportedType() { return BeanFieldInfo.class; } } ContentSanitizer.builder() .fieldMaskerSelector((BeanFieldMaskerSelector) info -> { acc.add(info); return FieldMasker.fallthrough(); }) .fieldMaskerSelector(((BeanFieldMaskerSelector) info -> {

I think I prefer the original version unless you have another idea

Yeah, let's use the original version, then.

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

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

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

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

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedBeanFieldInfo.java

minwoox · 2025-06-18T08:19:08Z

core/src/main/java/com/linecorp/armeria/internal/server/annotation/BeanFieldMaskerCache.java

+final class BeanFieldMaskerCache {
+
+    private final List<BeanFieldMaskerSelector> selectors;
+    private final Cache<BeanFieldInfo, FieldMasker> fieldMaskerCache = Caffeine.newBuilder().build();


Question: Is there any reason that you use the Caffeine instead of ConcurrentMap?

It should be fine to just use a simple map. Updated

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

minwoox

I'm not exactly sure how the decryptorFunction in FieldMaskerBuilder will be used yet, but that's totally fine because:

You mentioned that implementing the unmasking logic in Thrift and gRPC isn’t difficult.
The unmasking logic is completely decoupled, so it doesn't add complexity to the current implementation.

Great work — looking forward to your next PR. Thanks!

This reverts commit 5c29fdd.

jrhee17 added 2 commits May 7, 2025 21:56

minimal impl

6781931

minimal impl

fa379e9

jrhee17 added this to the 1.33.0 milestone May 7, 2025

jrhee17 added the new feature label May 7, 2025

jrhee17 marked this pull request as ready for review May 8, 2025 00:45

jrhee17 requested review from ikhoon, minwoox and trustin as code owners May 8, 2025 00:45

jrhee17 added 2 commits May 12, 2025 22:09

Merge branch 'main' into feat/log-maskers

1b930ed

cleanup

7cf7a11

jrhee17 force-pushed the feat/log-maskers branch from c5a929b to 7cf7a11 Compare May 13, 2025 01:19

lint

ecd3390

ikhoon reviewed May 28, 2025

View reviewed changes

jrhee17 added 2 commits May 29, 2025 18:33

Merge branch 'main' into feat/log-maskers

4a887c0

address comments by @ikhoon

d48e165

jrhee17 force-pushed the feat/log-maskers branch from eeeb95f to d48e165 Compare May 30, 2025 03:16

ikhoon approved these changes Jun 5, 2025

View reviewed changes

minwoox reviewed Jun 18, 2025

View reviewed changes

minwoox approved these changes Jun 18, 2025

View reviewed changes

jrhee17 added 5 commits June 19, 2025 10:00

Merge branch 'main' into feat/log-maskers

ebd62c1

address comments by @minwoox

0dd85bb

prefer map over caffeine

abc2164

address comment by @minwoox

5c29fdd

Revert "address comment by @minwoox"

4ca67f6

This reverts commit 5c29fdd.

Introduce log maskers for AnnotatedService #6232

Are you sure you want to change the base?

Introduce log maskers for AnnotatedService #6232

Uh oh!

Conversation

jrhee17 commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: 4ca67f6)

Uh oh!

jrhee17 commented May 13, 2025

Uh oh!

jrhee17 commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhee17 May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhee17 Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minwoox Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Introduce log maskers for `AnnotatedService` #6232

Introduce log maskers for `AnnotatedService` #6232

jrhee17 commented May 7, 2025 •

edited

Loading

codecov bot commented May 7, 2025 •

edited

Loading

github-actions bot commented May 7, 2025 •

edited

Loading

🔍 Build Scan® (commit: `4ca67f6`)

jrhee17 commented May 20, 2025 •

edited

Loading

jrhee17 May 30, 2025 •

edited

Loading

jrhee17 Jun 19, 2025 •

edited

Loading

minwoox Jun 19, 2025 •

edited

Loading