AES-CTR with re-seeding (#21740) #22013

Merged
merged 1 commit into from Jan 11, 2017

Projects

None yet

5 participants

@gosubpl
Contributor
gosubpl commented Dec 15, 2016

Introduces an AES-CTR re-seeding PRNG to replace the one from maths.uncommon.

@gosubpl
Contributor
gosubpl commented Dec 15, 2016

Refs #21740

@akka-ci
Member
akka-ci commented Dec 15, 2016

Test PASSed.

@akka-ci akka-ci added tested and removed validating labels Dec 15, 2016
@akka-ci
Member
akka-ci commented Dec 15, 2016

Test PASSed.

+ */
+class AES256CounterBuiltinRNG extends java.security.SecureRandomSpi {
+ /**Singleton instance. */
+ private final val SOURCE: SecureRandom = new SecureRandom
@patriknw
patriknw Jan 3, 2017 Member

all caps is not scala style, and the comment about singleton instance might be confusing

@gosubpl
gosubpl Jan 3, 2017 Contributor

fixed

+ override protected def engineNextBytes(bytes: Array[Byte]): Unit = rng.nextBytes(bytes)
+
+ /**
+ * Unused method
@patriknw
patriknw Jan 3, 2017 Member

"Unused method" ?

@gosubpl
gosubpl Jan 3, 2017 Contributor

this was a direct copy of uncommons.math file, used for performance comparisons, is removed now, as it is not needed in akka

@gosubpl
gosubpl Jan 3, 2017 Contributor

however, please note that I have renamed the new proposed algorithm from AESCounterBuiltinCTRRNG to AESCounterBuiltinRNG (which is a good name in itself), so the file itself will stay, albeit with different content

@gosubpl
gosubpl Jan 4, 2017 Contributor

please forgive the confusion

+ * NOTE: THIS CLASS IS NOT SERIALIZABLE
+ */
+
+class AESCounterBuiltinCTRRNG(val seed: Array[Byte]) extends Random {
@patriknw
patriknw Jan 3, 2017 Member

private[akka] if it's internal api

@gosubpl
gosubpl Jan 3, 2017 Contributor

fixed

+ private val COUNTER_SIZE_BYTES = 16
+ private val BITWISE_BYTE_TO_INT = 0x000000FF
+ private val RESEEDING_THRESHOLD = 1000000 // million
+ private val RESEEDING_DEADLINE = 140737488355328L // 2^47 safe as per SP800-90
@patriknw
patriknw Jan 3, 2017 Member

perhaps define those in the companion object instead?

@gosubpl
gosubpl Jan 3, 2017 Contributor

fixed

+ @Override
+ override protected def next(bits: Int): Int = {
+ try {
+ lock.lock()
@patriknw
patriknw Jan 3, 2017 Member

perhaps use synchronized instead of ReentrantLock?

@gosubpl
gosubpl Jan 3, 2017 Contributor

working on this - I need to run benchmarks to see what are the trade-offs

+ index += 4
+ result >>> (32 - bits)
+ } finally {
+ import scala.concurrent.ExecutionContext.Implicits.global
@patriknw
patriknw Jan 3, 2017 Member

no, we should not use that execution context, can we pass in an ExecutionContext in the constructor? We should add a blocking-dispatcher, which could be used by this also. #21170 (comment)

@gosubpl
gosubpl Jan 3, 2017 Contributor

working on this

+ if (bitsSinceSeeding > RESEEDING_THRESHOLD) {
+ if (reseedFuture == null) {
+// println(s"reseeding started! bits: $bitsSinceSeeding")
+ reseedFuture = Future { SOURCE.generateSeed(32) }
@patriknw
patriknw Jan 3, 2017 Member

I don't fully understand the intention here. current thread is anyway blocked by the below Await so I don't see the purpose of delegate the generateSeed to another thread. Other threads are blocked on the lock until this is completed.

@gosubpl
gosubpl Jan 3, 2017 Contributor

Hmmm... will add some clarifying comments - generally the flow looks like this:

  1. if the RESEEDING_THRESHOLD is exceeded (new entropy is needed), create a Future running the new seed generator
  2. cyclically check whether this future hasn't completed (in a non-blocking way). If it has - retrieve the value.
  3. check whether by any chance RESEEDING_DEADLINE was not exceeded - if it has, we must re-seed - in that case, activate Await which would block
+// println(s"reseeding started! bits: $bitsSinceSeeding")
+ reseedFuture = Future { SOURCE.generateSeed(32) }
+ }
+ if (bitsSinceSeeding > RESEEDING_DEADLINE) {
@patriknw
patriknw Jan 3, 2017 Member

this condition was already checked above

@gosubpl
gosubpl Jan 3, 2017 Contributor

not exactly, we have checked in line 81 if the bitsSinceSeeding exceeded RESEEDING_THRESHOLD which should trigger requesting for new entropy. As new entropy request would block, we run this on separate thread (ekhm, Future) thus avoiding blocking in the main thread (agreed, this should be on io dispatcher, not the default one).
However, if (what is very unlikely) we don't get the entropy back and we have reached another, higher limit of RESEEDING_DEADLINE, then we need to block (hence Await) and wait for new entropy to arrive, as proceeding would be unsafe

+ }
+ if (bitsSinceSeeding > RESEEDING_DEADLINE) {
+// println(s"blocking for the seeding result bits: $bitsSinceSeeding")
+ Await.ready(reseedFuture, Duration.Inf)
@patriknw
patriknw Jan 3, 2017 Member

is it safe to use Duration.Inf? it will always be completed?

@gosubpl
gosubpl Jan 3, 2017 Contributor

The thing inside the future is basically just a read from /dev/random, so with high probability should succeed, and if it doesn't, then what guarantee we have that subsequent call that will attempt the same read will succeed? In any way, at this stage we cannot proceed without the additional entropy. But for added security we could add some re-try logic in case this fails.

@gosubpl
gosubpl Jan 3, 2017 Contributor

The thing inside the future is basically just a read from /dev/random (or /dev/urandom on BSD or system call on Windows). If it does not return in reasonable timeframe, then probably something is very wrong. But I could imagine a retry logic backed up by an exception after a couple of failed attempts, WDYT?

+ Await.ready(reseedFuture, Duration.Inf)
+ }
+ if (reseedFuture != null && reseedFuture.isCompleted) {
+ val newSeed = Await.result(reseedFuture, Duration.Inf) // should be immediate, as the future is already completed
@patriknw
patriknw Jan 3, 2017 Member

then grab the value of the Future instead of using Await

@gosubpl
gosubpl Jan 3, 2017 Contributor

fixed

+ bitsSinceSeeding = 0
+ }
+ }
+ lock.unlock()
@patriknw
patriknw Jan 3, 2017 Member

lot's of code above that might throw and thereby not releasing the lock, might need another try-finally

@gosubpl
gosubpl Jan 3, 2017 Contributor

yes, agreed; will refactor if synchronized is not a solution

@akka-ci akka-ci added validating and removed tested labels Jan 4, 2017
+ index += 4
+ result >>> (32 - bits)
+ } finally {
+ import scala.concurrent.ExecutionContext.Implicits.global
@gosubpl
gosubpl Jan 4, 2017 Contributor

working on introducing separate execution context

+ @Override
+ override protected def next(bits: Int): Int = {
+ try {
+ lock.lock()
@gosubpl
gosubpl Jan 4, 2017 Contributor

analyzing whether not replace with synchronized block

+ }
+}
+
+private object CounterRNGConstants {
@gosubpl
gosubpl Jan 4, 2017 Contributor

not sure if this is a good name

+ }
+ if (bitsSinceSeeding > RESEEDING_DEADLINE) {
+ // println(s"blocking for the seeding result bits: $bitsSinceSeeding")
+ Await.ready(reseedFuture, Duration.Inf)
@gosubpl
gosubpl Jan 4, 2017 Contributor

maybe requires some logic for re-try and final fail with exception, but basically what's inside the Future is just a read from /dev/random so with high probability will not fail, WDYT?

@patriknw
patriknw Jan 4, 2017 Member

I'm always scared about blocking forever on something because there is no sign of why the application is halting. Yeah, it should always return within reasonable time, but I'd rather have a long timeout (e.g. 5 minutes) with logging in case of failure than trusting an external "service".

Retrying might be overkill.

+ reseedFuture = null // request creation of new seed
+ }
+ }
+ lock.unlock()
@gosubpl
gosubpl Jan 4, 2017 edited Contributor

if lock is kept (and not replaced with synchronized), another layer of try-finally will be added

@gosubpl
Contributor
gosubpl commented Jan 4, 2017 edited

@patriknw many thanks for the insightful comments. Please click "View Changes" to get all my replies as I have already pushed some modifications. I am still working on improving this code (especially on dispatcher and lock), will ping you when it's ready for another review.
In the meantime would be great if @drewhk could chime in on the re-seeding logic, especially on what could be a good value for RESEEDING_THRESHOLD (which exceeding initiates re-seeding in a non-blocking way as opposed to exceeding RESEEDING_DEADLINE that results in blocking)

@akka-ci akka-ci added tested and removed validating labels Jan 4, 2017
@akka-ci
Member
akka-ci commented Jan 4, 2017

Test PASSed.

+ // println(s"reseeding started! bits: $bitsSinceSeeding")
+ reseedFuture = Future { entropySource.generateSeed(32) }
+ }
+ if (bitsSinceSeeding > RESEEDING_DEADLINE) {
@patriknw
patriknw Jan 4, 2017 Member

ah, in my previous review I misread and though it was the same constant

+ }
+ if (bitsSinceSeeding > RESEEDING_DEADLINE) {
+ // println(s"blocking for the seeding result bits: $bitsSinceSeeding")
+ Await.ready(reseedFuture, Duration.Inf)
@patriknw
patriknw Jan 4, 2017 Member

I'm always scared about blocking forever on something because there is no sign of why the application is halting. Yeah, it should always return within reasonable time, but I'd rather have a long timeout (e.g. 5 minutes) with logging in case of failure than trusting an external "service".

Retrying might be overkill.

@gosubpl gosubpl changed the title from [DO NOT MERGE (yet) - for discussion] AES-CTR with re-seeding (#21740) to AES-CTR with re-seeding (#21740) Jan 5, 2017
@gosubpl gosubpl changed the title from AES-CTR with re-seeding (#21740) to [wip] AES-CTR with re-seeding (#21740) Jan 5, 2017
*/
class AES128CounterSecureRNG extends java.security.SecureRandomSpi {
- /**Singleton instance. */
- private final val Instance: SecureRandomSeedGenerator = new SecureRandomSeedGenerator
+ private val singleThreadPool = ExecutionContext.fromExecutorService(Executors.newSingleThreadExecutor())
@gosubpl
gosubpl Jan 5, 2017 Contributor

ThreadPoolExecutor with 1 thread. This is enough as if the future does not complete, the generator will block and will not submit another future.

@ktoso
ktoso Jan 10, 2017 edited Member

Pool is OK, but perhaps give the thread a nice name so people know what it is? You'll need to use a ThreadFactory.

@gosubpl
gosubpl Jan 11, 2017 Contributor

Done

- * Unused method
- * Returns the given number of seed bytes. This call may be used to
- * seed other random number generators.
+ * For completeness of SecureRandomSpi API implementation
@gosubpl
gosubpl Jan 5, 2017 Contributor

This method must be implemented to comply with SecureRandomSpi API

@akka-ci akka-ci added validating and removed tested labels Jan 5, 2017
+ cipher.init(Cipher.ENCRYPT_MODE, new this.AESKey(seed), ivSpec)
+
+ @Override
+ override protected def next(bits: Int): Int = synchronized {
@gosubpl
gosubpl Jan 5, 2017 Contributor

Synchronized is indeed faster (benchmarked) for uncontended access (which is the case here). Plus the control flow is easier (no need for unlocking in finally)

+ Await.ready(reseedFuture, reseedingTimeout)
+ } catch {
+ case ex: Exception
+ Console.err.println(s"[ERROR] AESCounterBuiltinRNG re-seeding failed or timed out after ${reseedingTimeout.toSeconds.toString}s !")
@gosubpl
gosubpl Jan 5, 2017 Contributor

This is the only option of logging - we don't have access to ActorSystem here

+ // we need to block on the future to wait for entropy
+ if (bitsSinceSeeding > reseedingDeadline) {
+ try {
+ Await.ready(reseedFuture, reseedingTimeout)
@gosubpl
gosubpl Jan 5, 2017 Contributor

Default time-out is 5 minutes. Something would need to be seriously wrong to exceed

+
+import scala.concurrent.ExecutionContext
+
+class RandomReplacementSpec extends AkkaSpec {
@gosubpl
gosubpl Jan 5, 2017 edited Contributor

A spec to test whether the new generator generates the same sequence as the original one. This spec can be removed together with the original generator and whole dependency on org.uncommons.math

@ktoso
ktoso Jan 10, 2017 Member

yeah remove at will

@gosubpl
gosubpl Jan 11, 2017 Contributor

Tests removed. I'd rather submit a separate PR for uncommons.math dependency removal

+import scala.concurrent.duration.Duration
+
+/*
+akka-bench-jmh > jmh:run -wi 15 -i 15 -f 1 -t 1 .*RandomReplacementBenchmark.*
@gosubpl
gosubpl Jan 5, 2017 Contributor

Benchmark for reference purposes. Can be removed before merge.

@gosubpl
gosubpl Jan 11, 2017 Contributor

Benchmark removed

@akka-ci akka-ci added needs-attention and removed validating labels Jan 6, 2017
@akka-ci
Member
akka-ci commented Jan 6, 2017

Test FAILed.

@akka-ci akka-ci added validating and removed needs-attention labels Jan 6, 2017
@gosubpl
Contributor
gosubpl commented Jan 6, 2017 edited

@patriknw @drewhk this PR is now ready for review and almost ready for merge. The things pending are:

  • is RESEEDING_THRESHOLD = 1000000000L (one billion) ok? this is the threshold at which re-seeding will be started. Shouldn't be too low to avoid too frequent re-seeds. The safe limit (defined by RESEEDING_DEADLINE is on the order of 2^48 (140 thousand billion)
  • delete the RandomReplacement(Spec|Benchmark)
  • perhaps also delete the (now redundant) dependency on org.uncommons.math (rather a separate PR, as this will not be mergable since the Inet deletion was merged)
  • squash the commits!
@akka-ci akka-ci added tested and removed validating labels Jan 6, 2017
@akka-ci
Member
akka-ci commented Jan 6, 2017

Test PASSed.

+}
+
+private object CounterRNGConstants {
+ final val COUNTER_SIZE_BYTES = 16
@ktoso
ktoso Jan 10, 2017 Member

Please use Scala style for these, so CamelCaseForConstants.

@gosubpl
gosubpl Jan 11, 2017 Contributor

Refactored :)

@ktoso
ktoso approved these changes Jan 10, 2017 View changes

LGTM, please apply the last changes you mentioned and let's get this in.

+
+import scala.concurrent.ExecutionContext
+
+class RandomReplacementSpec extends AkkaSpec {
@ktoso
ktoso Jan 10, 2017 Member

yeah remove at will

+ * NOTE: this class is not serializable
+ */
+
+private[akka] class AESCounterBuiltinRNG(val seed: Array[Byte], implicit val executionContext: ExecutionContext,
@ktoso
ktoso Jan 10, 2017 Member

You can mark it using the new akka.annotation.InternalApi annotation

@gosubpl
gosubpl Jan 11, 2017 Contributor

Added FIXME comment. I will follow with a separate PR removing org.uncommons.math dependency and adding InternalApi right after merge of this one.

@gosubpl
Contributor
gosubpl commented Jan 10, 2017

@ktoso thanks for reviewing. I will implement all requested changes and resubmit later today.

@gosubpl gosubpl AES-CTR with re-seeding, review remarks included (#21740)
1f20c86
@gosubpl gosubpl changed the title from [wip] AES-CTR with re-seeding (#21740) to AES-CTR with re-seeding (#21740) Jan 11, 2017
@gosubpl
Contributor
gosubpl commented Jan 11, 2017

Latest review remarks included. Commits squashed. IMHO ready for merge. After this is merged I will follow with a separate PR on org.uncommons.math removal and InternalApi addition as I want to avoid rebasing.
@patriknw , @ktoso many thanks for reviewing and @drewhk , @wsargent for insightful comments in previous versions of this PR!

@akka-ci
Member
akka-ci commented Jan 11, 2017

Test PASSed.

@ktoso
Member
ktoso commented Jan 11, 2017

LGTM from my side, looks correct

@drewhk
Member
drewhk commented Jan 11, 2017

LGTM, on my side as well.

@ktoso
Member
ktoso commented Jan 11, 2017

Sounds like ready then, thanks a lot @gosubpl :)

@ktoso ktoso merged commit ebc6bc1 into akka:master Jan 11, 2017

2 checks passed

default Test PASSed.
Details
typesafe-cla-validator All users have signed the CLA
Details
@gosubpl gosubpl deleted the gosubpl:wip/12636-aes-ctr-prng-alternative-random branch Jan 11, 2017
@patriknw

Um, I left one review comment as pending yesterday. This needs a follow up PR, unless I'm wrong.

+}
+
+private class AESCounterBuiltinRNGReSeeder extends ThreadFactory {
+ override def newThread(r: Runnable): Thread = new Thread(r, "AESCounterBuiltinRNGReSeeder")
@patriknw
patriknw Jan 12, 2017 Member

I think this should be a daemon thread. setDaemon(true)

@gosubpl
gosubpl Jan 12, 2017 Contributor

Thanks for spotting. I will add this in the follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment