Patch from #30491 "Generate right-length node name" seems invalid

[graben]

Describe the bug

A port of #30491 to the narayana-spring-boot project snowdrop/narayana-spring-boot#136 is actually causing enlisting issues with databases like MariaDB (snowdrop/narayana-spring-boot#140).

It seems that the SHA-224 hash is indeed 28 bytes long, but the UTF-8 String created from it is quite bigger, somewhere between 40 and 60 bytes. For MariaDB this is causing too large XID Strings created by Narayana while starting XA resource during enlisting. In my opinion this behaviour should also affect Quarkus (not verified!)

Small Groovy script to proove length of shortend node name to exceed 28 byte barrier

def nodeIdentifier = UUID.randomUUID().toString()
def nodeIdentifierAsBytes = nodeIdentifier.getBytes()
def messageDigest224 = java.security.MessageDigest.getInstance('SHA-224')
def digest = messageDigest224.digest(nodeIdentifierAsBytes)
println digest.length
def shortenNodeIdentifier = new String(digest, java.nio.charset.StandardCharsets.UTF_8)
println shortenNodeIdentifier
println shortenNodeIdentifier.getBytes(java.nio.charset.StandardCharsets.UTF_8).length

Expected behavior

Creation of a valid shortend node name

Actual behavior

Broken node name as described

How to Reproduce?

Enlist XAResource of MariaDB connection into Narayana JTA transaction.

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @mmusgrov (narayana)

[graben]

Interesting fact:
com.arjuna.ats.arjuna.common.CoreEnvironmentBean allows node name <= 64 bytes
com.arjuna.ats.arjuna.coordinatorTxControl allows node name <= 28 bytes
Btw. TxControl initial get node name from CoreEnvironmentBean. Mismatch in Narayana?
Maybe @mmusgrov can explain the oddness.

[mmusgrov]

SHA-224 hash is indeed 28 bytes long, but the UTF-8 String created from it is quite bigger,

Interesting but it should easy to fix, perhaps by using a different encoding or otherwise.

Interesting fact:
com.arjuna.ats.arjuna.common.CoreEnvironmentBean allows node name <= 64 bytes

ArjunaCore is a toolkit for implementing various transaction protocols but some integrations add further constraints on the length, the most restrictive of which is kubernetes which further restricts it to 23 characters because of a uniqueness restriction caused by the use of stateful sets.

com.arjuna.ats.arjuna.coordinatorTxControl allows node name <= 28 bytes

This is the standard length and is sufficient for working within the constraints of the Xid data structure, since we embed the nodeIdentifier in the Xid bytes so that the recovery manager can determine whether or not it is responsible for recovering it - the only platform we've come across where this length can't be used is kubernetes.

Btw. TxControl initial get node name from CoreEnvironmentBean. Mismatch in Narayana?

I don't understand this statement.

[graben]

I don't understand this statement.

Look how it gets initialized in TxControl without further proof!

[mmusgrov]

I don't understand this statement.

Look how it gets initialized in TxControl without further proof!

My point was that I didn't understand what you wrote because I was unable to parse the grammar.

[graben]

But you got my point now?

[mmusgrov]

I don't recall you being this uncooperative and obstinate in our previous interactions so let's avoid conversation and I will just use your bug description, thanks for the initial report.

[marcosgopen]

The problem here is when the original array of bytes are not UTF-8 encoded. I wonder if using an array of byte instead of a String for the nodeName wouldn't be better here.
Alternatively we can use an encoding like Base64 to preserve the original array of bytes also after the conversion to String.

[mmusgrov]

Why not avoid UTF-8 characters and just change the charset to one that will give you back the 28 or less bytes. The standard one would be ISO-LATIN-1 (java.nio.charset.StandardCharsets.ISO_8859_1).

[mmusgrov]

Note that the node name no longer needs to be readable by a human (which historically has been useful for debugging) since it gets hashed.

[mmusgrov]

This works because:

• we know that the original digest was 28 bytes or less, and
• ISO/IEC 8859-1 is an "8-bit single-byte coded graphic character set",
• whereas UTF-8 encodes "Unicode code points using one to four one-byte (8-bit) code units" (ie it is a "variable-length character encoding").

[marcosgopen]

@mmusgrov even if the byte array is encoded in ISO/IEC 8859-1 I don't think that the resulting SHA-224 will be a ISO/IEC 8859-1 byte array. I picked Base64 but ISO/IEC 8859-1 would be ok too.

[mmusgrov]

