Added properties for base64 formatting #504

kuzjka · 2025-08-11T17:21:13Z

New feature

This PR continues the mailing list thread and suggests system properties to control base64Binary values formatting:

org.apache.xml.security.base64.lineSeparator, org.apache.xml.security.base64.lineLength - allows to override default MIME encoding settings
org.apache.xml.security.base64.ignoreLineBreaks - disables line wrapping for base64 value, but allows to keep the whole XML pretty-printed. Takes precedence over line wrapping settings above.
existing org.apache.xml.security.ignoreLineBreaks takes precedence over all new options.

Default values result to CRLF line breaks with 76 chars in the line, making the whole thing fully compatible with previous implementation.

Other changes

Some of the logic is included to XMLUtils to provide better encapsulation:

adding line breaks in the beginning and in the end of wrapped base64 values in the element is implemented as XMLUtils.encodeElementValue
Base64OutputStream from Commons, used in XML encryption, is replaced with a java.util implementation and provided by XMLUtils, making it consistent with configured Base64 encoder.

Base64.Encoder/Decoder instances are thread-safe, so sharing a single instance should give us a little performance gain.

Test coverage

As the base64 configuration takes place during class load, unit-tests leverage from custom ClassLoader and Reflection API to reinitialize the class in each test.

Motivation

https://www.w3.org/TR/xmlschema-2/#base64Binary - base64Binary definition has note, that original RFC2045 line-length limitation must not be enforced. In current implementation it is only possible to remove all line breaks in the document using org.apache.xml.security.ignoreLineBreaks, sacrificing human readability.

Also, using LF instead of CRLF may be desired in systems where verifying side does not expect escape sequences in base64Binary values.

I would be thankful for your comments and suggestions.

seanjmullan · 2025-10-31T21:01:20Z

How about also removing org.apache.xml.security.utils.Base64? Is it needed anymore?

kuzjka · 2025-10-31T21:42:51Z

I can't see any usage of this class inside Santuario. However, I didn't touch it to avoid breaking changes. Despite deprecated, I can see it was retained in 8016fc3 for compatibility. Probably, @coheigea would give us better insight.

seanjmullan

Some comments on a first pass.

seanjmullan · 2025-10-31T20:48:46Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

 * DOM and XML accessibility and comfort functions.
 *
+ * @implNote
+ * Following system properties affect XML formatting:


Nit: "The following ..."

fixed in 8a598d0, thank you

seanjmullan · 2025-11-04T14:17:46Z

.../java/org/apache/xml/security/stax/impl/processor/output/AbstractEncryptOutputProcessor.java

-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
+import java.util.*;


Can you revert this change? I think it is better not to wildcard imports.

Oops, my IDE did it :) reverted

seanjmullan · 2025-11-04T14:49:53Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

+            return ignoreLineBreaks;
+        }
+
+        public void setIgnoreLineBreaks(boolean ignoreLineBreaks) {


Instead of set methods, why not add a constructor that takes the options and also checks for illegal syntax?

Since we would like to retain meaningful log messages for invalid property values, I couldn't find a better way than to move all properties reading together with validation logic to the constructor of Base64FormattingOptions.

Added warnings in case a property is set, but ignored.
Added handling of NumberFormatException as suggested in the comment below.

Example log:

testIgnoreLineBreaksTakesPrecedence: ... org.apache.xml.security.ignoreLineBreaks property takes precedence over org.apache.xml.security.base64.ignoreLineBreaks, line breaks will be ignored ... Property org.apache.xml.security.base64.lineSeparator has no effect since line breaks are ignored ... Property org.apache.xml.security.base64.lineLength has no effect since line breaks are ignored testIllegalPropertiesAreIgnored: ... Illegal value of org.apache.xml.security.base64.lineSeparator property is ignored: illegal ... Illegal value of org.apache.xml.security.base64.lineLength property is ignored: illegal

seanjmullan · 2025-11-04T14:57:45Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

-                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean("org.apache.xml.security.ignoreLineBreaks"));
+                    (PrivilegedAction<Boolean>) () -> Boolean.getBoolean(IGNORE_LINE_BREAKS_PROP));
+
+    private static Base64FormattingOptions base64Formatting =


