Internal ParserPool built by Decrypter

Description

The Decrypter class appears to install a custom-build BasicParserPool for decrypting XML fragments. It doesn't appear to be broken, but we really don't want multiple pools getting built with potentially conflicting settings. We need a way to inject one from outside so the one we use for everything else will get used.

Environment

None

Activity

Brent PutmanSeptember 11, 2018 at 1:09 AM

Fixed as described.

OpenSAML: f7ce2de868f75f9b2ac753b3a48d123e7566a923

IdP: 86fe2762ab3400a58e01a4dd84d2357eddf589d5

Brent PutmanSeptember 7, 2018 at 1:39 AM

Agree that it's worth understanding why we need that Xerces option. I think I knew the details in 2006, but there's not much in the comments etc, so that might take awhile. It did seem like it was a known issue or bug that apparently still hasn't been fixed. So not for 3.4.0.

Literally injecting a ParserPool from outside in the classic sense is a bit of a problem. This is typically an instance-per-use class and it doesn't have the setter+initialize() model. Mandatory params are ctor args, and we can't really add a new mandatory param in new ctors. Even adding a ParserPool to the DecryptionParameters wouldn't really work, b/c for historical reasons there are ctor variants that take the individual items rather than that single params instance. In fact the abstract Decrypter profile action uses one of those ctors instead of the DecryptionParameters one.

What I really wanted to do was just have it internally pull the global ParsePool. So probably the best option for now is to just init and store a Decrypter-specific ParserPool bean in the global configuration and then have the Decrypter pull that internally like other components do. Then we can simply update the IdP OpenSAMLConfigBean to optionally take that special bean and configure it if supplied, as it does now for the main ParserPool. That seems pretty easy and safe to do.

Scott CantorSeptember 6, 2018 at 1:13 PM

That seems like a strange option to need turned off and worth understanding. But for the time being, a compromise would be to at least allow us to control the settings by injecting a second ParserPool from outside. The real issue isn't so much having two but having one we can't control from a Spring bean.

Brent PutmanSeptember 6, 2018 at 4:38 AM

Had to really turn on the Way Back machine on this one...

I initially thought we could just switch this to use the global parser pool from the global config.  But turns out that there is an important delta between our "standard" pool config and the internal one here, which I "enjoyed" relearning from over a decade ago.

The Decrypter internal one sets the JAXP feature http://apache.org/xml/features/dom/defer-node-expansion to false. Ordinarily it defaults to true. Xerces docs are here:

https://xerces.apache.org/xerces-j/features.html#defer-node-expansion

(Despite what it says there, it does seem to matter even though we don't change {{http://apache.org/xml/properties/dom/document-class-name}} from the defaults.  Maybe the docs are just misworded).

I do vaguely remember this but not the details.  This feature has been set like this in the decrypter's ParserPool instance since the initial commit of this code in Dec 2006.

I thought/hoped that maybe whatever the parser issue was had gone away with the JRE's new internal JAXP parser, but if I remove that feature, one of the unit tests fails in my Eclipse environment:

org.opensaml.xmlsec.encryption.support.DecryptionSignedContentTest#testDecryptAndVerifySignedElement()

Tested it under both Java 7 and 8.

So, I don't know if we can get easily rid of that at the moment. What we had been doing is just keeping the security-related changes synced up here.

The alternative would be to add this feature to the global ParserPool, but that seems like a very big and invasive change to make to the whole OpenSAML and IdP environment.  It's possible it might have a negative impact on memory usage.  Certainly will affect how we process all XML.

Fixed

Details

Assignee

Reporter

Components

Fix versions

Created April 16, 2018 at 2:01 PM
Updated September 11, 2018 at 1:15 AM
Resolved September 11, 2018 at 1:15 AM

Flag notifications