@marcosgopen The bug report says that the UTF bytes for the SHA-224 hash produced by narayana-jta extension is longer than the 28 byte limit. But the bug report is trying to read the bytes back using UTF_8 even though UTF-8 is a variable-length character encoding, but if Benjamin uses ISO/IEC 8859-1 to get the bytes it will produce a byte array of the correct length. I ran a java version of Benjamin's Groovy test and if you run it too then you will find that the digest is 28 bytes, the UTF-8 bytes is about double that, depending on the input string, and the ISO/IEC 8859-1 bytes is less that or equal to 28, as we expect:

    public static void main(String[] args) throws Exception{
        System.out.println(Charset.defaultCharset().displayName());

        testNodeName(UUID.randomUUID().toString());
        testNodeName(UUID.randomUUID().toString());
    }

    private static void testNodeName(String nodeIdentifier) throws NoSuchAlgorithmException {
        byte[] nodeIdentifierAsBytes = nodeIdentifier.getBytes();
        MessageDigest messageDigest224 = java.security.MessageDigest.getInstance("SHA-224");
        byte[] digest = messageDigest224.digest(nodeIdentifierAsBytes);
        String shortenNodeIdentifier = new String(digest, java.nio.charset.StandardCharsets.UTF_8);
        byte[] shortenNodeIdentifierBytes = shortenNodeIdentifier.getBytes(java.nio.charset.StandardCharsets.UTF_8);
        byte[] iso = shortenNodeIdentifier.getBytes(ISO_8859_1);

        System.out.printf("%d vs %d vs %d%n", digest.length, shortenNodeIdentifierBytes.length, iso.length);
    }

[mmusgrov]

I declined #40613 because there is no bug here and the issue should be closed as not a bug.

[marcosgopen]

Thank you Mike for your clarification. I assume then that we need to change the way the TxControl reads the bytes here: https://github.com/jbosstm/narayana/blob/main/ArjunaCore/arjuna/classes/com/arjuna/ats/arjuna/coordinator/TxControl.java#L147 (invoked at https://github.com/quarkusio/quarkus/blob/main/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/NarayanaJtaRecorder.java#L51)
IIUC this needs to be fixed and I can open a PR for that.

[mmusgrov]

Yes that's correct, we'll need to use ISO_8859_1 as well, when you create the PR will you include a test.

[mmusgrov]

@marcosgopen We'll need to release in a major because it will break recovery unless we add a config flag since the nodeIdentifier will change and we won't be able to detect orphans from previous runs.

[mmusgrov]

Perhaps it would also help to use the ISO_8859_1 charset when quarkus shortens the node name.

So I think that the following change to line 62 should do it:

transactions.nodeName = new String(messageDigest224.digest(nodeNameAsBytes), StandardCharsets.ISO_8859_1);

[marcosgopen]

Perhaps it would also help to use the ISO_8859_1 charset when quarkus shortens the node name.

So I think that the following change to line 62 should do it:

transactions.nodeName = new String(messageDigest224.digest(nodeNameAsBytes), StandardCharsets.ISO_8859_1);

@mmusgrov I tested your solution but unfortunately it doesn't work because when bytes are encoded in a different Charset the length can increase. Which this is the case.
I think there is a misunderstanding in the sentence 'ISO/IEC 8859-1 is an "8-bit single-byte coded graphic character set"': a ISO/IEC 8859-1 encoded string is a '8-bit single-byte coded graphic character set', but our 'nodeNameAsBytes' it is not. Additionally encoding the string into ISO/IEC 8859-1 it might increase its length.
In fact In my closed PR that is what I was doing. Transform it into a Base64 encoded array of byte and then shorten it into a 28 bytes.

[mmusgrov]

nodeNameAsBytes is the original string which doesn't fit within the 28 byte limit. The encoding used for the original string is irrelavant because after applying the SHA-224 algorithm to it the resulting digest, which is 28 bytes in length, gets re-encoded using ISO/IEC 8859-1 which encodes the characters using fixed width chars so will still be 28 bytes, unlike UTF-8 which encodes using a width of between one to four bytes.

Here is the test to demonstrate that changing quarkus to use ISO_8859_1 will work as expected:

    private static void testNodeName(String nodeIdentifier) throws NoSuchAlgorithmException {
        byte[] nodeIdentifierAsBytes = nodeIdentifier.getBytes();
        MessageDigest messageDigest224 = java.security.MessageDigest.getInstance("SHA-224");
        byte[] digest = messageDigest224.digest(nodeIdentifierAsBytes);
        String shortenNodeIdentifier = new String(digest, ISO_8859_1);
        byte[] shortenNodeIdentifierBytes = shortenNodeIdentifier.getBytes(UTF_8);
        byte[] iso = shortenNodeIdentifier.getBytes(ISO_8859_1);

        System.out.printf("len=%d%n", shortenNodeIdentifier.length());
    }

