fix: restore OpenSAML class loading and make SAML parsing engine-portable - #19
Conversation
6b06a20 to
c338553
Compare
| ); | ||
| // Signature verification pulls in OpenSAML's crypto providers, discovered through the same | ||
| // ServiceLoader mechanism as initialisation, so it needs the same classloader context. | ||
| runWithClassLoader( function(){ |
There was a problem hiding this comment.
@dougcain why does this code get called a second time after initializeOpenSAMLLib(); is already used earlier in the function? Why not just call initializeOpenSAMLLib(); again if it is necessary?
There was a problem hiding this comment.
Good catch — you're right that it's redundant, and it's removed in c976525.
For the record on where it came from: it predates both #16 and #17. It arrived with 7cf0359 ("Verify SAML responses using OpenSAML") and was moved around by c8a56b0 / 86f2021; #18 never touched this file. My PR only wrapped what was already there, which is why it looked deliberate.
It really is a no-op, from the jar rather than from reading the CFML:
public static synchronized void initOpenSAML()
0: getstatic Field initialized:Z
3: ifeq 7
6: return
Static, synchronized, guarded by a static flag. Worth noting your suggested alternative wouldn't have done anything either — initializeOpenSAMLLib() early-returns on !isNull( variables.AuthNRequestGenerator ), and even past that guard initOpenSAML() short-circuits.
One thing the redundant call was quietly doing, though. The old ordering assigned variables.AuthNRequestGenerator before calling initOpenSAML(). So if initialisation threw, the guard at the top of initializeOpenSAMLLib() would short-circuit from then on and the provider could never initialise again for the life of the application — the second call was providing the retry. So rather than just deleting it, initialisation now publishes the objects only once initOpenSAML() has returned. Same effect, without depending on a duplicate call to recover.
The runWithClassLoader around parseAndValidate stays, but my justification for it was wrong and I've corrected the comment. I'd written that it was needed because parseAndValidate reads the provider registry through XMLObjectProviderRegistrySupport — but getRawSAMLRequest() goes through that same registry unwrapped and works fine, which disproves it. The real reason is narrower: verifySignature → SignatureValidator.validate resolves crypto providers through the thread context classloader, which on BoxLang isn't the one the OpenSAML classes were loaded from. The comment now says that, and warns against both wrong conclusions (wrap everything / remove it).
Two other things came out of chasing this, both in the same commit:
cacheCerts()is now reachable with an unsetfederationMetadataURL, since initialisation is lazy and no longer runs only from the setter. It throws a namedMicrosoftSAMLProvider.MissingConfigurationinstead of failing against an empty string on a user's first sign-in.- Changelog updated for that.
Re-verified end to end on BoxLang 1.14.0+55 / ColdBox 7 against Entra after a cold boot: registration does no jar loading or IdP fetch, sign-in redirects with a valid signed SAMLRequest, ACS authenticates. Worth flagging that CI can't cover any of this — the repo has no lib/, so the SAML provider path never executes there.
7447aae to
1b8c940
Compare
…ication boot Rebase of coldbox-modules#19 onto development. Two of the original five fixes are dropped as superseded: SAMLParsingService now matches on local-name() throughout (c305a29, 5dec7f6) and the extractErrorMessage/xmlData path was restructured out of existence by the extractStatus and extractIdentity split (e4f2dc9), so both hunks no longer apply. The classloading, laziness, separate certificate readiness, named missing-config error and per-provider isolation all still apply to development HEAD unchanged. Rebased over d1b9d25, which added the SAML request replay cache: onLoad() now calls ensureSAMLRequestCache() before appendPaths(), and parseAndValidateAssertion() takes the InResponseTo binding, so the classloader wrap moved onto the five-argument overload.
1b8c940 to
62e9f9c
Compare
…ication boot Rebase of coldbox-modules#19 onto development. Two of the original five fixes are dropped as superseded: SAMLParsingService now matches on local-name() throughout (c305a29, 5dec7f6) and the extractErrorMessage/xmlData path was restructured out of existence by the extractStatus and extractIdentity split (e4f2dc9), so both hunks no longer apply. The classloading, laziness, separate certificate readiness, named missing-config error and per-provider isolation all still apply to development HEAD unchanged. Rebased over d1b9d25, which added the SAML request replay cache: onLoad() now calls ensureSAMLRequestCache() before appendPaths(), and parseAndValidateAssertion() takes the InResponseTo binding, so the classloader wrap moved onto the five-argument overload.
62e9f9c to
3f13aab
Compare
|
One finding from the same integration work, recorded here because it bears on how this jar gets loaded rather than on the code in this PR. The shaded jar bundles Santuario unrelocated, so Measured rather than assumed: OpenSAML 4.0.1 runs correctly with a host-supplied xmlsec 4.0.4 ahead of the shaded copy — Relocating is the obvious fix and it is not safe as-is. Shadow rewrites bytecode but not class names held as text, and Santuario's own Relevant to this PR because restoring cbjavaloader gives the jar its own I opened a Santuario bump for this and closed it again: the CVE I cited (2021-40690) turns out not to be reachable, since |
Follow-up to #18. Tested against BoxLang 1.14.0+55 / ColdBox 7,
MicrosoftSAMLProvideragainst Microsoft Entra.On
3.0.0-snapshotas published, the consuming application could not boot at all. With these five changes a full SSO round trip completes — sign-in button → Entra → ACS → authenticated session.Fixes #16 and #17 are confirmed working, and
init()-seeding every property is a better fix than the per-getter defaults we had been carrying locally. Thank you for both.1. The bundled jar is on no classpath — blocker
initializeOpenSAMLLib()resolves withcreateObject( "java", "cbsso.opensaml.AuthNRequestGenerator" ), which searches the server classpath. Butcbjavaloaderwas dropped fromthis.dependenciesandonLoad()no longer appends this module's/lib, so nothing ever puts the jar anywhere reachable.box.jsonstill installs cbjavaloader, which is what makes this easy to miss.Restored:
cbjavaloaderinthis.dependencies,appendPaths( modulePath & "/lib" )inonLoad(), and resolution via thejavaloader:DSL so the lookup uses the loader those paths were actually added to.Why not
this.javaSettingsinstead? It cannot work from a module. It is anApplication.cfcsetting read before any module registers, and ColdBox does not merge a module's copy — there is no reference tojavaSettingsanywhere incoldbox/system, and no installed module declares it. So "native" would mean every consuming application adding cbsso's own lib path to itsApplication.cfc, with a path that varies by install location.cbmarkdownsolves the same problem the same way this PR does. There is also a real argument for keeping a 17MB OpenSAML/Santuario/Xerces/Guava bundle off the host application's classpath, where it can collide with the host's own versions.2. Boot did the work at all — and one bad provider took the app with it
Every provider setter runs inside
registerProviders(), insideonLoad(). SosetFederationMetadataURL()callinginitializeOpenSAMLLib()+cacheCerts()meant application boot loaded a 17MB jar and made an outbound HTTPS call to the IdP, per configured provider — with notry/catchanywhere in the chain. That is what escalated (1) from "SSO is broken" into "no request is served for any tenant".Two changes, independent of each other and of (1):
setFederationMetadataURL()stores the URL and nothing else; initialisation happens on first use, as it did in 1.0.7.registerProviders()isolates each definition, logs a failure, and leaves that provider unregistered — wheremissing()already reports it andAuthredirects toerrorRedirect.Worth calling out as a design question rather than a fix: this trades fail-fast for availability. In a multi-tenant app one tenant's unreachable IdP should not deny service to the others. Happy to drop this half if you would rather keep fail-fast.
3. Prefixed XPath does not resolve on BoxLang
SAMLParsingService.extractUserInfo()strips only the default namespace declaration, soxmlns:samlpsurvives on the document — which is exactly why the unprefixed//Attribute[…]queries work. But BoxLang'sxmlSearchwill not resolve//samlp:StatusCodeagainst a prefix declared in the document.So
detectSuccess()returnedfalsefor a valid, signed, successful Entra assertion, and every login failed closed with no usable error. Both prefixed queries now match onlocal-name(), which behaves identically on every engine.4. The invalid-response path never ran — engine-independent
processAuthorizationEvent()'s innercatchcalledextractErrorMessage( xmlData ), butxmlDataceased to exist when parsing moved toSAMLParsingService(it issamlDatanow). So the only path handling a failed signature or issuer check threw on an undefined variable, was swallowed by the outercatch, and reported that second failure instead of the validation failure. The statement was also missing its;.It now prefers the IdP's own status message where there is one and falls back to the validation error. The private
extractErrorMessage()is deleted: unreachable, declaredbooleanwhile returning a string, and a duplicate ofSAMLParsingService.extractErrorMessage().This is the path that matters for a malformed or hostile response, so it is worth a test.
5. BoxLang needs the thread context classloader for SPI discovery
OpenSAML's
InitializationServicediscovers providers throughServiceLoader, which reads the thread context classloader rather than the one the classes were loaded from. On BoxLang that is not cbjavaloader'sURLClassLoader, so discovery finds nothing. Set around initialisation and validation, restored in afinallyso a failure cannot leak the wrong loader into the request thread. Adobe ColdFusion resolves it unaided and skips the swap entirely.This is a consequence of the classloader isolation in (1), and confirmed still necessary against 3.0.0's rebuilt jar.
Verification
SAMLRequest, ACS authenticates, session established.GETof the ACS URL returns a redirect rather than a 500.box cfformat checkclean on all changed files.Item 4 is correct by inspection but not exercised at runtime — a successful login never enters that catch, and I did not force a signature failure. Flagging that rather than implying otherwise.
One note for the release, not fixed here
The
getRawResponseData()return-type change and the move of the provider guard into apreHandlerboth alter contracts that consumer specs pin — invoking an action directly no longer runs the guard, and the actions have no guard of their own. Both are reasonable changes; they just warrant a line in the release notes so consumers know to update.