Clean up API for BaseXXSupport decoders
Description
Environment
Activity
Philip SmartFebruary 24, 2020 at 11:54 AM
CryptoBinary fix in idp ff082d43fffee59adcba9f911062d85c45511aec
CryptoBinary fix in opensaml cb490dbeca816888464fcbdf2050612cba9b9130
Scott CantorFebruary 21, 2020 at 4:05 PM
Yeah, I'm just not a fan of runtime exceptions on those kinds of errors. I know people find catching exceptions a hassle, but all it's doing is literally saying this is going to throw whether you like it or not so you might as well plan for it.
Philip SmartFebruary 21, 2020 at 4:00 PM
Yes sorry, the setValueBigInt is on the interface CryptoBinary and implemented in CryptoBinaryImpl
Scott CantorFebruary 21, 2020 at 3:59 PM
Is that setter on CryptoBinaryImpl or CryptoBinary?
I wouldn't think KeyInfoSupport would be called an impl method, though it could.
Philip SmartFebruary 21, 2020 at 3:56 PMEdited
CryptoBinary
Deferring encoding until marshalling (looks to be `XSBase64BinaryMarshaller`) seemed like a good idea to me, but I can not see a way of doing this unless the base64 encoding step was lazy and attached to an overriden CryptoBinaryImpl#getValue method from XSBase64Binary. This would then require modifying the signature of XSBase64BinaryImpl#getValue() to throw an Encoding exception - or it could return null if it could not base64 encode (not sure we gain much from that). Throwing an EncodingException effected lots of call sites, lots of which were for subtypes that did not require on the fly base64 encoding.
So, unless I am missing something obvious. I added it (not committed yet) to the signature for `setValueBigInt` which is specific to `CryptoBinaryImpl`, narrowing the scope of the change, and only has 6 call sites in 2 methods all in `KeyInfoSupport`, namely; `buildDSAKeyValue` and `buildRSAKeyValue`. If all these sites rethrow the same expection, they can be eventually caught by `BasicKeyInfoGeneratorFactory#processPublicKey` and turned into a `SecurityException`, which seems OK.
The commons-coded BaseXX classes we wrap in our BaseXXSupport classes have changed in recent releases to start throwing IllegalArgumentException on certain invalid input strings.
See:
https://issues.apache.org/jira/browse/CODEC-134
https://issues.apache.org/jira/browse/CODEC-270
The IdP test failure triggered by the first of those (in commons-codec 1.13) was addressed by making the sealer code more robust in java-support commit 2c34093672b57c8726ffbacc7a07f58701895595.
The second (in commons-codec 1.14) triggers another IdP test failure. Reviewing our code here, Scott and I are of the opinion that we could improve the BaseXXSupport decoder APIs.
Specifically, throwing a new checked exception analogous to commons-codec's DecoderException would be an improvement over the current situation, as it would force us to catch it and handle it close to each call site; we're clearly not successfully doing this with IllegalArgumentException just now.
This would require catching IllegalArgumentException on each call to the underlying classes, and rethrowing. It's probably also worth looking at our classes' use of an (again, unchecked) ConstraintViolationException when the underlying classes return null. It's not clear that's the right thing to be doing in general.
Doing something like this would obviously require reviewing every call site. In some cases it will be fairly straightforward to address the new exception because there's already a try block around.
I've marked this as a blocker for 8.0.0 and therefore IdP 4.0.0 because unless sorted out one way or another before then we'll be stuck on commons-codec 1.13 and unable to upgrade to 1.14.