Skip to content

Added properties for base64 formatting #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kuzjka wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Added properties for base64 formatting #504

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3473b0
Added properties for base64 formatting
kuzjka Aug 11, 2025
8a598d0
updated XMLUtils in response to review comments
kuzjka Nov 5, 2025
8e67aaf
review: fixed access modifiers, added license and more comments in th…
kuzjka Nov 7, 2025
908c97f
added framework for formatting tests, refactored XMLUtilsTest
kuzjka Nov 11, 2025
9e3fdac
added high-level formatting tests for signature and encryption
kuzjka Dec 6, 2025
969eb8d
fixed grammar and codestyle in XMLUtilsTest.java
kuzjka Dec 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/main/java/org/apache/xml/security/signature/XMLSignature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,11 +684,7 @@ private void setSignatureValueElement(byte[] bytes) {
signatureValueElement.removeChild(signatureValueElement.getFirstChild());
}

String base64codedValue = XMLUtils.encodeToString(bytes);

if (base64codedValue.length() > 76 && !XMLUtils.ignoreLineBreaks()) {
base64codedValue = "\n" + base64codedValue + "\n";
}
String base64codedValue = XMLUtils.encodeElementValue(bytes);

Text t = createText(base64codedValue);
signatureValueElement.appendChild(t);
Expand Down
15 changes: 2 additions & 13 deletions ...va/org/apache/xml/security/stax/impl/processor/output/AbstractEncryptOutputProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,7 @@
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.spec.AlgorithmParameterSpec;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.*;
Copy link
Member

@seanjmullan seanjmullan Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@kuzjka kuzjka Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, my IDE did it :) reverted

import java.util.Map.Entry;

import javax.crypto.Cipher;
Expand All @@ -40,7 +35,6 @@
import javax.xml.stream.XMLStreamConstants;
import javax.xml.stream.XMLStreamException;

import org.apache.commons.codec.binary.Base64OutputStream;
import org.apache.xml.security.algorithms.JCEMapper;
import org.apache.xml.security.encryption.XMLCipherUtil;
import org.apache.xml.security.exceptions.XMLSecurityException;
Expand Down Expand Up @@ -175,12 +169,7 @@ public void init(OutputProcessorChain outputProcessorChain) throws XMLSecurityEx
symmetricCipher.init(Cipher.ENCRYPT_MODE, encryptionPartDef.getSymmetricKey(), parameterSpec);

characterEventGeneratorOutputStream = new CharacterEventGeneratorOutputStream();
Base64OutputStream base64EncoderStream = null; //NOPMD
if (XMLUtils.isIgnoreLineBreaks()) {
base64EncoderStream = new Base64OutputStream(characterEventGeneratorOutputStream, true, 0, null);
} else {
base64EncoderStream = new Base64OutputStream(characterEventGeneratorOutputStream, true);
}
OutputStream base64EncoderStream = XMLUtils.encodeStream(characterEventGeneratorOutputStream); //NOPMD
Copy link
Member

@seanjmullan seanjmullan Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this NOPMD comment? @coheigea do you know?

base64EncoderStream.write(iv);

