Change number masking default configuratuion in the API

[Breus]

Disclaimer:
Since release 0.2.0, the JSON masker can be considered production-ready.
Nonetheless, for a little while we still keep the library in initial development phase (version 0.x) to be able to make changes to the API to make sure it will be as stable as possible after the first major 1.x release. Every release initial release version (0.x) that changes the API will have this clearly documented in the release notes.

One of the things I think is currently sub-optimal in the API is that number masking is configured to be disabled by default.
This seems counter-intuitive for a library which is meant to mask JSON messages, for example to mask sensitive data.
I (and I heard this from other users) think it is somewhat unexpected that by default, when a key is masked but has a numeric value, it is not masked unless number masking is enabled.

This was decided in the past because of a combination of considerations:

1. This library produces valid JSON output when valid JSON is given as input. Even more so, the masker does not even change the format of the JSON nor the JSON type of any of the values.
2. In JSON, it is invalid to have needless trailing 0's in numeric values. I.e.: {"pin": 0000} is invalid JSON.
3. Before release 0.2.1, specifically before PR 33, masking which could not happen in-place resulted in a resize per value masked which is a very expensive operation in the context of this high-performance library.
4. This library aims to be highly performant in terms of throughput and in terms of minimizing memory allocations.

For strings in JSON, it is quite easy to come up with a logical mask for a value: E.g.: replacing each human-readable character with a single asterisk (*).
However, combining considerations 1. and 2. above, this is not so obvious for numbers:
Either users would like to preserve the length of the numeric values or they feel like '0' is the most logical mask. Preserving the length of the numeric value would mean that only the digits 1-9 can be used, and not 0 (again, because of consideration 2).

Considering the above, historically we decided to have number masking disabled by default and the user could determine what seemed most logic for them. Again, because of consideration 3., using a single '0' (see consideration 2) as mask for numbers used to come with a severe performance penalty for certain cases.

Now that this performance penalty has been mitigated and based on user feedback, I think we can consider enabling number masking by default and there are multiple options for this.

Therefore, we can do the following things:

1. Make maskNumberValueWith(int numberMask) a required part of the builder (of course, something like disableNumberMasking would be an alternative option there as well). This forces users to make the decision about number masking,
2. Mask numbers by default with a single 0, losing the information about the length of the original number
3. Mask numbers by default with some digit (1-9), which would preserve the length of the number, but seemingly "random" one of the digits from 1 to 9 will be the mask.
4. Keep number masking disabled by default and keep it optional (as is)
5. ???

Curious to hear about any input!

[donavdey]

I would assume the library to mask values for every key that I pass irrespective of the field type. So enabling numeric fields masking by default seems logical. That also concerns masking boolean fields.
I would also assume the "obfuscation length" parameter to be applied to numeric fields as well. Currently the obfuscation is disabled by default, which means that the default numeric masking strategy should be replacing every digit in a number by a "replacement" digit. It seems reasonable that the "replacement" digit should be consistent among the masker usages. The most intuitive value for the "replacement" digit is 8.
As for the builder API, having a "disableNumberMasking" option seems to be more intuitive. "maskNumberValueWith" also could potentially exist, but I can't imagine a scenario when someone needs to override the default "replacement" digit value.

[Breus]

That also concerns masking boolean fields.

I would like to keep this discussion focused on number masking, but I can give a short explainer why booleans are not masked:
First of all, a boolean value cannot really "contain" sensitive data (it can only be the case that the implied meaning of the boolean is sensitive).
Second, the only "logical" way to mask a boolean while preserving JSON format/types is using false which seems a bit odd as it isn't clear that this is masked (all numeric values being 88888 or 0 is a bit more obvious).

I would also assume the "obfuscation length" parameter to be applied to numeric fields as well.

It is when obfuscation is enabled, indeed.

The most intuitive value for the "replacement" digit is 8.

Somehow I agree, but I cannot really pinpoint why 😅

As for the builder API, having a "disableNumberMasking" option seems to be more intuitive. "maskNumberValueWith" also could potentially exist, but I can't imagine a scenario when someone needs to override the default "replacement" digit value.

Alright, so you propose replacing numeric values with 8's and preserving the length of the original value. Thanks for your input!

[gavlyukovskiy]

Second, the only "logical" way to mask a boolean while preserving JSON format/types is using false which seems a bit odd as it isn't clear that this is masked (all numeric values being 88888 or 0 is a bit more obvious).

One way we could still mask numbers and also booleans is to use null, it still constitutes a valid JSON (as all types are nullable). It might not be possible to deserialize the JSON using the same models (i.e. jackson with nullability annotations or kotlin), but we'd still have a valid JSON (same as masking number / booleans as string '***')

[donavdey]

