aremily commented on issue #92: OpenSSL 1.1.0 updates with backward compatibility for OpenSSL 1.0.2 and 1.0.1 URL: https://github.com/apache/commons-crypto/pull/92#issuecomment-451248384 Thanks for the feedback. Let me think on it a bit more and I'll take it up again once I've come up with an approach. Perhaps ossdev07 would be interested in collaborating, as it seems we're both trying to achive the same objective. On Wed, Jan 2, 2019 at 2:00 PM Marcelo Vanzin <notificati...@github.com> wrote: > *@vanzin* commented on this pull request. > > Sorry, but your code has exactly the same problem as other attempts at > doing this. > > Anytime you compile things conditionally, it means the JNI library only > works against a specific version of OpenSSL. We can't have that, unless you > also change the Java code to load different JNI libraries depending on the > OpenSSL version, and the libraries are created with a different name > depending on the version. And I don't see your code doing that. > > (I also have a preference for a single JNI library, but if you do the > alternative I won't complain too much.) > > I would also be good to add 1.1 to the test matrix; seems like the latest > Linux on Travis is 16.04, which still has 1.0.2; so maybe re-enabling > MacOS? See the .travis.yaml change in: > dc28a2b > <https://github.com/apache/commons-crypto/commit/dc28a2b7be5cbe21403f29db99635f8887b45a61> > ------------------------------ > > In .gitignore > <https://github.com/apache/commons-crypto/pull/92#discussion_r244810053>: > > > @@ -1,37 +0,0 @@ > -*~ > > Why delete this file? > ------------------------------ > > In Makefile.common > <https://github.com/apache/commons-crypto/pull/92#discussion_r244810433>: > > > Mac-x86_64_STRIP := strip -x > -Mac-x86_64_CFLAGS := -Ilib/inc_mac -I$(JAVA_HOME)/include -O2 -fPIC -mmacosx-version-min=10.5 -fvisibility=hidden -I/usr/local/include -I/usr/local/opt/openssl/include > -Mac-x86_64_CXXFLAGS := -Ilib/inc_mac -I$(JAVA_HOME)/include -O2 -fPIC -mmacosx-version-min=10.5 -fvisibility=hidden -I/usr/local/include -I/usr/local/opt/openssl/include > -Mac-x86_64_LINKFLAGS := -dynamiclib -L/usr/local/lib > +Mac-x86_64_CFLAGS := -Ilib/inc_mac -I$(JAVA_HOME)/include -O2 -fPIC -mmacosx-version-min=10.7 -fvisibility=hidden -I/usr/local/include -I/usr/local/opt/openssl/include > +Mac-x86_64_CXXFLAGS := -Ilib/inc_mac -I$(JAVA_HOME)/include -O2 -fPIC -mmacosx-version-min=10.7 -fvisibility=hidden -I/usr/local/include -I/usr/local/opt/openssl/include > +Mac-x86_64_LINKFLAGS := -dynamiclib -L/usr/local/lib > > nit: trailing whitespace > ------------------------------ > > In pom.xml > <https://github.com/apache/commons-crypto/pull/92#discussion_r244810551>: > > > - <modelVersion>4.0.0</modelVersion> > - > - <parent> > - <groupId>org.apache.commons</groupId> > - <artifactId>commons-parent</artifactId> > - <version>43</version> > - </parent> > - > - <groupId>org.apache.commons</groupId> > - <artifactId>commons-crypto</artifactId> > - <version>1.1.0-SNAPSHOT</version> > - <packaging>jar</packaging> > - > - <name>Apache Commons Crypto</name> > - <description> > +<!-- Licensed under the Apache License, Version 2.0 (the "License"); you > > This seems to be only formatting changes? Please undo them. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/OpenSslInfoNative.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244810727>: > > > @@ -18,13 +18,16 @@ > package org.apache.commons.crypto; > > /** > - * JNI interface of {@see CryptoRandom} implementation for OpenSSL. > + * JNI interface of @see CryptoRandom implementation for OpenSSL. > > Since you're touching this, it should be {@link CryptoRandom}. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/OpenSslInfoNative.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244810868>: > > > * The native method in this class is defined in > * OpenSslCryptoRandomNative.h (generated at build time by javah) > * and implemented in the file > * src/main/native/org/apache/commons/crypto/random/OpenSslCryptoRandomNative.c > */ > -class OpenSslInfoNative { > +public class OpenSslInfoNative { > > Why public? Users of the library should not need to care about this. > > It seems you're just calling this from Crypto.java so package private > should be fine. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl102NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244811499>: > > > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +package org.apache.commons.crypto.jna; > + > +import java.nio.ByteBuffer; > + > +import com.sun.jna.Native; > +import com.sun.jna.NativeLong; > +import com.sun.jna.ptr.PointerByReference; > + > +/** > + * @author alex > > Please remove @author tags. If not adding a proper javadoc, please omit > the whole thing. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl102NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244812371>: > > > + * limitations under the License. > + */ > + > +package org.apache.commons.crypto.jna; > + > +import java.nio.ByteBuffer; > + > +import com.sun.jna.Native; > +import com.sun.jna.NativeLong; > +import com.sun.jna.ptr.PointerByReference; > + > +/** > + * @author alex > + * > + */ > +public class OpenSsl102NativeJna { > > This class (and the 1.1 version) shouldn't be public. > > There also seems to be a lot of duplication between these classes. My gut > tells me there should be an abstract base class with the common stuff and > small specializations for each version. But this should be fine for now > since it's not that much code anyway, and to be fair I'm not familiar with > JNA and whether that would be ok. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl102NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244812522>: > > > + try { > + Native.register("crypto"); > + ok = true; > + } catch (Exception e) { > + thrown = e; > + } catch (UnsatisfiedLinkError e) { > + thrown = e; > + } finally { > + INIT_OK = ok; > + INIT_ERROR = thrown; > + } > + } > + > + /** > + * @return OPENSSL_VERSION_NUMBER which is a numeric release version > + * * identifier > > extra * > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl102NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244812584>: > > > + * OpenSSL library or giving information about the library build. > + */ > + public static native String SSLeay_version(int type); > + > + /** > + * Registers the error strings for all libcrypto functions. > + */ > + public static native void ERR_load_crypto_strings(); > + > + /** > + * @return the earliest error code from the thread's error queue without > + * modifying it. > + */ > + public static native NativeLong ERR_peek_error(); > + > + > > nit: too many blank lines > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl102NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244812689>: > > > + * modifying it. > + */ > + public static native NativeLong ERR_peek_error(); > + > + > + > + /** > + * Generates a human-readable string representing the error code e. > + * @see <a>https://www.openssl.org/docs/manmaster/crypto/ERR_error_string.html</a> > + * > + * @param err the error code > + * @param null_ buf is NULL, the error string is placed in a static buffer > + * @return the human-readable error messages. > + */ > + public static native String ERR_error_string(NativeLong err, char[] null_); > + //String ERR_lib_error_string(NativeLong err); > > ? > > (Seems copy & pasted from existing code, but better not to replicate > things that don't make sense.) > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl110NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244816990>: > > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +package org.apache.commons.crypto.jna; > + > +import java.nio.ByteBuffer; > + > +import com.sun.jna.Native; > +import com.sun.jna.NativeLong; > +import com.sun.jna.ptr.PointerByReference; > + > +public class OpenSsl110NativeJna { > + > + static final int OPENSSL_INIT_ENGINE_RDRAND = 0x00000200; > > Not used. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl110NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817093>: > > > + * limitations under the License. > + */ > + > +package org.apache.commons.crypto.jna; > + > +import java.nio.ByteBuffer; > + > +import com.sun.jna.Native; > +import com.sun.jna.NativeLong; > +import com.sun.jna.ptr.PointerByReference; > + > +public class OpenSsl110NativeJna { > + > + static final int OPENSSL_INIT_ENGINE_RDRAND = 0x00000200; > + > + static final int OOSL_JNA_ENCRYPT_MODE = 1; > > Not used. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSsl110NativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817116>: > > > + */ > + > +package org.apache.commons.crypto.jna; > + > +import java.nio.ByteBuffer; > + > +import com.sun.jna.Native; > +import com.sun.jna.NativeLong; > +import com.sun.jna.ptr.PointerByReference; > + > +public class OpenSsl110NativeJna { > + > + static final int OPENSSL_INIT_ENGINE_RDRAND = 0x00000200; > + > + static final int OOSL_JNA_ENCRYPT_MODE = 1; > + static final int OOSL_JNA_DECRYPT_MODE = 0; > > Not used. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817336>: > > > @@ -53,4 +53,14 @@ public static Throwable initialisationError() { > return OpenSslNativeJna.INIT_ERROR; > } > > + /** > + * Retrieves version/build information about OpenSSL library. > + * > + * @param type type can be OPENSSL_VERSION, OPENSSL_CFLAGS, OPENSSL_BUILT_ON... > + * @return A pointer to a constant string describing the version of the > + * OpenSSL library or giving information about the library build. > + */ > + public static String OpenSSLVersion(int type) { > > I'd rather not expose more public methods unless there's a real need for > them. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCipher.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817423>: > > > @@ -317,7 +317,7 @@ private void throwOnError(int retVal) { > String errdesc = OpenSslNativeJna.ERR_error_string(err, null); > > if (context != null) { > - OpenSslNativeJna.EVP_CIPHER_CTX_cleanup(context); > + OpenSslNativeJna.EVP_CIPHER_CTX_cleanup(context); > > Undo. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817494>: > > > @@ -100,14 +100,14 @@ public void nextBytes(byte[] bytes) { > > synchronized (OpenSslJnaCryptoRandom.class) { > //this method is synchronized for now > - //to support multithreading https://wiki.openssl.org/index.php/Manual:Threads(3) needs to be done > + //to support multithreading https://wiki.openssl.org/index.php/Manual:Threads(3) needs to be done > > Undo. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817688>: > > > > if(rdrandEnabled && OpenSslNativeJna.RAND_get_rand_method().equals(OpenSslNativeJna.RAND_SSLeay())) { > - close(); > + //close(); > > Why? If it's not needed, then just remove it. Otherwise it should be > called. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslJnaCryptoRandom.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817779>: > > > throw new RuntimeException("rdrand should be used but default is detected"); > } > - > - ByteBuffer buf = ByteBuffer.allocateDirect(bytes.length); > + > + ByteBuffer buf = ByteBuffer.allocateDirect(bytes.length); > > Undo. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslNativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244817987>: > > > - static { > - boolean ok = false; > - Throwable thrown = null; > - try { > - Native.register("crypto"); > - ERR_load_crypto_strings(); > - ok = true; > - } catch (Exception e) { > - thrown = e; > - } catch (UnsatisfiedLinkError e) { > - thrown = e; > - } finally { > - INIT_OK = ok; > - INIT_ERROR = thrown; > - } > + static final int OPENSSL_INIT_ENGINE_RDRAND = 0x00000200; > > You're re-indenting this whole file. Please don't do that, both to follow > the existing style, and to make it easy to see what's changing. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslNativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244818064>: > > > + static final int OOSL_JNA_DECRYPT_MODE = 0; > + > + static final boolean INIT_OK; > + > + static final Throwable INIT_ERROR; > + > + public static final long VERSION; > + public static final long VERSION_1_0_X = 0x10000000; > + public static final long VERSION_1_1_X = 0x10100000; > + > + static { > + NativeLibrary crypto = NativeLibrary.getInstance("crypto"); > + Function version = null; > + try { > + version = crypto.getFunction("SSLeay"); > + } catch (UnsatisfiedLinkError e) { > > Indentation is off from here on. > ------------------------------ > > In src/main/java/org/apache/commons/crypto/jna/OpenSslNativeJna.java > <https://github.com/apache/commons-crypto/pull/92#discussion_r244818314>: > > > - > - /** > - * Initializes the engine. > - */ > - public static native void ENGINE_load_rdrand(); > - > - //TODO callback multithreading > - /*public interface Id_function_cb extends Callback { > - long invoke (); > + public static PointerByReference ENGINE_by_id(String string) { > + if (VERSION == VERSION_1_1_X) { > + return OpenSsl110NativeJna.ENGINE_by_id(string); > + } else if (VERSION == VERSION_1_0_X) { > + return OpenSsl102NativeJna.ENGINE_by_id(string); > + } else { > + return null; > > This shouldn't really be reachable, right? So maybe throw an exception > instead? (Also in other methods.) > > The interface-based approach I suggested would make these cleaner, but > then I'm not sure how well it plays with JNA. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/OpenSslInfoNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r244818921>: > > > @@ -39,38 +39,24 @@ > #include "OpenSslInfoNative.h" > > #ifdef UNIX > -static unsigned long (*dlsym_SSLeay) (void); > -static char * (*dlsym_SSLeay_version) (int); > +static unsigned long (*dlsym_OpenSSL_version_num) (void); > +static char * (*dlsym_OpenSSL_version) (int); > > nit: this function returns const char *. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/OpenSslInfoNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r244819457>: > > > { > - if (!load_library(env)) { > - return NULL; > - } > - > - jstring answer = (*env)->NewStringUTF(env,dlsym_SSLeay_version(type)); > - return answer; > -} > +#ifdef UNIX > + dlerror(); // Clear any existing error > +#if OPENSSL_VERSION_NUMBER > VERSION_1_1_x > > This prevents the JNI library from working against different versions of > OpenSSL right? This should be a runtime check, not a compile-time check. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/OpenSslInfoNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r244819798>: > > > { > - if (!load_library(env)) { > - return NULL; > - } > - > - jstring answer = (*env)->NewStringUTF(env,dlsym_SSLeay_version(type)); > - return answer; > -} > +#ifdef UNIX > + dlerror(); // Clear any existing error > +#if OPENSSL_VERSION_NUMBER > VERSION_1_1_x > + LOAD_DYNAMIC_SYMBOL(dlsym_OpenSSL_version_num, env, openssl, "OpenSSL_version_num"); > > You'lll probably need to use do_dlsym directly instead of the macro, so > you can do runtime checks. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/OpenSslInfoNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r244819839>: > > > > -JNIEXPORT jlong JNICALL Java_org_apache_commons_crypto_OpenSslInfoNative_SSLeay > - (JNIEnv *env, jobject object) > -{ > - if (!load_library(env)) { > - return 0; > - } > - return dlsym_SSLeay(); > +#ifdef WINDOWS > > ? > ------------------------------ > > In src/main/native/org/apache/commons/crypto/cipher/OpenSslNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r244820275>: > > > @@ -30,9 +31,20 @@ > #include "OpenSslNative.h" > > #ifdef UNIX > +#if OPENSSL_VERSION_NUMBER > VERSION_1_1_x > > Same thing about runtime vs. compile-time checks. > ------------------------------ > > In src/main/native/org/apache/commons/crypto/cipher/OpenSslNative.c > <https://github.com/apache/commons-crypto/pull/92#discussion_r244820498>: > > > @@ -716,3 +692,104 @@ JNIEXPORT void JNICALL Java_org_apache_commons_crypto_cipher_OpenSslNative_clean > EVP_CTX_Wrapper *wrapper = CTX_WRAPPER(ctx); > free_context_wrapper(wrapper); > } > + > +#if OPENSSL_VERSION_NUMBER > VERSION_1_1_x > > Again. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/commons-crypto/pull/92#pullrequestreview-188772523>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGg8fiQ0zkAr7mXHu4Bl-f_FEI5XFDXSks5u_QHSgaJpZM4ZgACn> > . >
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
