feat: JSON Masking #1899

jamfor352 · 2024-08-10T21:46:57Z

Hi!

AKHQ is fantastic, but the data masking using regex doesn't really scale well if you have complex personal data (pii) requirements (for example, if you need to mask all fields by default apart from carefully selected fields in a production environment). Crafting a regex for this can be flaky. Especially if you need to deal with multiple lines and account for integers, arrays, all sorts of data types. Or if you want to be able to expose some fields on some topics, but not others, etc.

I've been trying my fork locally which introduces a new JSON filtering system (while keeping the regex stuff entirely in place) and it works great, and I'd like to contribute it upstream. I think I've added more than sufficient tests as well, and retained backwards compatibility by defaulting to using the regex filtering if not defined (which current users won't, since mode is a new property).

It has two modes - json_mask_by_default (where everything is masked by default and you have to specifically opt in to see specific fields - useful for regulation-heavy environments) and json_show_by_default where everything shows by default but you can mask specific bits of data.

jamfor352 · 2024-08-11T14:13:19Z

I just pushed a commit fixing datetime parsing in AvroSerializer as I realised it led to some flaky tests as well. Not related to my changes here, but a nice to have (just noticed it when checking the workflow results)

AlexisSouquiere · 2024-09-12T14:52:39Z

@jamfor352 I will take time to review it and give you my feedback next week

emerfan · 2024-09-18T13:08:41Z

Hi, have been using this build locally and it's awesome - following the PR

jamfor352 · 2024-09-19T18:36:57Z

Hi, have been using this build locally and it's awesome - following the PR

Thank you :) Glad to hear it's worked well for you!

AlexisSouquiere · 2024-09-26T13:07:51Z

@jamfor352 can you please update the documentation as well ? https://github.com/tchiotludo/akhq/blob/dev/docs/docs/configuration/akhq.md#data-masking

AlexisSouquiere · 2024-09-26T13:28:24Z

src/main/java/org/akhq/utils/JsonShowByDefaultMasker.java

