ARTEMIS-5902 optimize authn cache key creation#6258
ARTEMIS-5902 optimize authn cache key creation#6258jbertram wants to merge 1 commit intoapache:mainfrom
Conversation
Eliminate unnecessary calls to j.s.MessageDigest#getInstance(String) as well as unnecessary bytes in hash. Existing tests should suffice for validation.
| return ByteUtil.bytesToHex(MessageDigest.getInstance("SHA-256").digest((username + password + CertificateUtil.getCertSubjectDN(connection)).getBytes(StandardCharsets.UTF_8))); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| return (MessageDigest) SHA256.clone(); | ||
| } catch (CloneNotSupportedException e) { |
There was a problem hiding this comment.
Instead of throwing here would it not make sense to try and get an instance via something like return MessageDigest.getInstance(digest.getAlgorithm()); and only throw if that fails?
There was a problem hiding this comment.
I see your point, and that's exactly what org.apache.activemq.artemis.protocol.amqp.sasl.scram.ScramServerFunctionalityImpl#getDigest does. I think that's fair, but this optimization is the whole point of this change so I think I'll add some code to log once if this fails so we know the optimization isn't working. I'll also make this method protected and add a test to ensure it works as expected.
There was a problem hiding this comment.
Logging is a good addition. Given users want it to work and it can regardless of the clone support falling back and maybe losing some performance is acceptable vs not working at all.
2 participants
Eliminate unnecessary calls to j.s.MessageDigest#getInstance(String) as well as unnecessary bytes in hash.
Existing tests should suffice for validation.