Please review this md5 implementation, this fall under Cryptography.InsecureAlgorithm

[asimmanzoor]

logback-gelf/src/main/java/de/siegmar/logbackgelf/MessageIdSupplier.java

Line 67 in 0e7182d

return MessageDigest.getInstance("MD5").digest(data);

[osiegmar]

This is not used for any sort of cryptography. This method aims to generate a (more or less) unique Message ID for datagram chunking when using the UDP transport. Unfortunately the GELF protocol limits the Message ID field to 8 byte – this is why using UUID (16-byte) isn't possible.

Having said that, I'm open for specific suggestions to improve this.

[asimmanzoor]

Thanks for quick response, I agree that due to limitation of Message ID at GELF format we can't use UUID, However my concern with putting MD5 as Algo, which raise a concern with various security tools.
E.g.
https://help.hcltechsw.com/appscan/Source/9.0.3/topics/intro_products.html

As an alternative, we can use SHA-256 , Apparently will have same 8 byte that is supported by GELF. It's supported by MessageDigest as MessageDigest.getInstance("SHA-256").digest(data) and it will not be concern by security tools for now.

Looking forward to hear your response.

[osiegmar]

My concern with SHA-256 is, that I'm already cutting half of the MD5-hash to fit into the Message ID field. With SHA-256 I'd have to cut 8 out of 32 byte and fear that collisions might occur more likely. Furthermore, at some point in the future SHA-256 probably will have the same fate as MD5.

Probably its better to implement something that creates a message id based a timestamp (System.currentTimeMillis() returns 4 byte) and an also 4-byte host specific or random value.

[asimmanzoor]

I think, you are right, We don't know how long SHA-256 last. Keeping actual use of messgeId in mind, I am agree with you.

[osiegmar]

One correction to my post earlier: The timestamp (regardless of System.currentTimeMillis() or System.nanoTime()) already consumes 8 byte (long).

I re-implemented the MessageIdSupplier to create Message-IDs based on a random host value (integer) and concatenating that with a timestamp that is cased to an integer. That should do the job pretty fine.

Although this breaks the API and a new major release is needed for that.

Feedback is very welcome!

[asimmanzoor]

Look good to me.
I would like to appreciate you for quick addressing the issue.