if org.apache.xml.security.ignoreLineBreaks is true, none of these options matter, so did you consider skipping the getting of the other properties?

Also should log a warning if the other properties are set, since they have no impact if org.apache.xml.security.ignoreLineBreaks is true.

seanjmullan · 2025-11-04T16:06:30Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

+                }

-    private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
+                Integer lineLength = Integer.getInteger(BASE64_LINE_LENGTH_PROP);


I think it would be better to use System.getProperty and then Integer.valueOf(String) because Integer.getInteger does not distinguish between the property not being set and an invalid value for the integer.

seanjmullan · 2025-11-07T13:53:46Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

+        /**
+         * Creates new formatting options by reading system properties.
+         */
+        public Base64FormattingOptions() {


The constructors and methods of this class should be package-private since the class is package-private.

seanjmullan · 2025-11-07T13:55:12Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

+            this.bytes = bytes;
+        }
+
+        public byte[] getBytes() {


Change to package-private.

seanjmullan · 2025-11-07T13:59:46Z

src/test/java/org/apache/xml/security/utils/XMLUtilsTest.java

Missing Apache license header.

seanjmullan · 2025-11-07T14:01:32Z

src/test/java/org/apache/xml/security/utils/XMLUtilsTest.java

+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.*;
+import static org.junit.jupiter.api.Assertions.*;
+


Add a comment describing what this test does.

seanjmullan · 2025-11-07T14:24:43Z

src/main/java/org/apache/xml/security/utils/XMLUtils.java

Did you consider testing the properties by generating signed XML and then checking the format? This test won't catch issues where the internal methods are not called accidentally.

The fact that XMLUtils is a utility class with static methods, and the configuration can only be done with system properties at the class load time makes it hard to test. That's why the test contains custom class loader which reloads the class in each test, allowing to run static fields initialization in the middle of the test.

To build a sort of integration test against public API e.g. XMLSignature, I believe we will need to load XMLSignature and all the classes it depends on with this custom classloader. There are additional considerations regarding Jigsaw module system: since the new classloader loads classes in its own module, all packages from xmlsec which are not exported cannot be used by this classloader. I can try to implement such a test, but I'm afraid it will be totally unreadable...

Intuitively, a good test interacts with the code in a way it is intended to be used. In our case, parts of the public interface are system properties. It looks proper to run the test suite with different JVMs started with different properties. Probably, Maven has something to achieve this, I'll investigate.

On the other hand, we can make it more testable (and probably a little more convenient for usage) if we provide a way to set Base64 options in runtime, for example:

refer to system properties and resolve the options each time we do base64 encoding (e.g. inside encodeToString etc.)

add public setters for base64FormattingOptions and ignoreLineBreaks

add public setter for base64Encoder - then we don't need new options at all. We can default to Base64.getEncoder() / Base64.getMimeEncoder() depending on ignoreLineBreaks, and if the clients need custom formatting - they just provide any implementation of Base64.Encoder

However, I'm not aware of security implications behind this...

All other inline comments were addressed in 8a598d0. Thank you for checking the code carefully.

…e test

Added properties for base64 formatting

e3473b0

seanjmullan reviewed Nov 4, 2025

View reviewed changes

updated XMLUtils in response to review comments

8a598d0

seanjmullan reviewed Nov 7, 2025

View reviewed changes

review: fixed access modifiers, added license and more comments in th…

8e67aaf

…e test

Added properties for base64 formatting #504

Are you sure you want to change the base?

Added properties for base64 formatting #504

Uh oh!

Conversation

kuzjka commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New feature

Other changes

Test coverage

Motivation

Uh oh!

seanjmullan commented Oct 31, 2025

Uh oh!

kuzjka commented Oct 31, 2025

Uh oh!

seanjmullan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kuzjka commented Aug 11, 2025 •

edited

Loading