More srtp factory improvements

[JonathanLennox]

No description provided.

[ibauersachs]

Line 892/893 will probably trigger a reflection warning on Java 11+ and can be moved to the Java 8 code block.

[ibauersachs]

Why not move this inside the null-check?

[JonathanLennox]

Because I don't need/want to call Provider.getPrototype() if getMethod("configure") throws, as it will in Java 1.8.

[ibauersachs]

The entire finally block could just be the else-part of the provier-null check above and the inner try/catch made redundant.

[JonathanLennox]

I'm not sure I follow? This is executed if getMethod("configure") throws, as it will in Java 1.8.

[JonathanLennox]

Line 892/893 will probably trigger a reflection warning on Java 11+ and can be moved to the Java 8 code block.

This is a public method of a public class, which as I understand it won't trigger reflection warnings. (I don't see any warnings, myself, when I use Java 11 or 14.). Basically, if it works without needing to call setAccessible, it's fine.

The purpose of it is to let us use the Java 9+ API but still build with the Java 8 JDK, so it needs to be in the Java 9+ code block.

[ibauersachs]

Sorry, the inline comments are confusing. Wouldn't this work?

        public static synchronized Provider getProvider()
            throws Exception
        {
            Provider provider = SunPKCS11CipherFactory.provider;
            if (provider == null && useProvider)
            {
                try
                {
                    Provider prototype = Security.getProvider("SunPKCS11");
                    if (prototype != null && !System.getProperty("java.version").startsWith("1.8"))
                    {
                        // Java 9+. PKCS11 configuration is set using the configure method.
                        // Use reflection so we can build with JDK 8.
                        Method configureMethod = Provider.class.getMethod("configure", String.class);
                        provider = (Provider)configureMethod.invoke(prototype, getConfiguration());
                    }
                    else
                    {
                        Class<?> clazz = Class.forName("sun.security.pkcs11.SunPKCS11");
                        if (Provider.class.isAssignableFrom(clazz))
                        {
                            // Java 8. PKCS11 configuration is passed as a constructor parameter.
                            Constructor<?> contructor = clazz.getConstructor(String.class);
                            provider = (Provider) contructor.newInstance(getConfiguration());
                        }
                }
                catch (Exception e)
                {
                    logger.debug("Unable to construct PKCS11 provider: " + e.getMessage());
                }
                finally
                {
                    if (provider == null)
                        useProvider = false;
                    else
                        SunPKCS11CipherFactory.provider = provider;
                }
            }
            return provider;
        }

[JonathanLennox]

Sorry, the inline comments are confusing. Wouldn't this work?

No, I'm pretty sure Security.getProvider("SunPKCS11") can succeed on Java 8, if that provider happens to be configured in java.security, but getMethod("configure") will throw.

(This isn't the default setup, to be sure.)

[ibauersachs]

Sorry, the inline comments are confusing. Wouldn't this work?

No, I'm pretty sure Security.getProvider("SunPKCS11") can succeed on Java 8, if that provider happens to be configured in java.security, but getMethod("configure") will throw.

(This isn't the default setup, to be sure.)

Okay, updated to include a version check.

[JonathanLennox]

Okay, updated to include a version check.

I hate doing version checks if there's any way the code can directly determine whether a feature is available - what's your objection to the existing logic?

(I did a slight rearrangement to avoid getting the class object until it's needed.)

[ibauersachs]

Okay, updated to include a version check.

I hate doing version checks if there's any way the code can directly determine whether a feature is available - what's your objection to the existing logic?

The nested try's are confusing to read. Using exceptions as an expected outcome is bad code style and using finally as an else block is abuse. But that's just my opinion and I agree that version checks aren't a good thing either. The multi-release jars are supposed to solve things like this, but they have their own drawbacks.

(I did a slight rearrangement to avoid getting the class object until it's needed.)

I'm not insisting on the changes.

[JonathanLennox]

Note I pulled out several parts of this PR into #28 ; what remains is less critical.