OutputStream outputStream = new CipherOutputStream(base64EncoderStream, symmetricCipher); //NOPMD
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/org/apache/xml/security/utils/ElementProxy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,7 @@ public void addTextElement(String text, String localname) {
*/
public void addBase64Text(byte[] bytes) {
if (bytes != null) {
Text t = XMLUtils.ignoreLineBreaks()
? createText(XMLUtils.encodeToString(bytes))
: createText("\n" + XMLUtils.encodeToString(bytes) + "\n");
Text t = createText(XMLUtils.encodeElementValue(bytes));
appendSelf(t);
}
}
Expand Down
141 changes: 134 additions & 7 deletions src/main/java/org/apache/xml/security/utils/XMLUtils.java
View file
Open in desktop
Copy link
Member

@seanjmullan seanjmullan Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@kuzjka kuzjka Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 8e67aaf. Thank you for checking the code carefully.

Copy link
Member

@seanjmullan seanjmullan Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a limitation of junit in that it isn't possible or easy to run tests with different system property settings. Anyway, I don't think you should spend an inordinate amount of time trying to address my concern. See what you can do, but if it seems like too much work or is not feasible for some reason, let's just use the current test. I would prefer to avoid adding hooks to set the options at runtime as that has the potential to be misused. Thanks.

Copy link
Author

@kuzjka kuzjka Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, take a look at 908c97f

We can check the formatting in various scenarios with these steps:

  1. Annotate test class or method with @FormattingTest (this adds a JUnit tag)
  2. Get a FormattingChecker instance using FormattingCheckerFactory - it returns an implementation depending on system properties
  3. Implement test scenario, i.e. create an XML signature.
  4. Check line breaks in the whole document and formatting of Base64 values using FormattingChecker
  5. Maven Surefire takes care to run the tagged tests with various configurations because of additional <executions>

Looks a little clearer than cheating with class loaders.

I started by refactoring the XMLUtilsTest, which can be retained as a unit test suite. It looks much simpler now.
Now, if the approach looks good, I can add a few high-level tests for basic scenarios (signature/encryption, DOM/StAX)

Copy link
Member

@seanjmullan seanjmullan Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, this new approach looks good.

Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,63 @@
/**
* DOM and XML accessibility and comfort functions.
*
* @implNote
* Following system properties affect XML formatting:
Copy link
Member

@seanjmullan seanjmullan Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "The following ..."

Copy link
Author

@kuzjka kuzjka Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 8a598d0, thank you

* <ul>
* <li>{@systemProperty org.apache.xml.security.ignoreLineBreaks} - ignores all line breaks,
* making a single-line document. Overrides all other formatting options. Default: false</li>
* <li>{@systemProperty org.apache.xml.security.base64.ignoreLineBreaks} - ignores line breaks in base64Binary values.
* Takes precedence over line length and separator options (see below). Default: false</li>
* <li>{@systemProperty org.apache.xml.security.base64.lineSeparator} - Sets the line separator sequence in base64Binary values.
* Possible values: crlf, lf. Default: crlf</li>
* <li>{@systemProperty org.apache.xml.security.base64.lineLength} - Sets maximum line length in base64Binary values.
* The value is rounded down to nearest multiple of 4. Values less than 4 are ignored. Default: 76</li>
* </ul>
*/
public final class XMLUtils {

private static final Logger LOG = System.getLogger(XMLUtils.class.getName());

private static final String IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.ignoreLineBreaks";
private static final String BASE64_IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.base64.ignoreLineBreaks";
private static final String BASE64_LINE_SEPARATOR_PROP = "org.apache.xml.security.base64.lineSeparator";
private static final String BASE64_LINE_LENGTH_PROP = "org.apache.xml.security.base64.lineLength";

private static boolean ignoreLineBreaks =
AccessController.doPrivileged(
(PrivilegedAction<Boolean>) () -> Boolean.getBoolean("org.apache.xml.security.ignoreLineBreaks"));
(PrivilegedAction<Boolean>) () -> Boolean.getBoolean(IGNORE_LINE_BREAKS_PROP));

private static Base64FormattingOptions base64Formatting =
Copy link
Member

@seanjmullan seanjmullan Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@seanjmullan seanjmullan Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AccessController.doPrivileged((PrivilegedAction<Base64FormattingOptions>) () -> {
Base64FormattingOptions options = new Base64FormattingOptions();
options.setIgnoreLineBreaks(Boolean.getBoolean(BASE64_IGNORE_LINE_BREAKS_PROP));

String lineSeparator = System.getProperty(BASE64_LINE_SEPARATOR_PROP);
if (lineSeparator != null) {
try {
options.setLineSeparator(Base64LineSeparator.valueOf(lineSeparator.toUpperCase()));
} catch (IllegalArgumentException e) {
LOG.log(Level.WARNING, "Illegal value of {0} property ignored: {1}",
BASE64_LINE_SEPARATOR_PROP, lineSeparator);
}
}

private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
Integer lineLength = Integer.getInteger(BASE64_LINE_LENGTH_PROP);
Copy link
Member

@seanjmullan seanjmullan Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (lineLength != null && lineLength >= 4) {
options.setLineLength(lineLength);
} else if (lineLength != null) {
LOG.log(Level.WARNING, "Illegal value of {0} property ignored: {1}",
BASE64_LINE_LENGTH_PROP, lineLength);
}

return options;
});

private static Base64.Encoder base64Encoder = (ignoreLineBreaks || base64Formatting.isIgnoreLineBreaks()) ?
Base64.getEncoder() :
Base64.getMimeEncoder(base64Formatting.getLineLength(), base64Formatting.getLineSeparator().getBytes());

private static Base64.Decoder base64Decoder = Base64.getMimeDecoder();

private static XMLParser xmlParserImpl =
AccessController.doPrivileged(
Expand Down Expand Up @@ -515,18 +564,48 @@ public static void addReturnBeforeChild(Element e, Node child) {
}

public static String encodeToString(byte[] bytes) {
if (ignoreLineBreaks) {
return Base64.getEncoder().encodeToString(bytes);
return base64Encoder.encodeToString(bytes);
}

/**
* Encodes bytes using Base64, with or without line breaks, depending on configuration (see {@link XMLUtils}).
* @param bytes Bytes to encode
* @return Base64 string
*/
public static String encodeElementValue(byte[] bytes) {
String encoded = encodeToString(bytes);
if (!ignoreLineBreaks && !base64Formatting.isIgnoreLineBreaks()
&& encoded.length() > base64Formatting.getLineLength()) {
encoded = "\n" + encoded + "\n";
}
return Base64.getMimeEncoder().encodeToString(bytes);
return encoded;
}

/**
* Wraps output stream for Base64 encoding.
* Output data may contain line breaks or not, depending on configuration (see {@link XMLUtils})
* @param stream The underlying output stream to write Base64-encoded data
* @return Stream which writes binary data using Base64 encoder
*/
public static OutputStream encodeStream(OutputStream stream) {
return base64Encoder.wrap(stream);
}

public static byte[] decode(String encodedString) {
return Base64.getMimeDecoder().decode(encodedString);
return base64Decoder.decode(encodedString);
}

public static byte[] decode(byte[] encodedBytes) {
return Base64.getMimeDecoder().decode(encodedBytes);
return base64Decoder.decode(encodedBytes);
}

/**
* Wraps input stream for Base64 decoding.
* @param stream Input stream with Base64-encoded data
* @return Input stream with decoded binary data
*/
public static InputStream decodeStream(InputStream stream) {
return base64Decoder.wrap(stream);
}

public static boolean isIgnoreLineBreaks() {
Expand Down Expand Up @@ -1068,4 +1147,52 @@ public static byte[] getBytes(BigInteger big, int bitlen) {

return resizedBytes;
}

/**
* Aggregates formatting options for base64Binary values.
*/
static class Base64FormattingOptions {
private boolean ignoreLineBreaks = false;
private Base64LineSeparator lineSeparator = Base64LineSeparator.CRLF;
private int lineLength = 76;

public boolean isIgnoreLineBreaks() {
return ignoreLineBreaks;
}

public void setIgnoreLineBreaks(boolean ignoreLineBreaks) {
Copy link
Member

@seanjmullan seanjmullan Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@kuzjka kuzjka Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.ignoreLineBreaks = ignoreLineBreaks;
}

public Base64LineSeparator getLineSeparator() {
return lineSeparator;
}

public void setLineSeparator(Base64LineSeparator lineSeparator) {
this.lineSeparator = lineSeparator;
}

public int getLineLength() {
return lineLength;
}

public void setLineLength(int lineLength) {
this.lineLength = lineLength;
}
}

enum Base64LineSeparator {
CRLF(new byte[]{'\r', '\n'}),
LF(new byte[]{'\n'});

private byte[] bytes;

Base64LineSeparator(byte[] bytes) {
this.bytes = bytes;
}

public byte[] getBytes() {
Copy link
Member

@seanjmullan seanjmullan Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to package-private.

return bytes;
}
}
}
Loading