Exclude sensitive info from the jackson serialization stacktraces

[willyborankin]

Description

If Jackson can't parse JSON body it throws IOException which contains the whole request body including hashes, passwords and so on. This property was added in 2.9 version, so the body will be excluded from logs. Instead, Jackson adds UNKNOWN for the source and provides the property name it can't parse.

So exception now looks like that:

Caused by: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "password" (class org.opensearch.security.securityconf.impl.v7.InternalUserV7), not marked as ignorable (10 known properties: "opendistro_security_roles", "enabled", "backend_roles", "service", "attributes", "reserved", "hidden", "description", "hash", "static"])
 at [Source: UNKNOWN; line: 1, column: 299] (through reference chain: org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration["test"]->org.opensearch.security.securityconf.impl.v7.InternalUserV7["password"])
	at com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException.from(UnrecognizedPropertyException.java:61) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.DeserializationContext.handleUnknownProperty(DeserializationContext.java:1138) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.std.StdDeserializer.handleUnknownProperty(StdDeserializer.java:2224) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownProperty(BeanDeserializerBase.java:1709) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1687) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:320) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.SettableAnyProperty.deserialize(SettableAnyProperty.java:198) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.SettableAnyProperty.deserializeAndSet(SettableAnyProperty.java:179) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.handleUnknownVanilla(BeanDeserializerBase.java:1681) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:320) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4825) ~[jackson-databind-2.15.2.jar:2.15.2]
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3772) ~[jackson-databind-2.15.2.jar:2.15.2]
	at org.opensearch.security.DefaultObjectMapper.lambda$readValue$4(DefaultObjectMapper.java:209) ~[main/:?]
	at java.security.AccessController.doPrivileged(Native Method) ~[?:?]
	at org.opensearch.security.DefaultObjectMapper.readValue(DefaultObjectMapper.java:209) ~[main/:?]
	at org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration.fromJson(SecurityDynamicConfiguration.java:93) ~[main/:?]
	at org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration.fromJson(SecurityDynamicConfiguration.java:69) ~[main/:?]
	at org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration.fromNode(SecurityDynamicConfiguration.java:135) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$patchEntity$11(AbstractApiAction.java:245) ~[main/:?]
	at org.opensearch.security.dlic.rest.validation.ValidationResult.map(ValidationResult.java:55) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$patchEntity$12(AbstractApiAction.java:240) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.withJsonPatchException(AbstractApiAction.java:318) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$patchEntity$13(AbstractApiAction.java:232) ~[main/:?]
	at org.opensearch.security.dlic.rest.validation.ValidationResult.map(ValidationResult.java:55) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$patchEntity$14(AbstractApiAction.java:229) ~[main/:?]
	at org.opensearch.security.dlic.rest.support.Utils.withIOException(Utils.java:275) ~[main/:?]

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Merging #3195 (fce7552) into main (5e8f12c) will decrease coverage by 1.82%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3195      +/-   ##
============================================
- Coverage     62.42%   60.61%   -1.82%     
+ Complexity     3352     3280      -72     
============================================
  Files           254      254              
  Lines         19749    19750       +1     
  Branches       3334     3334              
============================================
- Hits          12329    11972     -357     
- Misses         5788     6162     +374     
+ Partials       1632     1616      -16     

Files Changed	Coverage Δ
...a/org/opensearch/security/DefaultObjectMapper.java	64.55% <100.00%> (+0.45%)	⬆️

... and 22 files with indirect coverage changes

[peternied]

Happy to merge when there is a test that verifies the behavior.

I'm also curious - what did the error message look like before?

[willyborankin]

Happy to merge when there is a test that verifies the behavior.

I'm also curious - what did the error message look like before?

I'm not sure how I can test it, since we just log the exception every where. I need a hint :-).
the exception looks like this (found out when I was testing PATCH in this PR #3123):

java.io.UncheckedIOException: com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "password" (class org.opensearch.security.securityconf.impl.v7.InternalUserV7), not marked as ignorable (10 known properties: "opendistro_security_roles", "enabled", "backend_roles", "service", "attributes", "reserved", "hidden", "description", "hash", "static"])
 at [Source: (String)"{"rest_api_admin_roles":{"hash":"$2y$12$BR.CBsElNLj8v2dzpHJ7bOKVLwWKWjKDhlEvBIvAe9b6/m0xWy2Bq","description":"REST API Roles admin user"},"other":{"hash":"someotherhash","description":"Migrated from v6"},"test":{"hash":"123","opendistro_security_roles":["opendistro_security_reserved"],"password":"neu password 42"},"rest_api_admin_rolesmapping":{"hash":"$2y$12$WQb7PsnRRr04zxjuZsDwU.F7QEr7W0f/rJLjUNLf50hpoJuTqqnaS","description":"REST API Roles Mapping admin user"},"sarek":{"hash":"$2a$12$Ioo1uXmH"[truncated 2181 chars]; line: 1, column: 299] (through reference chain: org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration["test"]->org.opensearch.security.securityconf.impl.v7.InternalUserV7["password"])
	at org.opensearch.security.dlic.rest.support.Utils.withIOException(Utils.java:277) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.patchEntity(AbstractApiAction.java:221) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$processPatchRequest$5(AbstractApiAction.java:181) ~[main/:?]
	at java.util.Optional.map(Optional.java:265) ~[?:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$processPatchRequest$7(AbstractApiAction.java:181) ~[main/:?]
	at org.opensearch.security.dlic.rest.validation.ValidationResult.map(ValidationResult.java:55) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$processPatchRequest$8(AbstractApiAction.java:179) ~[main/:?]
	at org.opensearch.security.dlic.rest.validation.ValidationResult.map(ValidationResult.java:55) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.processPatchRequest(AbstractApiAction.java:178) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.RequestHandler$RequestHandlersBuilder.lambda$onChangeRequest$8(RequestHandler.java:157) ~[main/:?]
	at org.opensearch.security.dlic.rest.api.AbstractApiAction.lambda$prepareRequest$23(AbstractApiAction.java:594) ~[main/:?]

[willyborankin]

Ok now it is clear if logging is in DEBUG you will see the stacktrace, but in any case I think it makes sense to add.

[peternied]

I had to put it into a diffing tool to figure it out, looks good and its pretty minimal, no test needed IMO