+
+    public Record maskRecord(Record record) {
+        try {
+            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {


You should test for record.getValue() != null first (for tombstones) to avoid generating an exception (even if this exception is catch just after)

AlexisSouquiere · 2024-09-26T13:32:13Z

src/main/java/org/akhq/utils/JsonMaskByDefaultMasker.java

+
+    public Record maskRecord(Record record) {
+        try {
+            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {


You should test for record.getValue() != null first (for tombstones) to avoid generating an exception (even if this exception is catch just after)

AlexisSouquiere · 2024-09-26T13:46:38Z

src/main/java/org/akhq/utils/JsonMaskByDefaultMasker.java

+                JsonMaskingFilter foundFilter = null;
+                for (JsonMaskingFilter filter : jsonMaskingFilters) {
+                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
+                        foundFilter = filter;
+                    }
+                }
+                if (foundFilter != null) {
+                    return applyMasking(record, foundFilter.getKeys());
+                } else {
+                    return applyMasking(record, List.of());
+                }
+            } else {
+                return record;
+            }
+        } catch (Exception e) {
+            LOG.error("Error masking record", e);
+            return record;
+        }


To improve readability, what do you think of this ?

if (record.getValue() == null) { return record; } try { jsonMaskingFilters.stream() .filter(filter -> record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) .findFirst() .ifPresentOrElse(filter -> applyMasking(record, filter.getKeys()).getValue(), () -> applyMasking(record, List.of()).getValue()); } catch (Exception e) { LOG.error("Error masking record", e); } return record;

AlexisSouquiere · 2024-09-26T13:50:02Z

src/main/java/org/akhq/utils/JsonShowByDefaultMasker.java

+                JsonMaskingFilter foundFilter = null;
+                for (JsonMaskingFilter filter : jsonMaskingFilters) {
+                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
+                        foundFilter = filter;
+                    }
+                }
+                if (foundFilter != null) {
+                    return applyMasking(record, foundFilter.getKeys());
+                } else {
+                    return record;
+                }
+            } else {
+                return record;
+            }


I think we can improve it like my previous comment (just using ifPresent and the first parameter)

AlexisSouquiere · 2024-09-26T14:19:21Z

src/main/java/org/akhq/utils/MaskerFactory.java

MaskerFactory can be removed by adding @requires annotation on the different maskers

AlexisSouquiere · 2024-09-26T14:22:20Z

src/main/java/org/akhq/utils/JsonMaskByDefaultMasker.java

+@RequiredArgsConstructor
+public class JsonMaskByDefaultMasker implements Masker {


Using this to create the masker only if we have the json_mask_by_default property will follow what we already have at some other places. And we won't need the Factory anymore :)

@Singleton @Requires(property = "akhq.security.data-masking.mode", value = "json_mask_by_default") @RequiredArgsConstructor public class JsonMaskByDefaultMasker implements Masker { @Inject private final DataMasking dataMasking;

And then use dataMasking.getJsonFilters(), etc.

src/main/java/org/akhq/utils/NoOpMasker.java

src/test/java/org/akhq/utils/JsonMaskByDefaultMaskerTest.java

AlexisSouquiere · 2024-09-26T14:30:44Z

@jamfor352 thanks for the PR it's really interesting 😄
Can you check the different comments I put ? Don't hesitate if you have a question !

jamfor352 · 2024-10-07T07:43:31Z

@jamfor352 thanks for the PR it's really interesting 😄
Can you check the different comments I put ? Don't hesitate if you have a question !

Thanks, they all seem reasonable - will get around to it sometime this week.

jamfor352 · 2024-10-18T20:48:47Z

Will update in the coming days - been busy moving house so haven't had much free time!

emerfan · 2024-10-24T14:59:29Z

Keeping it fresh in minds - I'm very interested in deploying this build, I can obviously pull it down and make the changes myself but would prefer to use the master build

jamfor352 · 2024-10-29T09:44:14Z

Keeping it fresh in minds - I'm very interested in deploying this build, I can obviously pull it down and make the changes myself but would prefer to use the master build

Hey, definitely. Was planning to do this last week but quite a lot came up, so wasn't able to get around to it. I've set some time aside specifically for this PR this evening (7pm UK time) so should be all good.

jamfor352 · 2024-10-31T09:13:25Z

Hi @AlexisSouquiere I've updated my PR based on recommendations (with one small difference, re: default masking filter, to not break current compatibility). Added documentation, too.

I've also made some other improvements that I've spotted - made the filtering more consistent (ie, always using qualified paths for both JSON maskers), and also some slight performance improvements.

cc: @emerfan could you try this new build out and confirm everything is working okay for you as well after the changes? Please note the change to json_mask_by_default where nested objects are now distinct from each other (eg metadata.value and address.value need separate entries, rather than just value) to ensure it still works 👍

emerfan · 2024-11-03T14:55:05Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further

  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results

jamfor352 · 2024-11-03T15:03:37Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results

Could you provide an example record for me to try out with my end? All the tests are passing, which is odd based on how the masker is activated. In theory the logic hasnt changed either beyond the nested field adjustment. Will take another look.

emerfan · 2024-11-03T17:56:03Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results
Could you provide an example record for me to try out with my end? All the tests are passing, which is odd based on how the masker is activated. In theory the logic hasnt changed either beyond the nested field adjustment. Will take another look.

Here is a close sample, there's nothing complex about it though... I've tested this value in the unit tests and it also passes.

            {
              "Product": "Cheese",
              "Id": 12345,
              "SellByDate": "2024-01-11T00:00:00+00:00",
              "IsDelicious": true
            }

Have you tried running it and testing with the UI / Kafka?

If I replace the updated maskRecord with your original one, it works fine. I can debug to the point I can see the masked record returned to RecordRepository but not as easy to see from there what's happening

    public Record maskRecord(Record record) {
        try {
            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {
                JsonMaskingFilter foundFilter = null;
                for (JsonMaskingFilter filter : jsonMaskingFilters) {
                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
                        foundFilter = filter;
                    }
                }
                if (foundFilter != null) {
                    return applyMasking(record, foundFilter.getKeys());
                } else {
                    return applyMasking(record, List.of());
                }
            } else {
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record", e);
            return record;
        }
    }

jamfor352 · 2024-11-03T18:31:39Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results
Could you provide an example record for me to try out with my end? All the tests are passing, which is odd based on how the masker is activated. In theory the logic hasnt changed either beyond the nested field adjustment. Will take another look.
Here is a close sample, there's nothing complex about it though... I've tested this value in the unit tests and it also passes.
            {
              "Product": "Cheese",
              "Id": 12345,
              "SellByDate": "2024-01-11T00:00:00+00:00",
              "IsDelicious": true
            }
Have you tried running it and testing with the UI / Kafka?

If I replace the updated maskRecord with your original one, it works fine. I can debug to the point I can see the masked record returned to RecordRepository but not as easy to see from there what's happening
    public Record maskRecord(Record record) {
        try {
            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {
                JsonMaskingFilter foundFilter = null;
                for (JsonMaskingFilter filter : jsonMaskingFilters) {
                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
                        foundFilter = filter;
                    }
                }
                if (foundFilter != null) {
                    return applyMasking(record, foundFilter.getKeys());
                } else {
                    return applyMasking(record, List.of());
                }
            } else {
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record", e);
            return record;
        }
    }

Can you provide your full application.yml, if possible?

emerfan · 2024-11-03T18:43:19Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results
Could you provide an example record for me to try out with my end? All the tests are passing, which is odd based on how the masker is activated. In theory the logic hasnt changed either beyond the nested field adjustment. Will take another look.
Here is a close sample, there's nothing complex about it though... I've tested this value in the unit tests and it also passes.
            {
              "Product": "Cheese",
              "Id": 12345,
              "SellByDate": "2024-01-11T00:00:00+00:00",
              "IsDelicious": true
            }
Have you tried running it and testing with the UI / Kafka?
If I replace the updated maskRecord with your original one, it works fine. I can debug to the point I can see the masked record returned to RecordRepository but not as easy to see from there what's happening
    public Record maskRecord(Record record) {
        try {
            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {
                JsonMaskingFilter foundFilter = null;
                for (JsonMaskingFilter filter : jsonMaskingFilters) {
                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
                        foundFilter = filter;
                    }
                }
                if (foundFilter != null) {
                    return applyMasking(record, foundFilter.getKeys());
                } else {
                    return applyMasking(record, List.of());
                }
            } else {
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record", e);
            return record;
        }
    }
Can you provide your full application.yml, if possible?

akhq:
  connections:
    local:
      properties:
        bootstrap.servers: "localhost:9092"
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Product

jamfor352 · 2024-11-03T19:00:45Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results
Could you provide an example record for me to try out with my end? All the tests are passing, which is odd based on how the masker is activated. In theory the logic hasnt changed either beyond the nested field adjustment. Will take another look.
Here is a close sample, there's nothing complex about it though... I've tested this value in the unit tests and it also passes.
            {
              "Product": "Cheese",
              "Id": 12345,
              "SellByDate": "2024-01-11T00:00:00+00:00",
              "IsDelicious": true
            }
Have you tried running it and testing with the UI / Kafka?
If I replace the updated maskRecord with your original one, it works fine. I can debug to the point I can see the masked record returned to RecordRepository but not as easy to see from there what's happening
    public Record maskRecord(Record record) {
        try {
            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {
                JsonMaskingFilter foundFilter = null;
                for (JsonMaskingFilter filter : jsonMaskingFilters) {
                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
                        foundFilter = filter;
                    }
                }
                if (foundFilter != null) {
                    return applyMasking(record, foundFilter.getKeys());
                } else {
                    return applyMasking(record, List.of());
                }
            } else {
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record", e);
            return record;
        }
    }
Can you provide your full application.yml, if possible?
akhq:
  connections:
    local:
      properties:
        bootstrap.servers: "localhost:9092"
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Product

I'm wondering if it's something to do with the "disable if no filters found" (which I have realised probably shouldnt be in the mask by default mode) nd there being a bug with determining this, or perhaps the JSON parsing

Thanks for the YAML - I'll check it out later.

emerfan · 2024-11-03T19:26:47Z

Hi @jamfor352 this isn't working for me .... debugging I can see that masking is being applied for the record (I'm using json_mask_by_default, but the unmasked record is being returned to the UI. Sanity checked by dropping your original try/catch logic in to the maskRecord method and that's still working. I'm guessing something funky up with the RecordRepository / how it's returning the record maybe - I'll get back to it later and dig further
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Results
Could you provide an example record for me to try out with my end? All the tests are passing, which is odd based on how the masker is activated. In theory the logic hasnt changed either beyond the nested field adjustment. Will take another look.
Here is a close sample, there's nothing complex about it though... I've tested this value in the unit tests and it also passes.
            {
              "Product": "Cheese",
              "Id": 12345,
              "SellByDate": "2024-01-11T00:00:00+00:00",
              "IsDelicious": true
            }
Have you tried running it and testing with the UI / Kafka?
If I replace the updated maskRecord with your original one, it works fine. I can debug to the point I can see the masked record returned to RecordRepository but not as easy to see from there what's happening
    public Record maskRecord(Record record) {
        try {
            if(record.getValue().trim().startsWith("{") && record.getValue().trim().endsWith("}")) {
                JsonMaskingFilter foundFilter = null;
                for (JsonMaskingFilter filter : jsonMaskingFilters) {
                    if (record.getTopic().getName().equalsIgnoreCase(filter.getTopic())) {
                        foundFilter = filter;
                    }
                }
                if (foundFilter != null) {
                    return applyMasking(record, foundFilter.getKeys());
                } else {
                    return applyMasking(record, List.of());
                }
            } else {
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record", e);
            return record;
        }
    }
Can you provide your full application.yml, if possible?
akhq:
  connections:
    local:
      properties:
        bootstrap.servers: "localhost:9092"
  security:
    data-masking:
      mode: json_mask_by_default
      json-filters:
        - description: Unmask non-sensitive values
          topic: results
          keys:
            - Product
I'm wondering if it's something to do with the "disable if no filters found" (which I have realised probably shouldnt be in the mask by default mode) nd there being a bug with determining this, or perhaps the JSON parsing

Thanks for the YAML - I'll check it out later.

Just to make this slightly weirder ....

For easier testing, I created a topic and published a single message to it.

When I run the application, the record is returned unmasked. When I run in debug mode and debug through the masker, the record is eventually returned (to the UI) masked as expected ... 😕

jamfor352 · 2024-11-03T23:37:44Z

Just to make this slightly weirder ....

For easier testing, I created a topic and published a single message to it.

When I run the application, the record is returned unmasked. When I run in debug mode and debug through the masker, the record is eventually returned (to the UI) masked as expected ... 😕

I'm not entirely sure what the issue here is... but I've changed a few things relating to how masking is validated to work (removed the enabled functionality since it doesn't really bring much and breaks the mask by default flow, and switched JSON checking to a different library which may be better suited)

emerfan · 2024-11-04T18:59:04Z

Just to make this slightly weirder ....
For easier testing, I created a topic and published a single message to it.
When I run the application, the record is returned unmasked. When I run in debug mode and debug through the masker, the record is eventually returned (to the UI) masked as expected ... 😕

I'm not entirely sure what the issue here is... but I've changed a few things relating to how masking is validated to work (removed the enabled functionality since it doesn't really bring much and breaks the mask by default flow, and switched JSON checking to a different library which may be better suited)

Does this work as expected for you against a cluster? I'm still seeing the debug vs run issue

emerfan · 2024-11-04T20:35:45Z

Just to make this slightly weirder ....
For easier testing, I created a topic and published a single message to it.
When I run the application, the record is returned unmasked. When I run in debug mode and debug through the masker, the record is eventually returned (to the UI) masked as expected ... 😕

I'm not entirely sure what the issue here is... but I've changed a few things relating to how masking is validated to work (removed the enabled functionality since it doesn't really bring much and breaks the mask by default flow, and switched JSON checking to a different library which may be better suited)

Does this work as expected for you against a cluster? I'm still seeing the debug vs run issue

I've found the issue ... it's with the new isTombstone and isJson methods. The record is initially null when it hits them.

This works:

public interface Masker {

    Logger LOG = LoggerFactory.getLogger(Masker.class);

    Record maskRecord(Record record);

    default boolean isTombstone(Record record) {
        return record.getValue() == null;
    }

   default boolean isJson(Record record) {
        if(isTombstone(record)) {
            return false;
        }
        try {
            new JSONObject(record.getValue());
        } catch (JSONException ex) {
            try {
                new JSONArray(record.getValue());
            } catch (JSONException ex1) {
                return false;
            }
        }
        return true;
    }
}

and

    public Record maskRecord(Record record) {
        try {
            if(isTombstone(record)) {
                LOG.debug("Record at topic {}, partition {}, offset {} is a tombstone, so not masking.", record.getTopic(), record.getPartition(), record.getOffset());
                return record;
            } else if(isJson(record)) {
                return jsonMaskingFilters
                    .stream()
                    .filter(jsonMaskingFilter -> record.getTopic().getName().equalsIgnoreCase(jsonMaskingFilter.getTopic()))
                    .findFirst()
                    .map(filter -> applyMasking(record, filter.getKeys()))
                    .orElseGet(() -> applyMasking(record, List.of()));
            } else {
                LOG.debug("Record at topic {}, partition {}, offset {} is not JSON, so not masking.", record.getTopic(), record.getPartition(), record.getOffset());
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record at topic {}, partition {}, offset {} due to {}", record.getTopic(), record.getPartition(), record.getOffset(), e.getMessage());
            record.setValue("An exception occurred during an attempt to mask this record. This record is unavailable to view due to safety measures from json_mask_by_default to not leak sensitive data. Please contact akhq administrator.");
            return record;
        }
    }

jamfor352 · 2024-11-05T12:59:47Z

Just to make this slightly weirder ....
For easier testing, I created a topic and published a single message to it.
When I run the application, the record is returned unmasked. When I run in debug mode and debug through the masker, the record is eventually returned (to the UI) masked as expected ... 😕

I'm not entirely sure what the issue here is... but I've changed a few things relating to how masking is validated to work (removed the enabled functionality since it doesn't really bring much and breaks the mask by default flow, and switched JSON checking to a different library which may be better suited)

Does this work as expected for you against a cluster? I'm still seeing the debug vs run issue

I've found the issue ... it's with the new isTombstone and isJson methods. The record is initially null when it hits them.

This works:

public interface Masker {

    Logger LOG = LoggerFactory.getLogger(Masker.class);

    Record maskRecord(Record record);

    default boolean isTombstone(Record record) {
        return record.getValue() == null;
    }

   default boolean isJson(Record record) {
        if(isTombstone(record)) {
            return false;
        }
        try {
            new JSONObject(record.getValue());
        } catch (JSONException ex) {
            try {
                new JSONArray(record.getValue());
            } catch (JSONException ex1) {
                return false;
            }
        }
        return true;
    }
}

and

    public Record maskRecord(Record record) {
        try {
            if(isTombstone(record)) {
                LOG.debug("Record at topic {}, partition {}, offset {} is a tombstone, so not masking.", record.getTopic(), record.getPartition(), record.getOffset());
                return record;
            } else if(isJson(record)) {
                return jsonMaskingFilters
                    .stream()
                    .filter(jsonMaskingFilter -> record.getTopic().getName().equalsIgnoreCase(jsonMaskingFilter.getTopic()))
                    .findFirst()
                    .map(filter -> applyMasking(record, filter.getKeys()))
                    .orElseGet(() -> applyMasking(record, List.of()));
            } else {
                LOG.debug("Record at topic {}, partition {}, offset {} is not JSON, so not masking.", record.getTopic(), record.getPartition(), record.getOffset());
                return record;
            }
        } catch (Exception e) {
            LOG.error("Error masking record at topic {}, partition {}, offset {} due to {}", record.getTopic(), record.getPartition(), record.getOffset(), e.getMessage());
            record.setValue("An exception occurred during an attempt to mask this record. This record is unavailable to view due to safety measures from json_mask_by_default to not leak sensitive data. Please contact akhq administrator.");
            return record;
        }
    }

Good catch! I put this in the domain object as I thought it would be cleaner.
I'll update my branch with this tonight

jamfor352 · 2024-11-05T19:19:24Z

@emerfan updated my branch, so it should all be working out of the box for you now.

@AlexisSouquiere can I get a re-review please now changes have been applied and bugs fixed? 🙏

emerfan · 2024-11-05T20:57:42Z

@emerfan updated my branch, so it should all be working out of the box for you now.

@AlexisSouquiere can I get a re-review please now changes have been applied and bugs fixed? 🙏

Looks great! I've run it against a substantial cluster. I like the mask all topics by default, and the updated handling of nested objects. This is a great feature, thanks!

AlexisSouquiere · 2024-11-14T17:34:47Z

@jamfor352 I will review it tomorrow !

jamfor352 · 2024-11-14T17:35:31Z

@jamfor352 I will review it tomorrow !

Thank you!

AlexisSouquiere

Few updates needed and then it's good for me

AlexisSouquiere · 2024-11-15T13:40:38Z

src/main/java/org/akhq/configs/DataMasking.java

+   List<RegexFilter> filters = new ArrayList<>(); // regex filters still use `filters` for backwards compatibility
+   List<JsonMaskingFilter> jsonFilters = new ArrayList<>();
+   String jsonMaskReplacement = "xxxx";
+   boolean cachingEnabled = false;


No reference to cachingEnabled, should be removed ?

AlexisSouquiere · 2024-11-15T13:42:08Z

src/main/java/org/akhq/models/Record.java

+import com.google.gson.JsonParser;
+import com.google.gson.JsonSyntaxException;


Unused imports

AlexisSouquiere · 2024-11-15T13:42:26Z

src/main/java/org/akhq/models/Record.java

+import org.json.JSONArray;
+import org.json.JSONException;
+import org.json.JSONObject;


Unused imports

AlexisSouquiere · 2024-11-15T13:43:15Z

src/main/java/org/akhq/models/Record.java

+    public void setTopic(Topic topic) {
+        this.topic = topic;
+    }
+


Prefer adding Lombok @Setter on the topic field

src/main/java/org/akhq/utils/JsonShowByDefaultMasker.java

AlexisSouquiere · 2024-11-15T13:52:35Z

src/main/java/org/akhq/utils/RegexMasker.java

+
+    public RegexMasker(DataMasking  dataMasking ) {
+        this.filters = dataMasking.getFilters();
+    }    @Override


jamfor352 · 2024-11-16T16:50:05Z

Hey @AlexisSouquiere applied all the changes requested. Although I also realised I may as well inline the isTombstone logic into the isJson since it was no longer worth having the separate isTombstone method anyway since it's never used elsewhere and the logical conclusion only really makes sense here.

(Note - all the commits below are the same, I just rebased on upstream - the last commit mentioned is the only change since the review)

Hope this looks all OK now 👍

…sing map rather than rely on side effects

…sking is not correctly applied.

AlexisSouquiere

Sounds good to me !

jamfor352 · 2024-11-19T21:17:07Z

Sounds good to me !

Thanks for the approval! What is the process in terms of getting this upstream, does it need more reviewers?

tchiotludo · 2024-11-19T21:41:34Z

@jamfor352 nothing required, thanks for the contributions 👏

jamfor352 force-pushed the dev branch 4 times, most recently from c5bf417 to 84d5d58 Compare September 10, 2024 19:10

AlexisSouquiere self-requested a review September 12, 2024 14:52

jamfor352 force-pushed the dev branch from 84d5d58 to 3437ae0 Compare September 19, 2024 16:03

AlexisSouquiere requested changes Sep 26, 2024

View reviewed changes

jamfor352 force-pushed the dev branch from 3437ae0 to 8d39a7c Compare October 29, 2024 18:59

jamfor352 requested a review from AlexisSouquiere October 29, 2024 19:06

jamfor352 force-pushed the dev branch 3 times, most recently from 02eeaf2 to 7fb2499 Compare October 29, 2024 19:33

jamfor352 force-pushed the dev branch from 24f5760 to 0adedba Compare November 3, 2024 23:33

jamfor352 force-pushed the dev branch from 6169050 to ffe9fe3 Compare November 3, 2024 23:54

AlexisSouquiere requested changes Nov 15, 2024

View reviewed changes

jamfor352 requested a review from AlexisSouquiere November 16, 2024 16:50

jamfor352 added 12 commits November 17, 2024 16:25

feat(masking): JSON Masking

3e2816a

fix(masking): Update as per PR recommendations.

8a1eed3

fix(masking): Remove DataMaskingMode

c7a4bfa

fix(masking): Fix edge case and don't rely on string interpolation

fcc4af7

fix(masking): Consistency & performance improvements

9f59e14

fix(masking): Update docs

fca3aa0

fix(masking): typo

9e1d2f3

fix(masking): disable 'enabled' functionality and directly return u…

aa9dbbd

…sing map rather than rely on side effects

fix(making): use JSONObject library instead of JSONParseString

4412c8a

fix(masking): In event of an exception, return placeholder text.

bd84db8

fix(masking): Fix reported bug where, under certain circumstances, ma…

6ba2a65

…sking is not correctly applied.

fix(masking): PR response

6572abc

jamfor352 force-pushed the dev branch from 9e4abfd to 6572abc Compare November 17, 2024 16:26

AlexisSouquiere approved these changes Nov 18, 2024

View reviewed changes

tchiotludo merged commit 836d996 into tchiotludo:dev Nov 19, 2024
4 checks passed

		@RequiredArgsConstructor
		public class JsonMaskByDefaultMasker implements Masker {

		import com.google.gson.JsonParser;
		import com.google.gson.JsonSyntaxException;

feat: JSON Masking #1899

feat: JSON Masking #1899

Conversation

jamfor352 commented Aug 10, 2024 • edited Loading

jamfor352 commented Aug 11, 2024 • edited Loading

AlexisSouquiere commented Sep 12, 2024

emerfan commented Sep 18, 2024

jamfor352 commented Sep 19, 2024

AlexisSouquiere commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexisSouquiere commented Sep 26, 2024 • edited Loading

jamfor352 commented Oct 7, 2024

jamfor352 commented Oct 18, 2024

emerfan commented Oct 24, 2024

jamfor352 commented Oct 29, 2024

jamfor352 commented Oct 31, 2024

emerfan commented Nov 3, 2024 • edited Loading

jamfor352 commented Nov 3, 2024 • edited Loading

emerfan commented Nov 3, 2024

jamfor352 commented Nov 3, 2024

emerfan commented Nov 3, 2024 • edited Loading

jamfor352 commented Nov 3, 2024 • edited Loading

emerfan commented Nov 3, 2024 • edited Loading

jamfor352 commented Nov 3, 2024

emerfan commented Nov 4, 2024

emerfan commented Nov 4, 2024

jamfor352 commented Nov 5, 2024

jamfor352 commented Nov 5, 2024

emerfan commented Nov 5, 2024

AlexisSouquiere commented Nov 14, 2024

jamfor352 commented Nov 14, 2024

AlexisSouquiere left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamfor352 commented Nov 16, 2024 • edited Loading

AlexisSouquiere left a comment

Choose a reason for hiding this comment

jamfor352 commented Nov 19, 2024

tchiotludo commented Nov 19, 2024

jamfor352 commented Aug 10, 2024 •

edited

Loading

jamfor352 commented Aug 11, 2024 •

edited

Loading

AlexisSouquiere commented Sep 26, 2024 •

edited

Loading

emerfan commented Nov 3, 2024 •

edited

Loading

jamfor352 commented Nov 3, 2024 •

edited

Loading

emerfan commented Nov 3, 2024 •

edited

Loading

jamfor352 commented Nov 3, 2024 •

edited

Loading

emerfan commented Nov 3, 2024 •

edited

Loading

AlexisSouquiere left a comment •

edited

Loading

jamfor352 commented Nov 16, 2024 •

edited

Loading