Enable debug_redact for Java Logging

[anilburakbilsel]

What language does this apply to?

Java and Proto version -> libprotoc 23.4

Describe the problem you are trying to solve.

There are many ways that protocol buffers might be stringified into logs and then printed into stack traces or service/application logs, etc. The built-in behavior stringifies the entire protocol buffer (protobuf) recursively, including all field data.

So for example, assuming that Person is a protobuf generated class (so it extends com.google.protobuf.GeneratedMessageV3) and there is an instance of it "person" then System.out.print(person); will print all the fields and values.

I can see a two conversations going on around redacting sensitive information from protocol buffers when they are stringified. Those discussions can be found at:

• https://groups.google.com/g/protobuf/c/uENdMwYSgmo?pli=1
• Support data redaction, so that sensitive data in protos doesn't leak into places like log files #1160

It seems like debug_redact has been added into descriptor.proto and generate the code, I can see that the generated Java files can have access to it. However, when I try to stringfy the object, the values for debug_redactenabled fields are still being printed into the logs. I believe that happens because there is no check in toString() method for this field in Java.

Describe the solution you'd like

It seems like currently toString() method uses private printFieldValue() method while stringifyinh values - and it does not check whether the descriptor is being used or not - or whether debug_redact set to true or not. Maybe we can add a check at the beginning of that method, so that if the debug_redact is set to true then the value will not be printed. The following might be added before line 546 (in this method), where the switch statement is:

......
    private void printFieldValue(
        final FieldDescriptor field, final Object value, final TextGenerator generator)
        throws IOException {
      if (field.getOptions().hasDebugRedact() && field.getOptions().getDebugRedact()) {
        break;  // do not print anything
     }
      switch (field.getType()) {
        case INT32:
        case SINT32:
        case SFIXED32:
......
}

There are other ways to achieve this goal as well but this is a straigtforward solution I believe.

Dear @acozzette, I kindly wanted to ping you as well since you have some background information and idea about this issues. And it seems like you also recently made some changes into TextFormat.java class.

Additional context

• Seems like a possible way is already present in Go: https://github.com/bufbuild/prototransform/blame/main/filter.go

protobuf/java/core/src/main/java/com/google/protobuf/TextFormat.java

Lines 543 to 607 in 71a9ae2

	private void printFieldValue(
	final FieldDescriptor field, final Object value, final TextGenerator generator)
	throws IOException {
	switch (field.getType()) {
	case INT32:
	case SINT32:
	case SFIXED32:
	generator.print(((Integer) value).toString());
	break;
	case INT64:
	case SINT64:
	case SFIXED64:
	generator.print(((Long) value).toString());
	break;
	case BOOL:
	generator.print(((Boolean) value).toString());
	break;
	case FLOAT:
	generator.print(((Float) value).toString());
	break;
	case DOUBLE:
	generator.print(((Double) value).toString());
	break;
	case UINT32:
	case FIXED32:
	generator.print(unsignedToString((Integer) value));
	break;
	case UINT64:
	case FIXED64:
	generator.print(unsignedToString((Long) value));
	break;
	case STRING:
	generator.print("\"");
	generator.print(
	escapeNonAscii
	? TextFormatEscaper.escapeText((String) value)
	: escapeDoubleQuotesAndBackslashes((String) value).replace("\n", "\\n"));
	generator.print("\"");
	break;
	case BYTES:
	generator.print("\"");
	if (value instanceof ByteString) {
	generator.print(escapeBytes((ByteString) value));
	} else {
	generator.print(escapeBytes((byte[]) value));
	}
	generator.print("\"");
	break;
	case ENUM:
	generator.print(((EnumValueDescriptor) value).getName());
	break;
	case MESSAGE:
	case GROUP:
	print((MessageOrBuilder) value, generator);
	break;

[zhangskz]

I believe the internal proposal around debug_redact intended for toString() in Java to replace redacted fields with a placeholder value.

Let me follow up with the original proposal author to see where this is at.

[anilburakbilsel]

Hello @zhangskz , thank you. Yes, a placeholder value can be sufficient as well - I think the main purpose here is just to hide the sensitive field values when the object is converted to a string (or printed out) so that nothing sensitive will be printed within logs. In that case, as a simple solution, maybe the following can be added:

......
    private void printFieldValue(
        final FieldDescriptor field, final Object value, final TextGenerator generator)
        throws IOException {
      if (field.getOptions().hasDebugRedact() && field.getOptions().getDebugRedact()) {
        generator.print("REDACTED_VALUE");;  // print a placeholder value
     }
      switch (field.getType()) {
        case INT32:
        case SINT32:
        case SFIXED32:
......
}

Having such a change in Java will definitely make the use of debug_redact more accurate and possible.

(With that being said, it seems like in Go, the current approach is not to print out anything).

[zhangskz]

So we are definitely still planning on making this happen in Java and are tracking this proposal and implementation internally. We're still focused on C++ currently, after which we will tackle this similarly in Java.

[zhangskz]

This will likely be in ~early 2024.

[zellyn]

I assume the C++ version will take care of Ruby, etc. Are y'all planning on making the same change in Go?

(Redaction of fields marked with the squareup.redacted option is basically the main remaining reason for our internal protobuf forks, and I would love to unfork and just use upstream…)

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[Jakub-CZ]

Bump

[anilburakbilsel]

Hello @zhangskz, wanted to kindly bump this issue to remind. Thank you!