Details

      Description

      This JIRA is to implement Secure random using JNI to OpenSSL, and implementation should be thread-safe.

      Utilize RdRand to return random numbers from hardware random number generator. It's TRNG(True Random Number generators) having much higher performance than java.security.SecureRandom.

      https://wiki.openssl.org/index.php/Random_Numbers
      http://en.wikipedia.org/wiki/RdRand
      https://software.intel.com/en-us/articles/performance-impact-of-intel-secure-key-on-openssl

      1. HADOOP-10734.5.patch
        39 kB
        Yi Liu
      2. HADOOP-10734.4.patch
        39 kB
        Yi Liu
      3. HADOOP-10734-fs-enc.004.patch
        11 kB
        Colin Patrick McCabe
      4. HADOOP-10734.3.patch
        25 kB
        Yi Liu
      5. HADOOP-10734.2.patch
        24 kB
        Yi Liu
      6. HADOOP-10734.1.patch
        25 kB
        Yi Liu
      7. HADOOP-10734.patch
        25 kB
        Yi Liu

        Issue Links

          Activity

          Hide
          Colin Patrick McCabe added a comment -

          OK, I looked at this again, and Random implements Serializable (for some reason?) So we probably should have this to avoid generating warnings. So +1 for the patch as-is.

          Thanks, all. I will commit shortly and let you guys file whatever follow-ups make sense

          Show
          Colin Patrick McCabe added a comment - OK, I looked at this again, and Random implements Serializable (for some reason?) So we probably should have this to avoid generating warnings. So +1 for the patch as-is. Thanks, all. I will commit shortly and let you guys file whatever follow-ups make sense
          Hide
          Colin Patrick McCabe added a comment -

          It looks like this is still hanging around in v5:

          +public class OpensslSecureRandom extends Random {
          +  private static final long serialVersionUID = -7828193502768789584L;
          
          Show
          Colin Patrick McCabe added a comment - It looks like this is still hanging around in v5: + public class OpensslSecureRandom extends Random { + private static final long serialVersionUID = -7828193502768789584L;
          Hide
          Yi Liu added a comment -

          Thanks Mike Yoder, if RdRand not available, we will use default Openssl security random. That should be OK.

          Show
          Yi Liu added a comment - Thanks Mike Yoder , if RdRand not available, we will use default Openssl security random. That should be OK.
          Hide
          Yi Liu added a comment -

          Thanks Andrew Wang and Colin Patrick McCabe, update the patch for Colin's comments.

          Show
          Yi Liu added a comment - Thanks Andrew Wang and Colin Patrick McCabe , update the patch for Colin's comments.
          Hide
          Mike Yoder added a comment -

          Andrew Wang - RDRAND does indeed use SP 800-90 internally, which is good... but it's more convoluted than that. "Meets standards such as [blah blah blah]" is not the same as "you can actually be validated under FIPS or Common Criteria using this thing". Meeting those standards is the first step, jumping through other arbitrary hoops is the second.

          Support for the rdrand instruction is not in the latest openssl fips code (openssl-fips-2.0.7 - it's actually a separate code fork for significant parts of openssl). So one could not use a FIPS-compliant openssl library and use rdrand.

          Anyway, I'm not going to object on FIPS or crypto grounds. What we have now is an entirely reasonable first step.

          Show
          Mike Yoder added a comment - Andrew Wang - RDRAND does indeed use SP 800-90 internally, which is good... but it's more convoluted than that. "Meets standards such as [blah blah blah]" is not the same as "you can actually be validated under FIPS or Common Criteria using this thing". Meeting those standards is the first step, jumping through other arbitrary hoops is the second. Support for the rdrand instruction is not in the latest openssl fips code (openssl-fips-2.0.7 - it's actually a separate code fork for significant parts of openssl). So one could not use a FIPS-compliant openssl library and use rdrand. Anyway, I'm not going to object on FIPS or crypto grounds. What we have now is an entirely reasonable first step.
          Hide
          Colin Patrick McCabe added a comment -

          Andrew Wang: yeah, I agree. Mike?

          +public class OpensslSecureRandom extends Random {
          +  private static final long serialVersionUID = -7828193502768789584L;
          

          We don't need the serialVersionUID.

          I think CRYPTO_OS_RANDOM_DEV_PATH and CRYPTO_OS_RANDOM_DEV_PATH_DEFAULT belong in CommonConfigurationKeysPublic with the other keys.

          +1 pending these fixes

          Show
          Colin Patrick McCabe added a comment - Andrew Wang : yeah, I agree. Mike? + public class OpensslSecureRandom extends Random { + private static final long serialVersionUID = -7828193502768789584L; We don't need the serialVersionUID. I think CRYPTO_OS_RANDOM_DEV_PATH and CRYPTO_OS_RANDOM_DEV_PATH_DEFAULT belong in CommonConfigurationKeysPublic with the other keys. +1 pending these fixes
          Hide
          Andrew Wang added a comment -

          I think the OpenSSL mode that Yi's patch added is FIPS compliant. Wikipedia says that RdRand meets "standards such as NIST SP 800-90A,[4] FIPS 140-2, and ANSI X9.82.[2]", so users who care can enable that. At the same time, it seems like others would like to use /dev/urandom, and this patch also allows that.

          Mike, does this work for you? I think we could also commit what's here right now, and re-examine FIPS compliance if necessary in a follow-on JIRA.

          Show
          Andrew Wang added a comment - I think the OpenSSL mode that Yi's patch added is FIPS compliant. Wikipedia says that RdRand meets "standards such as NIST SP 800-90A, [4] FIPS 140-2, and ANSI X9.82. [2] ", so users who care can enable that. At the same time, it seems like others would like to use /dev/urandom, and this patch also allows that. Mike, does this work for you? I think we could also commit what's here right now, and re-examine FIPS compliance if necessary in a follow-on JIRA.
          Hide
          Mike Yoder added a comment -

          Another consideration in the US is meeting the FIPS specification regarding random number generation. The latest guidance for FIPS requires the use of NIST SP 800-90 (http://csrc.nist.gov/publications/nistpubs/800-90A/SP800-90A.pdf) for random number generation. We can get this "for free" using openssl, if openssl is recent and compiled in FIPS mode. So we should at least have a switch that enables the use of openssl's prng (I confess I haven't looked at the patches...). I'm not sure if there is a java implementation of SP 800-90 or not...

          Show
          Mike Yoder added a comment - Another consideration in the US is meeting the FIPS specification regarding random number generation. The latest guidance for FIPS requires the use of NIST SP 800-90 ( http://csrc.nist.gov/publications/nistpubs/800-90A/SP800-90A.pdf ) for random number generation. We can get this "for free" using openssl, if openssl is recent and compiled in FIPS mode. So we should at least have a switch that enables the use of openssl's prng (I confess I haven't looked at the patches...). I'm not sure if there is a java implementation of SP 800-90 or not...
          Hide
          Yi Liu added a comment -

          Hi Colin Patrick McCabe and Andrew Wang, I add the alternate implementation to the patch, user can choose through configuration.
          Colin, for your implementation, I do a bit update:

          • Add thread-safe
          • override void nextBytes(byte[] bytes) and implement using System.arraycopy, it's more efficient.
          Show
          Yi Liu added a comment - Hi Colin Patrick McCabe and Andrew Wang , I add the alternate implementation to the patch, user can choose through configuration. Colin, for your implementation, I do a bit update: Add thread-safe override void nextBytes(byte[] bytes) and implement using System.arraycopy, it's more efficient.
          Hide
          Yi Liu added a comment -

          Thanks Colin Patrick McCabe for an alternate implementation, it's very good.
          Thanks Andrew Wang for your comments.

          I will add it as one of the implementations and user can choose through configuration. I will update the patch later. Thanks for your effort.

          Show
          Yi Liu added a comment - Thanks Colin Patrick McCabe for an alternate implementation, it's very good. Thanks Andrew Wang for your comments. I will add it as one of the implementations and user can choose through configuration. I will update the patch later. Thanks for your effort.
          Hide
          Andrew Wang added a comment -

          Hi Colin and Yi, do you mind expanding a bit more on these issues with /dev/urandom?

          This source [1] indicates that initial seeding isn't really an issue unless you're cloning VMs, and even then you can fix it. I'm not sure who wrote that earlier blog, but crypto experts like DJB [2] and others [3] like urandom. The kernel also mixes in entropy from the chip, so if the Intel instructions are present, it's no less secure. The kernel also has additional entropy that OpenSSL in userspace doesn't have access to, so you'd think it'd be higher quality randomness.

          When it comes to portability, FreeBSD/MacOS have /dev/urandom with very compatible behavior. Windows has its own APIs, but again, kernel-level randomness should be higher quality than anything we get from userspace (assuming that Windows mixes in chip randomness).

          [1] http://www.2uo.de/myths-about-urandom/
          [2] http://www.mail-archive.com/cryptography@randombit.net/msg04763.html
          [3] http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/

          Show
          Andrew Wang added a comment - Hi Colin and Yi, do you mind expanding a bit more on these issues with /dev/urandom? This source [1] indicates that initial seeding isn't really an issue unless you're cloning VMs, and even then you can fix it. I'm not sure who wrote that earlier blog, but crypto experts like DJB [2] and others [3] like urandom. The kernel also mixes in entropy from the chip, so if the Intel instructions are present, it's no less secure. The kernel also has additional entropy that OpenSSL in userspace doesn't have access to, so you'd think it'd be higher quality randomness. When it comes to portability, FreeBSD/MacOS have /dev/urandom with very compatible behavior. Windows has its own APIs, but again, kernel-level randomness should be higher quality than anything we get from userspace (assuming that Windows mixes in chip randomness). [1] http://www.2uo.de/myths-about-urandom/ [2] http://www.mail-archive.com/cryptography@randombit.net/msg04763.html [3] http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/
          Hide
          Colin Patrick McCabe added a comment -

          Yi and I had an offline conversation about this. Essentially, there are some issues with /dev/urandom and /dev/random on Linux. One is that they're not portable to BSD and other OSes, whereas the openssl stuff is. Another is that if /dev/urandom is incorrectly seeded, the randomness may not be adequate. There's more background on the difficulty of using /dev/urandom for portable cryptography here: http://insanecoding.blogspot.com/2014/05/a-good-idea-with-bad-usage-devurandom.html

          For these reasons, I'm +1 on v3 of the patch (Yi's version that uses openssl).

          Thanks, guys.

          Show
          Colin Patrick McCabe added a comment - Yi and I had an offline conversation about this. Essentially, there are some issues with /dev/urandom and /dev/random on Linux. One is that they're not portable to BSD and other OSes, whereas the openssl stuff is. Another is that if /dev/urandom is incorrectly seeded, the randomness may not be adequate. There's more background on the difficulty of using /dev/urandom for portable cryptography here: http://insanecoding.blogspot.com/2014/05/a-good-idea-with-bad-usage-devurandom.html For these reasons, I'm +1 on v3 of the patch (Yi's version that uses openssl). Thanks, guys.
          Hide
          Colin Patrick McCabe added a comment -

          Hi Yi,

          I wanted to present an alternate implementation here. This implementation just reads from /dev/urandom. This has a few advantages:

          • does not require JNI or openssl to be installed
          • mixes in additional randomness sources in addition to RDRAND (on Intel machines with Linux, RDRAND will be included, of course)
          • a lot shorter

          Because it reads a large chunk (8k) at a time from /dev/urandom, the overhead of the read system call should be pretty low. Take a look and see if this makes sense to you. I'm curious what the performance difference is... maybe if the openssl method is a lot faster, we should still go with that. But I wanted us to consider this.

          Show
          Colin Patrick McCabe added a comment - Hi Yi, I wanted to present an alternate implementation here. This implementation just reads from /dev/urandom . This has a few advantages: does not require JNI or openssl to be installed mixes in additional randomness sources in addition to RDRAND (on Intel machines with Linux, RDRAND will be included, of course) a lot shorter Because it reads a large chunk (8k) at a time from /dev/urandom , the overhead of the read system call should be pretty low. Take a look and see if this makes sense to you. I'm curious what the performance difference is... maybe if the openssl method is a lot faster, we should still go with that. But I wanted us to consider this.
          Hide
          Yi Liu added a comment -

          Thanks Colin Patrick McCabe for review. I update the patch for your comments.

          I was just suggesting looping until they're not equal. This catches the case where it's always returning a constant value (it will timeout). So I don't see why we "need to assert something."

          OK, I see. I update them in the new patch.

          This is still wrong. If you don't want to use gettid, you can use some code like this:

          After discussion with you, I have further looked into Openssl implementation. You are right, Openssl just requires distinct ids for different threads(It compares locking thread id with thread id got from callback to decide whether lock for operations). So both the two approaches you suggested are good, I prefer gettid.
          In the new patch, I make syscall for SYS_gettid.

          Show
          Yi Liu added a comment - Thanks Colin Patrick McCabe for review. I update the patch for your comments. I was just suggesting looping until they're not equal. This catches the case where it's always returning a constant value (it will timeout). So I don't see why we "need to assert something." OK, I see. I update them in the new patch. This is still wrong. If you don't want to use gettid, you can use some code like this: After discussion with you, I have further looked into Openssl implementation. You are right, Openssl just requires distinct ids for different threads(It compares locking thread id with thread id got from callback to decide whether lock for operations). So both the two approaches you suggested are good, I prefer gettid. In the new patch, I make syscall for SYS_gettid.
          Hide
          Colin Patrick McCabe added a comment -

          Alejandro Abdelnur: I agree with Yi's idea of making this a separate class; coupling it with CryptoCodec would be confusing. Although they both use the openssl library, they use different C functions. For example, these functions are not needed for the crypto codec stuff, only for the random stuff:

          +  LOAD_DYNAMIC_SYMBOL(dlsym_ENGINE_finish, env, openssl, "ENGINE_finish");
          +  LOAD_DYNAMIC_SYMBOL(dlsym_ENGINE_free, env, openssl, "ENGINE_free");
          +  LOAD_DYNAMIC_SYMBOL(dlsym_ENGINE_cleanup, env, openssl, "ENGINE_cleanup");
          +  LOAD_DYNAMIC_SYMBOL(dlsym_RAND_bytes, env, openssl, "RAND_bytes");
          +  LOAD_DYNAMIC_SYMBOL(dlsym_ERR_get_error, env, openssl, "ERR_get_error");
          

          [Yi wrote]: I agree it’s not good to test true random numbers in this way. I try to loop until rand2 is not equal to rand1, but then we need to Assert something, your suggestion is?

          I was just suggesting looping until they're not equal. This catches the case where it's always returning a constant value (it will timeout). So I don't see why we "need to assert something."

          There definitely are more sophisticated tests for randomness out there, but that would require a bit of research and might be best to do in another JIRA, if we do it.

          +static unsigned long pthreads_thread_id(void)
          +{
          +  return (unsigned long)pthread_self();
          +}
          

          This is still wrong. If you don't want to use gettid, you can use some code like this:

          pthread_key_t key;
          unsigned long highest_thread_id;
          
          static unsigned long pthreads_thread_id(void)
          {
            void *v;
            unsigned long id;
          
            v = pthread_getspecific(key);
            if (v) {
              return (unsigned long)(uintptr_t)v;
            }
            id = __add_and_fetch(&highest_thread_id, 1);
            pthread_setspecific(key, (void*)id);
            return id;
          }
          

          You'll need to manage setting up and tearing down the pthread_key_t as well.

          Show
          Colin Patrick McCabe added a comment - Alejandro Abdelnur : I agree with Yi's idea of making this a separate class; coupling it with CryptoCodec would be confusing. Although they both use the openssl library, they use different C functions. For example, these functions are not needed for the crypto codec stuff, only for the random stuff: + LOAD_DYNAMIC_SYMBOL(dlsym_ENGINE_finish, env, openssl, "ENGINE_finish" ); + LOAD_DYNAMIC_SYMBOL(dlsym_ENGINE_free, env, openssl, "ENGINE_free" ); + LOAD_DYNAMIC_SYMBOL(dlsym_ENGINE_cleanup, env, openssl, "ENGINE_cleanup" ); + LOAD_DYNAMIC_SYMBOL(dlsym_RAND_bytes, env, openssl, "RAND_bytes" ); + LOAD_DYNAMIC_SYMBOL(dlsym_ERR_get_error, env, openssl, "ERR_get_error" ); [Yi wrote]: I agree it’s not good to test true random numbers in this way. I try to loop until rand2 is not equal to rand1, but then we need to Assert something, your suggestion is? I was just suggesting looping until they're not equal. This catches the case where it's always returning a constant value (it will timeout). So I don't see why we "need to assert something." There definitely are more sophisticated tests for randomness out there, but that would require a bit of research and might be best to do in another JIRA, if we do it. + static unsigned long pthreads_thread_id(void) +{ + return (unsigned long )pthread_self(); +} This is still wrong. If you don't want to use gettid, you can use some code like this: pthread_key_t key; unsigned long highest_thread_id; static unsigned long pthreads_thread_id(void) { void *v; unsigned long id; v = pthread_getspecific(key); if (v) { return (unsigned long )(uintptr_t)v; } id = __add_and_fetch(&highest_thread_id, 1); pthread_setspecific(key, (void*)id); return id; } You'll need to manage setting up and tearing down the pthread_key_t as well.
          Hide
          Yi Liu added a comment -

          Refine the patch to let OpensslSecureRandom extends java.util.Random.

          Show
          Yi Liu added a comment - Refine the patch to let OpensslSecureRandom extends java.util.Random .
          Hide
          Yi Liu added a comment -

          Colin Patrick McCabe, I update the patch for your comments.

          If you want to do this in a follow-on JIRA, that's OK too.

          Thanks, let do it together in HADOOP-10735

          I think it's confusing that we have both org.apache.hadoop.crypto.random.SecureRandom andjava.security.SecureRandom. Maybe a better name for this new class would be OpenSslSecureRandom or something like that, to emphasize that it is using OpenSSL to get random bits.

          Right, I change the name to OpensslSecureRandom

          I think the comment is a bit misleading here. Openssl compiles on a lot of platforms that don't have RDRAND. So all we really know here is that we're using openssl, not that we're using RDRAND. I think it's appropriate to have a comment saying, "if you are using an Intel chipset with RDRAND, the high-performance random number generator will be used", or something like that. But it's platform specific and we may be compiling on another platform.

          OK, I change the comment to If using an Intel chipset with RDRAND, the high-performance hardware random number generator will be used.

          It's definitely difficult to test something which is returning true random numbers. It requires a lot of mathematics. So I see why you did it this way. Just one comment... maybe I'm being overly paranoid here, but can we loop until rand2 is not equal to rand1?

          I agree it’s not good to test true random numbers in this way. I try to loop until rand2 is not equal to rand1, but then we need to Assert something, your suggestion is?

          The stuff in /usr/include/bits is not public; it is an implementation detail that could change at any time.

          Agree, but maybe it’s not suitable to use gettid as a replacement, since from man page

          The thread ID returned by this call is not the same thing as a POSIX thread ID (i.e., the opaque value returned by pthread_self(3)).

          I’m thinking we are not including /usr/include/bits/pthreadtypes.h directly, we are using pthread.h which is public. If bits/pthreadtypes.h changed, there should be some other place to define pthread_t and make sure existing programs using pthread still work.
          Or you have other good way to replace pthread_self.

          Show
          Yi Liu added a comment - Colin Patrick McCabe , I update the patch for your comments. If you want to do this in a follow-on JIRA, that's OK too. Thanks, let do it together in HADOOP-10735 I think it's confusing that we have both org.apache.hadoop.crypto.random.SecureRandom andjava.security.SecureRandom. Maybe a better name for this new class would be OpenSslSecureRandom or something like that, to emphasize that it is using OpenSSL to get random bits. Right, I change the name to OpensslSecureRandom I think the comment is a bit misleading here. Openssl compiles on a lot of platforms that don't have RDRAND. So all we really know here is that we're using openssl, not that we're using RDRAND. I think it's appropriate to have a comment saying, "if you are using an Intel chipset with RDRAND, the high-performance random number generator will be used", or something like that. But it's platform specific and we may be compiling on another platform. OK, I change the comment to If using an Intel chipset with RDRAND, the high-performance hardware random number generator will be used. It's definitely difficult to test something which is returning true random numbers. It requires a lot of mathematics. So I see why you did it this way. Just one comment... maybe I'm being overly paranoid here, but can we loop until rand2 is not equal to rand1? I agree it’s not good to test true random numbers in this way. I try to loop until rand2 is not equal to rand1, but then we need to Assert something, your suggestion is? The stuff in /usr/include/bits is not public; it is an implementation detail that could change at any time. Agree, but maybe it’s not suitable to use gettid as a replacement, since from man page The thread ID returned by this call is not the same thing as a POSIX thread ID (i.e., the opaque value returned by pthread_self(3)). I’m thinking we are not including /usr/include/bits/pthreadtypes.h directly, we are using pthread.h which is public. If bits/pthreadtypes.h changed, there should be some other place to define pthread_t and make sure existing programs using pthread still work. Or you have other good way to replace pthread_self .
          Hide
          Yi Liu added a comment -

          Thanks Alejandro Abdelnur for your comments.

          Openssl secure random is separated functionality and is used in OpensslAesCtrCryptoCodec and also can be used directly. We should have a java class couple with the JNI implementation. It’s not suitable to put

          private native static void initSR();
          private native boolean nextRandBytes(byte[] bytes);
          

          into OpensslCipher. Having two classes makes code more clear. OpensslAesCtrCryptoCodec doesn't contain native methods directly, it will use OpensslCipher and Openssl secure random.

          Show
          Yi Liu added a comment - Thanks Alejandro Abdelnur for your comments. Openssl secure random is separated functionality and is used in OpensslAesCtrCryptoCodec and also can be used directly. We should have a java class couple with the JNI implementation. It’s not suitable to put private native static void initSR(); private native boolean nextRandBytes( byte [] bytes); into OpensslCipher . Having two classes makes code more clear. OpensslAesCtrCryptoCodec doesn't contain native methods directly, it will use OpensslCipher and Openssl secure random.
          Hide
          Yi Liu added a comment -

          Thanks Colin Patrick McCabe for your review, I will updated the patch and respond you later.

          Show
          Yi Liu added a comment - Thanks Colin Patrick McCabe for your review, I will updated the patch and respond you later.
          Hide
          Alejandro Abdelnur added a comment -

          Any reason for not baking this in the CryptoCodec for OpenSSL instead a new java class?

          Show
          Alejandro Abdelnur added a comment - Any reason for not baking this in the CryptoCodec for OpenSSL instead a new java class?
          Hide
          Colin Patrick McCabe added a comment -

          [discussion of randomness methods]

          I don't think /dev/random is a practical choice, due to the blocking issue. From what I've heard, /dev/urandom can often be a good choice. I'm tempted to try a super-simple piece of code that just periodically fills a big buffer from /dev/urandom and see if that performs well. I have a hunch that it would (and it would also use RDRAND on supported platforms.) But I think it's fine if you want to provide an option to go through openssl as well... we already have a dependency on that library.

          And we can also add enable flag in configuration and user can disable it.

          I agree. I think we should have a configuration option like encryption.random.number.generator which specifies a comma-separated list of class names to try to use. That way a user could specify the openssl one plus a fallback to the standard java one if they so chose. Or alternately, the user could enable just the java one (and configure it to use /dev/urandom) to get something which used RDRAND plus some additional randomness. If you want to do this in a follow-on JIRA, that's OK too.

          I think it's confusing that we have both org.apache.hadoop.crypto.random.SecureRandom and java.security.SecureRandom. Maybe a better name for this new class would be OpenSslSecureRandom or something like that, to emphasize that it is using OpenSSL to get random bits.

          +/**
          + * Utilize RdRand to return random numbers from hardware random number 
          + * generator. It's TRNG(True Random Number generators) having high performance. 
          + * https://wiki.openssl.org/index.php/Random_Numbers#Hardware
          + * http://en.wikipedia.org/wiki/RdRand
          + */
          +static ENGINE * rdrand_init(JNIEnv *env)
          

          I think the comment is a bit misleading here. Openssl compiles on a lot of platforms that don't have RDRAND. So all we really know here is that we're using openssl, not that we're using RDRAND. I think it's appropriate to have a comment saying, "if you are using an Intel chipset with RDRAND, the high-performance random number generator will be used", or something like that. But it's platform specific and we may be compiling on another platform.

          +  \@Test(timeout=120000)
          +  public void testRandomInt() throws Exception {
          +    SecureRandom random = new SecureRandom();
          +    
          +    int rand1 = random.nextInt();
          +    int rand2 = random.nextInt();
          +    Assert.assertFalse(rand1 == rand2);
          +  }
          

          It's definitely difficult to test something which is returning true random numbers. It requires a lot of mathematics. So I see why you did it this way. Just one comment... maybe I'm being overly paranoid here, but can we loop until rand2 is not equal to rand1?

          I suppose you mean direct ByteBuffer. Per my understanding, merit of direct ByteBuffer is to avoid bytes copy. But SecureRandom#nextBytes will accept an pre-allocated byte[] array, if we use direct ByteBuffer for JNI, then there is additional copy in java layer, so the performance is the same, and we need to manage the direct ByteBuffer.

          OK.

          Can you explain a bit more, I’m not sure I get your meaning. Per my understanding, pthread_t is defined in /usr/include/bits/pthreadtypes.h as

          The stuff in /usr/include/bits is not public; it is an implementation detail that could change at any time.

          from man pthread_self:

          POSIX.1 allows an implementation wide freedom in choosing the type used to represent a thread ID; for example, representation using either an arithmetic type or a structure is permitted. Therefore, variables of type pthread_t can't portably be compared using the C equality operator (==); use pthread_equal(3) instead. Thread identifiers should be considered opaque: any attempt to use a thread ID other than in pthreads calls is nonportable and can lead to unspecified results.

          Show
          Colin Patrick McCabe added a comment - [discussion of randomness methods] I don't think /dev/random is a practical choice, due to the blocking issue. From what I've heard, /dev/urandom can often be a good choice. I'm tempted to try a super-simple piece of code that just periodically fills a big buffer from /dev/urandom and see if that performs well. I have a hunch that it would (and it would also use RDRAND on supported platforms.) But I think it's fine if you want to provide an option to go through openssl as well... we already have a dependency on that library. And we can also add enable flag in configuration and user can disable it. I agree. I think we should have a configuration option like encryption.random.number.generator which specifies a comma-separated list of class names to try to use. That way a user could specify the openssl one plus a fallback to the standard java one if they so chose. Or alternately, the user could enable just the java one (and configure it to use /dev/urandom) to get something which used RDRAND plus some additional randomness. If you want to do this in a follow-on JIRA, that's OK too. I think it's confusing that we have both org.apache.hadoop.crypto.random.SecureRandom and java.security.SecureRandom . Maybe a better name for this new class would be OpenSslSecureRandom or something like that, to emphasize that it is using OpenSSL to get random bits. +/** + * Utilize RdRand to return random numbers from hardware random number + * generator. It's TRNG(True Random Number generators) having high performance. + * https: //wiki.openssl.org/index.php/Random_Numbers#Hardware + * http: //en.wikipedia.org/wiki/RdRand + */ + static ENGINE * rdrand_init(JNIEnv *env) I think the comment is a bit misleading here. Openssl compiles on a lot of platforms that don't have RDRAND. So all we really know here is that we're using openssl, not that we're using RDRAND. I think it's appropriate to have a comment saying, "if you are using an Intel chipset with RDRAND, the high-performance random number generator will be used", or something like that. But it's platform specific and we may be compiling on another platform. + \@Test(timeout=120000) + public void testRandomInt() throws Exception { + SecureRandom random = new SecureRandom(); + + int rand1 = random.nextInt(); + int rand2 = random.nextInt(); + Assert.assertFalse(rand1 == rand2); + } It's definitely difficult to test something which is returning true random numbers. It requires a lot of mathematics. So I see why you did it this way. Just one comment... maybe I'm being overly paranoid here, but can we loop until rand2 is not equal to rand1? I suppose you mean direct ByteBuffer. Per my understanding, merit of direct ByteBuffer is to avoid bytes copy. But SecureRandom#nextBytes will accept an pre-allocated byte[] array, if we use direct ByteBuffer for JNI, then there is additional copy in java layer, so the performance is the same, and we need to manage the direct ByteBuffer. OK. Can you explain a bit more, I’m not sure I get your meaning. Per my understanding, pthread_t is defined in /usr/include/bits/pthreadtypes.h as The stuff in /usr/include/bits is not public; it is an implementation detail that could change at any time. from man pthread_self : POSIX.1 allows an implementation wide freedom in choosing the type used to represent a thread ID; for example, representation using either an arithmetic type or a structure is permitted. Therefore, variables of type pthread_t can't portably be compared using the C equality operator (==); use pthread_equal(3) instead. Thread identifiers should be considered opaque: any attempt to use a thread ID other than in pthreads calls is nonportable and can lead to unspecified results.
          Hide
          Yi Liu added a comment -

          Colin Patrick McCabe, thanks for the review .

          I actually have the same problem with the scheme here: JNI calls are expensive... do we know how many random bits the API user is getting at a time? If that number is small, we might want to implement batching.

          In most cases, we use it to generate key(16bytes, 32bytes, 128bytes, 256bytes), IV(16 bytes), long (8 bytes).
          Furthermore, to make the random bytes good enough, we can’t avoid JNI, even java.security.SecureRandom also uses JNI.

          I also think we should consider using ByteBuffer rather than byte[] array, if performance is the primary goal.

          I suppose you mean direct ByteBuffer. Per my understanding, merit of direct ByteBuffer is to avoid bytes copy. But SecureRandom#nextBytes will accept an pre-allocated byte[] array, if we use direct ByteBuffer for JNI, then there is additional copy in java layer, so the performance is the same, and we need to manage the direct ByteBuffer.

          +  final protected int next(int numBits) {
          

          Should be private

          OK, I will update it.

          +  public long nextLong() {
          +    return ((long)(next(32)) << 32) + next(32);
          +  }
          

          Why use addition rather than bitwise OR here?

          Bitwise OR is also OK. Actually nextLong, nextFloat and nextDouble are copied from implementations in java.security.SecureRandom

          This is not correct. The type of pthread_t is not known. If you want a numeric thread ID, you could try gettid on Linux.

          Can you explain a bit more, I’m not sure I get your meaning. Per my understanding, pthread_t is defined in /usr/include/bits/pthreadtypes.h as

          typedef unsigned long int pthread_t;
          

          And this patch is compiled and run successfully on my Linux server.

          Show
          Yi Liu added a comment - Colin Patrick McCabe , thanks for the review . I actually have the same problem with the scheme here: JNI calls are expensive... do we know how many random bits the API user is getting at a time? If that number is small, we might want to implement batching. In most cases, we use it to generate key(16bytes, 32bytes, 128bytes, 256bytes), IV(16 bytes), long (8 bytes). Furthermore, to make the random bytes good enough, we can’t avoid JNI, even java.security.SecureRandom also uses JNI. I also think we should consider using ByteBuffer rather than byte[] array, if performance is the primary goal. I suppose you mean direct ByteBuffer. Per my understanding, merit of direct ByteBuffer is to avoid bytes copy. But SecureRandom#nextBytes will accept an pre-allocated byte[] array, if we use direct ByteBuffer for JNI, then there is additional copy in java layer, so the performance is the same, and we need to manage the direct ByteBuffer. + final protected int next( int numBits) { Should be private OK, I will update it. + public long nextLong() { + return (( long )(next(32)) << 32) + next(32); + } Why use addition rather than bitwise OR here? Bitwise OR is also OK. Actually nextLong , nextFloat and nextDouble are copied from implementations in java.security.SecureRandom This is not correct. The type of pthread_t is not known. If you want a numeric thread ID, you could try gettid on Linux. Can you explain a bit more, I’m not sure I get your meaning. Per my understanding, pthread_t is defined in /usr/include/bits/pthreadtypes.h as typedef unsigned long int pthread_t; And this patch is compiled and run successfully on my Linux server.
          Hide
          Yi Liu added a comment -

          And we can also add enable flag in configuration and user can disable it.

          Show
          Yi Liu added a comment - And we can also add enable flag in configuration and user can disable it.
          Hide
          Yi Liu added a comment -

          Thanks Colin Patrick McCabe, Andrew Purtell, Andrew Wang for the comments.

          I summarize several ways to generate secure random in linux, and why RdRand.

          • /dev/random, it uses an entropy pool of several entropy sources, such as mouse movement, keyboard type and so on. If entropy pool is empty, reads to /dev/random will be blocked until additional environment noise is gathered.
            RdRand is used to improve the entropy by combining the values received from RdRand with other sources of randomness.
            The reason of the combining way is some developers concern there may be back doors in RdRand, but it’s not true.
          • /dev/urandom, it reuses the internal entropy pool and will return as many random bytes as requested. The call will not block, and the outpout may contain less entropy than the corresponding read from /dev/random. If the entropy pool is empty, it will generate data using SHA or other algorithms.
          • In java, new SecureRandom(), will read bytes from /dev/urandom and do xor with bytes from java SHA1PRNG.
          • RdRand, hardware generator. In Openssl, it’s recommended to use hardware generators, it says their entropy is always nearly 100%. We can use RdRand directly.

          So we can see, option 4, the RdRand is faster than others and the entropy is nearly 100%.

          http://en.wikipedia.org/wiki/RdRand
          http://wiki.openssl.org/index.php/Random_Numbers
          http://en.wikipedia.org/?title=/dev/random
          http://docs.oracle.com/javase/7/docs/api/java/security/SecureRandom.html

          Show
          Yi Liu added a comment - Thanks Colin Patrick McCabe , Andrew Purtell , Andrew Wang for the comments. I summarize several ways to generate secure random in linux, and why RdRand. /dev/random, it uses an entropy pool of several entropy sources, such as mouse movement, keyboard type and so on. If entropy pool is empty, reads to /dev/random will be blocked until additional environment noise is gathered. RdRand is used to improve the entropy by combining the values received from RdRand with other sources of randomness. The reason of the combining way is some developers concern there may be back doors in RdRand, but it’s not true. /dev/urandom, it reuses the internal entropy pool and will return as many random bytes as requested. The call will not block, and the outpout may contain less entropy than the corresponding read from /dev/random. If the entropy pool is empty, it will generate data using SHA or other algorithms. In java, new SecureRandom(), will read bytes from /dev/urandom and do xor with bytes from java SHA1PRNG. RdRand, hardware generator. In Openssl, it’s recommended to use hardware generators, it says their entropy is always nearly 100%. We can use RdRand directly. So we can see, option 4, the RdRand is faster than others and the entropy is nearly 100%. http://en.wikipedia.org/wiki/RdRand http://wiki.openssl.org/index.php/Random_Numbers http://en.wikipedia.org/?title=/dev/random http://docs.oracle.com/javase/7/docs/api/java/security/SecureRandom.html
          Hide
          Colin Patrick McCabe added a comment -

          I guess one advantage of the proposed scheme here is that it works on Windows and BSD, where /dev/urandom doesn't exist (although there may be other platform-specific mechanisms).

          It seems easy to read a few kilobytes from /dev/urandom at a time, so that you don't find yourself doing lots of small read operations. I actually have the same problem with the scheme here: JNI calls are expensive... do we know how many random bits the API user is getting at a time? If that number is small, we might want to implement batching. I also think we should consider using ByteBuffer rather than byte[] array, if performance is the primary goal.

          +static unsigned long pthreads_thread_id(void)
          +{
          +  return (unsigned long)pthread_self();
          +}
          

          This is not correct. The type of pthread_t is not known. If you want a numeric thread ID, you could try gettid on Linux.

          +  public long nextLong() {
          +    return ((long)(next(32)) << 32) + next(32);
          +  }
          

          Why use addition rather than bitwise OR here?

          +  final protected int next(int numBits) {
          

          Should be private

          Show
          Colin Patrick McCabe added a comment - I guess one advantage of the proposed scheme here is that it works on Windows and BSD, where /dev/urandom doesn't exist (although there may be other platform-specific mechanisms). It seems easy to read a few kilobytes from /dev/urandom at a time, so that you don't find yourself doing lots of small read operations. I actually have the same problem with the scheme here: JNI calls are expensive... do we know how many random bits the API user is getting at a time? If that number is small, we might want to implement batching. I also think we should consider using ByteBuffer rather than byte[] array, if performance is the primary goal. + static unsigned long pthreads_thread_id(void) +{ + return (unsigned long )pthread_self(); +} This is not correct. The type of pthread_t is not known. If you want a numeric thread ID, you could try gettid on Linux. + public long nextLong() { + return (( long )(next(32)) << 32) + next(32); + } Why use addition rather than bitwise OR here? + final protected int next( int numBits) { Should be private
          Hide
          Andrew Wang added a comment -

          I have only a modest background in this area, but my understanding is that the Linux random subsystem is carefully designed to mix in different entropy sources. This prevents a single bad entropy source from leading to poor entropy. [1] has some info from Ted Ts'o, who's the maintainer of the random subsystem; he explicitly mentions the importance of using a mixed source of randomness for things like encryption keys.

          How bad is the perf difference going to be for /dev/urandom with rdrand mixed in vs direct rdrand? Is it going to matter for our workloads?

          [1] https://news.ycombinator.com/item?id=6038473

          Show
          Andrew Wang added a comment - I have only a modest background in this area, but my understanding is that the Linux random subsystem is carefully designed to mix in different entropy sources. This prevents a single bad entropy source from leading to poor entropy. [1] has some info from Ted Ts'o, who's the maintainer of the random subsystem; he explicitly mentions the importance of using a mixed source of randomness for things like encryption keys. How bad is the perf difference going to be for /dev/urandom with rdrand mixed in vs direct rdrand? Is it going to matter for our workloads? [1] https://news.ycombinator.com/item?id=6038473
          Hide
          Andrew Purtell added a comment -

          When using Linux, why not just read from /dev/urandom? That will also use the RdRand feature when available.

          I think this can be done with the default SecureRandom implementation, with -Djava.security.egd=file:/dev/./urandom . However there is file IO involved, it may still be faster to invoke the rdrand instruction directly. Docs suggest a direct throughput of ~6Gbps.

          Show
          Andrew Purtell added a comment - When using Linux, why not just read from /dev/urandom? That will also use the RdRand feature when available. I think this can be done with the default SecureRandom implementation, with -Djava.security.egd= file:/dev/./urandom . However there is file IO involved, it may still be faster to invoke the rdrand instruction directly. Docs suggest a direct throughput of ~6Gbps.
          Hide
          Colin Patrick McCabe added a comment -

          When using Linux, why not just read from /dev/urandom? That will also use the RdRand feature when available.

          Show
          Colin Patrick McCabe added a comment - When using Linux, why not just read from /dev/urandom ? That will also use the RdRand feature when available.
          Hide
          Yi Liu added a comment -

          RdRand begins from Ivy Bridge processors, you can get part of enabled CPUs from the list: http://en.wikipedia.org/wiki/List_of_Intel_Xeon_microprocessors#Ivy_Bridge-based_Xeons

          Show
          Yi Liu added a comment - RdRand begins from Ivy Bridge processors, you can get part of enabled CPUs from the list: http://en.wikipedia.org/wiki/List_of_Intel_Xeon_microprocessors#Ivy_Bridge-based_Xeons
          Hide
          Yi Liu added a comment -

          We get 22x speedup using this implementation to generate random bytes, and it's True Random Number generators.
          java.security.SecureRandom only generates pseudo random bytes.

          Test environment:
          Generate 100000 num keys (256 bytes)
          CPU: Intel(R) Xeon(R) CPU E3-1280 V2 @ 3.60GHz
          Mem: 16GB
          OpenSSL version: openssl-1.0.1h

          java.security.SecureRandom takes 2510ms
          RdRand implementation takes 114ms

          Show
          Yi Liu added a comment - We get 22x speedup using this implementation to generate random bytes, and it's True Random Number generators . java.security.SecureRandom only generates pseudo random bytes. Test environment: Generate 100000 num keys (256 bytes) CPU: Intel(R) Xeon(R) CPU E3-1280 V2 @ 3.60GHz Mem: 16GB OpenSSL version: openssl-1.0.1h java.security.SecureRandom takes 2510ms RdRand implementation takes 114ms

            People

            • Assignee:
              Yi Liu
              Reporter:
              Yi Liu
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development