Evidence of exception driven control flow in bouncy castle TLS connections

[bsmith-tridium-com]

See write up below by one of our engineers:

Using Yourkit, I can demonstrate an excessive amount of exceptions are created surrounding the normal function of TLS connections between Niagara Workbench and the Niagara Daemon.

See the following top offenders in Workbench: See the following top offenders in Niagara Daemon: This the output of a functioning TLS connection(s) between Niagara Workbench and Niagara Daemon with 5 instances of the Platform Administration View open to the local Niagara Daemon.

The counts associated with:

java.security.spec.InvalidParameterSpecException
java.lang.IllegalStateException
org.bouncycastle.crypto.InvalidCipherTextException
javax.crypto.AEADBadTagException 

Will increase indefinitely as long as the connections remain open.

Exceptions are expensive

Control flow using exceptions is an anti-pattern

We should see if the exception generation can be reduced as much as possible to encourage performance of TLS connections.

See a possible solution to submit to BC team to sanity test the first character of a string as a valid IPv4 / IPv6 character before falling back to the exception driven design:

D:\bc-java>git diff
diff --git a/core/src/main/java/org/bouncycastle/util/IPAddress.java b/core/src/main/java/org/bouncycastle/util/IPAddress.java
index 8f57263f4..9877ff295 100644
--- a/core/src/main/java/org/bouncycastle/util/IPAddress.java
+++ b/core/src/main/java/org/bouncycastle/util/IPAddress.java
@@ -45,6 +45,10 @@ public static boolean isValidIPv4(
         {
             return false;
         }
+        if (Character.digit((int)address.charAt(0), 10) < 0)
+        {
+            return false;
+        }
         int octets = 0;
@@ -127,6 +131,11 @@ public static boolean isValidIPv6(
         {
             return false;
         }
+        char firstChar = address.charAt(0);
+        if (Character.digit(firstChar != ':' && (int)firstChar, 16) < 0)
+        {
+            return false;
+        }
         int octets = 0; 

Further evidence of exceptions in 1.70 BC provider with TLS connection to the Niagara Daemon:

[peterdettman]

Thanks, I've now overhauled the IPAddress class to remove all Integer.parseInt calls. Expect a new beta release to test in the next few days.

[bsmith-tridium-com]

Thx Peter!

[peterdettman]

New beta is up at https://downloads.bouncycastle.org/betas/ .

[peterdettman]

@bsmith-tridium-com Were you able to test the beta and compare to your earlier results?

[bsmith-tridium-com]

@peterdettman, Yes, This seems to be better. One additional observation:

"Yet the others remain. This is progress; however, exceptions like NoSuchAlgorithmException invite the question - if we don't support an algorithm, and we have no reason to suspect that algorithm availability is subject to change during the lifetime of a web server - why must we keep asking the question? Why must a CertificateException be generated for every load? If the exception is because of a name alias not properly formatted or matching, surely that can be implemented with a simple boolean check. I would love to work with BC team, attaching YourKit to a simple TLS load test, and track these down."