armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Introduce log maskers for `AnnotatedService`

Open jrhee17 opened this issue 11 months ago • 4 comments

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

  1. Users will be using ContentPreviewingService for annotated services anyways
  2. 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

jrhee17 avatar May 07 '25 14:05 jrhee17

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 :warning:
...ternal/server/annotation/JacksonBeanFieldInfo.java 63.15% 7 Missing :warning:
...rnal/server/annotation/AnnotatedBeanFieldInfo.java 64.70% 6 Missing :warning:
...rmeria/common/logging/ContentSanitizerBuilder.java 92.85% 2 Missing and 1 partial :warning:
...rmeria/common/logging/BeanFieldMaskerSelector.java 66.66% 1 Missing and 1 partial :warning:
...ecorp/armeria/common/logging/ContentSanitizer.java 33.33% 2 Missing :warning:
.../linecorp/armeria/common/logging/FieldMaskers.java 80.00% 2 Missing :warning:
...er/annotation/MaskingBeanDeserializerModifier.java 91.30% 1 Missing and 1 partial :warning:
...rver/annotation/MaskingBeanSerializerModifier.java 93.54% 1 Missing and 1 partial :warning:
...a/internal/server/annotation/AnnotatedRequest.java 91.66% 0 Missing and 1 partial :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar May 07 '25 14:05 codecov[bot]

🔍 Build Scan® (commit: 4ca67f691466668c8c5f428de370e01a091635cc)

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

github-actions[bot] avatar May 07 '25 15:05 github-actions[bot]

Ready for review

jrhee17 avatar May 13 '25 02:05 jrhee17

Bump) Ready for review

jrhee17 avatar May 20 '25 08:05 jrhee17