From c5683e61b96e555c6cbbbf870a17d9e1b1ab3962 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Thu, 4 Jul 2019 10:08:17 -0400 Subject: [PATCH 1/3] Revert "Make MACs thread-local to avoid repeated reinstantiation/garbage collection." This reverts commit 5e625e7b3f06d4f5252b794c6c341bb25a14a0a6. --- .../otp/HmacOneTimePasswordGenerator.java | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java b/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java index 0cf32a0..e3147b1 100644 --- a/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java +++ b/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java @@ -42,8 +42,6 @@ public class HmacOneTimePasswordGenerator { private final int modDivisor; - private final ThreadLocal macThreadLocal; - /** * The default length, in decimal digits, for one-time passwords. */ @@ -120,16 +118,6 @@ protected HmacOneTimePasswordGenerator(final int passwordLength, final String al // Our purpose here is just to throw an exception immediately if the algorithm is bogus. Mac.getInstance(algorithm); this.algorithm = algorithm; - - this.macThreadLocal = ThreadLocal.withInitial(() -> { - try { - return Mac.getInstance(algorithm); - } catch (final NoSuchAlgorithmException e) { - // This should never happen; we just checked to make sure we could instantiate a MAC with this - // algorithm, and if we made it this far, we know it worked. - throw new RuntimeException(e); - } - }); } /** @@ -144,8 +132,15 @@ protected HmacOneTimePasswordGenerator(final int passwordLength, final String al * @throws InvalidKeyException if the given key is inappropriate for initializing the {@link Mac} for this generator */ public int generateOneTimePassword(final Key key, final long counter) throws InvalidKeyException { - final Mac mac = this.macThreadLocal.get(); - mac.init(key); + final Mac mac; + + try { + mac = Mac.getInstance(this.algorithm); + mac.init(key); + } catch (final NoSuchAlgorithmException e) { + // This should never happen since we verify that the algorithm is legit in the constructor. + throw new RuntimeException(e); + } final ByteBuffer buffer = ByteBuffer.allocate(8); buffer.putLong(0, counter); From fc1926448c8ec5993396221e4ef7bfd8b08eb7a3 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Fri, 21 Jun 2019 20:59:03 -0400 Subject: [PATCH 2/3] Store a local Mac instead of regenerating one every time. --- ...HmacOneTimePasswordGeneratorBenchmark.java | 2 +- .../otp/HmacOneTimePasswordGenerator.java | 27 ++++++------------- .../TimeBasedOneTimePasswordGenerator.java | 3 +-- 3 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/benchmark/java/com/eatthepath/otp/HmacOneTimePasswordGeneratorBenchmark.java b/src/benchmark/java/com/eatthepath/otp/HmacOneTimePasswordGeneratorBenchmark.java index 1d46ed5..676a7e9 100644 --- a/src/benchmark/java/com/eatthepath/otp/HmacOneTimePasswordGeneratorBenchmark.java +++ b/src/benchmark/java/com/eatthepath/otp/HmacOneTimePasswordGeneratorBenchmark.java @@ -10,7 +10,7 @@ import java.security.Key; import java.security.NoSuchAlgorithmException; -@State(Scope.Thread) +@State(Scope.Benchmark) public class HmacOneTimePasswordGeneratorBenchmark { private HmacOneTimePasswordGenerator hotp; diff --git a/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java b/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java index e3147b1..ef75f5f 100644 --- a/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java +++ b/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java @@ -31,13 +31,12 @@ *

Generates HMAC-based one-time passwords (HOTP) as specified in * RFC 4226.

* - *

{@code HmacOneTimePasswordGenerator} instances are thread-safe and may be shared and re-used across multiple - * threads.

+ *

{@code HmacOneTimePasswordGenerator} instances are thread-safe and may be shared between threads.

* * @author Jon Chambers */ public class HmacOneTimePasswordGenerator { - private final String algorithm; + private final Mac mac; private final int passwordLength; private final int modDivisor; @@ -92,6 +91,8 @@ public HmacOneTimePasswordGenerator(final int passwordLength) throws NoSuchAlgor * @throws NoSuchAlgorithmException if the given algorithm is not supported by the underlying JRE */ protected HmacOneTimePasswordGenerator(final int passwordLength, final String algorithm) throws NoSuchAlgorithmException { + this.mac = Mac.getInstance(algorithm); + switch (passwordLength) { case 6: { this.modDivisor = 1_000_000; @@ -114,10 +115,6 @@ protected HmacOneTimePasswordGenerator(final int passwordLength, final String al } this.passwordLength = passwordLength; - - // Our purpose here is just to throw an exception immediately if the algorithm is bogus. - Mac.getInstance(algorithm); - this.algorithm = algorithm; } /** @@ -131,21 +128,13 @@ protected HmacOneTimePasswordGenerator(final int passwordLength, final String al * * @throws InvalidKeyException if the given key is inappropriate for initializing the {@link Mac} for this generator */ - public int generateOneTimePassword(final Key key, final long counter) throws InvalidKeyException { - final Mac mac; - - try { - mac = Mac.getInstance(this.algorithm); - mac.init(key); - } catch (final NoSuchAlgorithmException e) { - // This should never happen since we verify that the algorithm is legit in the constructor. - throw new RuntimeException(e); - } + public synchronized int generateOneTimePassword(final Key key, final long counter) throws InvalidKeyException { + this.mac.init(key); final ByteBuffer buffer = ByteBuffer.allocate(8); buffer.putLong(0, counter); - final byte[] hmac = mac.doFinal(buffer.array()); + final byte[] hmac = this.mac.doFinal(buffer.array()); final int offset = hmac[hmac.length - 1] & 0x0f; for (int i = 0; i < 4; i++) { @@ -174,6 +163,6 @@ public int getPasswordLength() { * @return the name of the HMAC algorithm used by this generator */ public String getAlgorithm() { - return this.algorithm; + return this.mac.getAlgorithm(); } } diff --git a/src/main/java/com/eatthepath/otp/TimeBasedOneTimePasswordGenerator.java b/src/main/java/com/eatthepath/otp/TimeBasedOneTimePasswordGenerator.java index 57ed5d0..21df927 100644 --- a/src/main/java/com/eatthepath/otp/TimeBasedOneTimePasswordGenerator.java +++ b/src/main/java/com/eatthepath/otp/TimeBasedOneTimePasswordGenerator.java @@ -31,8 +31,7 @@ *

Generates time-based one-time passwords (TOTP) as specified in * RFC 6238.

* - *

{@code TimeBasedOneTimePasswordGenerator} instances are thread-safe and may be shared and re-used across multiple - * threads.

+ *

{@code TimeBasedOneTimePasswordGenerator} instances are thread-safe and may be shared between threads.

* * @author Jon Chambers */ From 27880c7dbe4917a3528032f9b730aeb79f4d1049 Mon Sep 17 00:00:00 2001 From: Jon Chambers Date: Fri, 21 Jun 2019 21:15:34 -0400 Subject: [PATCH 3/3] Reuse a local byte array instead of repeatedly instantiating new ones. --- .../otp/HmacOneTimePasswordGenerator.java | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java b/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java index ef75f5f..c9db579 100644 --- a/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java +++ b/src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java @@ -20,13 +20,12 @@ package com.eatthepath.otp; -import java.nio.ByteBuffer; +import javax.crypto.Mac; +import javax.crypto.ShortBufferException; import java.security.InvalidKeyException; import java.security.Key; import java.security.NoSuchAlgorithmException; -import javax.crypto.Mac; - /** *

Generates HMAC-based one-time passwords (HOTP) as specified in * RFC 4226.

@@ -39,6 +38,7 @@ public class HmacOneTimePasswordGenerator { private final Mac mac; private final int passwordLength; + private final byte[] buffer; private final int modDivisor; /** @@ -115,6 +115,9 @@ protected HmacOneTimePasswordGenerator(final int passwordLength, final String al } this.passwordLength = passwordLength; + + // We need at least 8 bytes to store the 64-bit counter value + this.buffer = new byte[Math.max(8, this.mac.getMacLength())]; } /** @@ -131,21 +134,32 @@ protected HmacOneTimePasswordGenerator(final int passwordLength, final String al public synchronized int generateOneTimePassword(final Key key, final long counter) throws InvalidKeyException { this.mac.init(key); - final ByteBuffer buffer = ByteBuffer.allocate(8); - buffer.putLong(0, counter); - - final byte[] hmac = this.mac.doFinal(buffer.array()); - final int offset = hmac[hmac.length - 1] & 0x0f; - - for (int i = 0; i < 4; i++) { - // Note that we're re-using the first four bytes of the buffer here; we just ignore the latter four from - // here on out. - buffer.put(i, hmac[i + offset]); + this.buffer[0] = (byte) ((counter & 0xff00000000000000L) >>> 56); + this.buffer[1] = (byte) ((counter & 0x00ff000000000000L) >>> 48); + this.buffer[2] = (byte) ((counter & 0x0000ff0000000000L) >>> 40); + this.buffer[3] = (byte) ((counter & 0x000000ff00000000L) >>> 32); + this.buffer[4] = (byte) ((counter & 0x00000000ff000000L) >>> 24); + this.buffer[5] = (byte) ((counter & 0x0000000000ff0000L) >>> 16); + this.buffer[6] = (byte) ((counter & 0x000000000000ff00L) >>> 8); + this.buffer[7] = (byte) (counter & 0x00000000000000ffL); + + this.mac.update(this.buffer, 0, 8); + + try { + this.mac.doFinal(this.buffer, 0); + } catch (final ShortBufferException e) { + // We allocated the buffer to (at least) match the size of the MAC length at construction time, so this + // should never happen. + throw new RuntimeException(e); } - final int hotp = buffer.getInt(0) & 0x7fffffff; + final int offset = this.buffer[this.mac.getMacLength() - 1] & 0x0f; - return hotp % this.modDivisor; + return ((this.buffer[offset] & 0x7f) << 24 | + (this.buffer[offset + 1] & 0xff) << 16 | + (this.buffer[offset + 2] & 0xff) << 8 | + (this.buffer[offset + 3] & 0xff)) % + this.modDivisor; } /**