[marcosgopen]

    private static void testNodeName(String nodeIdentifier) throws NoSuchAlgorithmException {
        byte[] nodeIdentifierAsBytes = nodeIdentifier.getBytes();
        MessageDigest messageDigest224 = java.security.MessageDigest.getInstance("SHA-224");
        byte[] digest = messageDigest224.digest(nodeIdentifierAsBytes);
        String shortenNodeIdentifier = new String(digest, ISO_8859_1);
        byte[] shortenNodeIdentifierBytes = shortenNodeIdentifier.getBytes(UTF_8);
        byte[] iso = shortenNodeIdentifier.getBytes(ISO_8859_1);

        System.out.printf("len=%d%n", shortenNodeIdentifier.length());
    }

Sorry @mmusgrov , I think my previous comment was not so clear.
You are right and in you snippet the final String length is 28 when encoded with ISO_8859_1.
But as mentioned in my previous comment in the TxControl we execute byte[] bytes = name.getBytes(StandardCharsets.UTF_8);.
When a String encoded with ISO_8859_1 of length 28 is processed as theString.getBytes(StandardCharsets.UTF_8) the final length can be > 28.
I tested it in my branch: https://github.com/marcosgopen/quarkus/tree/node-name

So if we want to use your snippet we would need to change the TxControl and make a release.

[mmusgrov]

Ah I see, my original code snippet showed that problem too, I don't why I didn't spot it. So it looks like the only way around it is to change Narayana to read it using ISO_8859_1. But that would need to go into a major release since it could break orphan detection.

[mmusgrov]

And we'd need to provide users with a migration path.

[marcosgopen]

@mmusgrov what about truncating the string instead as suggested in my PR?

[mmusgrov]

That will impact uniqueness. We'll just have to "bite the bullet" and provide a migration path for our users.

[mmusgrov]

@turing85 Can you comment on the risks of taking Marco's PR?

[mmusgrov]

The current hash using SHA-224 already presents potential duplicates and rehashing it a second time increases that risk.

[turing85]

The current hash using SHA-224 already presents potential duplicates and rehashing it a second time increases that risk.

Every hash has a potential to generate duplicates.

I did some calculation on the original issue. The probability for a SHA224 to collide was astronomical small. We can afford to loose some order of magnitudes. Haven't done the calculation for base64 encoding, though.

[mmusgrov]

Okay thanks, note that as well as rehashing a second time, Marco's PR is truncating the hash which further increases the risk of duplicates.

[turing85]

Even if we effectively half the entropy we have, we still need something at the order of 10^16 tries to have a chance of 1% to get a collision. As reference: there are approximately 10^18 grains of sand on this planet.

[mmusgrov]

So @graben I assume that your original cryptic responses to my questions on this issue were referring to how Narayana works rather than how quarkus works and if so we all could have avoided a lot of wasted effort if you'd raised the issue against Narayana in the first place.

[graben]

@mmusgrov : sorry for the confusion. I was not sure whether the solution in Quarkus is buggy or Narayana needs improvement. Later one looked okay from my first point of view. :-)

[mmusgrov]

Ok @marcosgopen it looks like we'll be needing your PR after all.
@turing85 would it be possible for you to review Marco's PR?

[mmusgrov]

@mmusgrov : sorry for the confusion. I was not sure whether the solution in Quarkus is buggy or Narayana needs improvement. Later one looked okay from my first point of view. :-)

ok. And thanks again for bringing the issue to our attention.

[turing85]

Ok @marcosgopen it looks like we'll be needing your PR after all. @turing85 would it be possible for you to review Marco's PR?

Done. Added a single remark.

[mmusgrov]

@marcosgopen So let's still go with your PR, but in the meantime I will raise a narayana issue to allow callers to specify the character set: void setNodeIdentifier(String nodeIdentifier, Charset charset);.

[Edit] I raised issue JBTM-3883 to allow the charset to be specified when setting the narayana nodeIdentifier config property.

[mmusgrov]

@geoand Would it be possible for someone to reopen Marco's PR?

[geoand]

I did, but we'll need a better title for it as for a Quarkus maintainer pov (and also a user looking at change logs), the title does not tell me anything :)

[mmusgrov]

@marcosgopen Can you rename the title to something like "Ensure the Transaction Manager node name is less than or equal to 28 bytes", it's up to you whether or not to update the description to indicate why the limit exists (because we need to include it in the fixed size Xid data structure so that the TM can determine which XA resource branches it is responsible for).

[marcosgopen]

@marcosgopen Can you rename the title to something like "Ensure the Transaction Manager node name is less than or equal to 28 bytes", it's up to you whether or not to update the description to indicate why the limit exists (because we need to include it in the fixed size Xid data structure so that the TM can determine which XA resource branches it is responsible for).

I can see the PR is open now. @mmusgrov I updated the title and the description.