The idea with nullifying numeric/boolean values is elegant. However there is a convention that "null" is interpreted as "the absence of a value". In json case, that would also mean the absence of a key. This is more suitable if we "hide" sensitive data, not "mask" it.
If we want to preserve the deserializability of a json, then setting boolean keys to a "replacement" boolean value (the most intuitive replacement value is "false") is the only option.
if we do not preserve the deserializability of a json, then we can just as well replace the type of a key from boolean to string and mask it as "****".

More generally speaking, there could be an option named something like "preserveSerialisationContract", which makes the masker mask numeric values as "8888" and boolean values as "false". If the option is disabled, then every value is replaced with "****" string irrespective of the type.

[RobertBlaauwendraad]

Since the main function of this library is to mask values, I expect it to mask any fields out of the box without any side-effects. Thus disabling number masking or obfuscating the length by default is kind of weird in my opinion. Making the maskNumberValueWith parameter required only moves this 'issue' to the user who most likely doesn't have a preferences either.

That will leave 3 options:

1. Randomize each digit, but this will probably affect performance.
2. Start the masked value with a digit (e.g. 1) and follow with zeroes. For exampe, 6789 will become 1000. However this will still require to make a choice for the leading digit.
3. Choose one specific digit and replace all digits with it. I think the digit 1 can make sense since it is the successor of 0 and used in binary. Yet my preference goes to 8, simply because on an eight-segment display it will light up all segments masking any digit. Hence it feels natural/logical.

In conclusion, I think it's important to keep the main functionality in mind and don't worry too much about the looks of it. For this reason picking some digit, where 8 is my favorite, is the best solution in my opinion. Maybe someday it will become a standard for masking numbers, like * is for characters.

[gavlyukovskiy]

To me the option 2:

Mask numbers by default with a single 0, losing the information about the length of the original number

seems like a good default to have together with masking everything by default.

However about length obfuscation, I think we should obfuscate by default and instead introduce an option config.preserveMaskedValueLength() to keep the length. I was thinking about it for quite some time now and I don't quite see much benefits of keeping the length by default. For example spring-boot has some sanitizing capabilities that are enabled by default for actuator endpoints and they sanitize everything with the same value without even an option to preserve length.

We could also make it possible to choose from variable options and checking invariants on the config builder, for example we might have these configuration options:

config
    .preserveLength()
    .maskStringWith("[redacted]")      // incompatible with config.preserveLength() or config.maskStringCharactersWith()
    .maskNumberWith(0).                // incompatible with config.preserveLength() or config.maskNumberDigitsWith()
    .maskNumberWith("[redacted]")      // converts the JSON type, but is explicit for the user
    .maskBooleanWith(null)
    .maskBooleanWith("[redacted]")     // converts the JSON type, but is explicit for the user
    .maskNumberDigitsWith(1)           // cannot be 0, incompatible with config.maskNumberWith()
    .maskStringCharactersWith('*')     // has to be a single character, incompatible with config.maskStringWith()

and the default config being something like this

config
    .maskStringWith("***")
    .maskNumberWith(0)
    .maskBooleanWith(null)

As part of larger discussion I'd love to be able to set these on the level of a target key(s), but that's probably for the different discussion.

[Breus]

We could also make it possible to choose from variable options and checking invariants on the config builder, for example we might have these configuration options:

I really like this idea! It gives much more options to the user and makes it very explicit. The default we could clearly document.

However about length obfuscation, I think we should obfuscate by default and instead introduce an option config.preserveMaskedValueLength() to keep the length.

On top of your configuration ideas, I think it should also be possible to disableNumber/Boolean masking

[gavlyukovskiy]

Having a second thought here - config.preserveLength() doesn't make sense since other options per type only make sense with preserving the length.

I think it should also be possible to disableNumber/Boolean masking

I'm a bit conflicted on this, on one hand masking is explicit, but I guess it would make some sense for allow mode

[donavdey]

I like the idea of configuring masking strategy per type.
Generally speaking, that means that the config accepts a set of masking strategies, one per type. A masking strategy has "obfuscateLength", "preserveType" and "replacementValue" properties.
In this model, the builder would look like this:

config
  .numericMaskingStrategy()
    .preserveLength()
    .replacementValue(8)
    .build()
  .booleanMaskingStrategy()
    .replacementValue("****") // will not preserve the type
    .build()
  .stringMaksingStrategy()
    .replacementValue("[REDACTED]")
    .build()
  .build();

Also this potentially could be extended to array and object types:

config
  .objectMaksingStrategy()
    .applyMaskingToChilds()
    .build()
.build();
// OR
config
  .objectMaksingStrategy()
    .replacementValue("****")
    .build()
  .build();

[Breus]

To conclude this Github discussion: this had led to this ADR