diff --git a/ModuleConfig.cfc b/ModuleConfig.cfc index 8ed3769..a22467c 100644 --- a/ModuleConfig.cfc +++ b/ModuleConfig.cfc @@ -20,7 +20,12 @@ component { this.entryPoint = "/cbsso"; // Dependencies - this.dependencies = [ "hyper", "jwtcfml" ]; + // cbjavaloader is load-order critical, not merely installed: onLoad() hands it this module's /lib so the + // bundled OpenSAML jar becomes resolvable. A module cannot place that jar on the classpath itself - + // this.javaSettings is an Application.cfc setting, read before any module registers, and ColdBox does + // not merge a module's copy of it - so without cbjavaloader every consuming app would have to add + // cbsso's own lib path to its Application.cfc. + this.dependencies = [ "hyper", "jwtcfml", "cbjavaloader" ]; routes = [ { @@ -63,7 +68,15 @@ component { * Fired when the module is registered and activated. */ function onLoad(){ - // Register all app disks + // Must precede registerProviders(): a SAML provider resolves cbsso.opensaml.* out of this path. + // Guarded because /lib is a build artifact of the Gradle project in /java and is absent from a + // source checkout, where appendPaths would throw "Invalid library path". Skipping it there leaves a + // SAML provider to fail on first use with its own error rather than taking the application down. + var openSAMLLibPath = modulePath & "/lib"; + if ( directoryExists( openSAMLLibPath ) ) { + wirebox.getInstance( "loader@cbjavaloader" ).appendPaths( openSAMLLibPath ); + } + wirebox.getInstance( "ProviderService@cbsso" ).registerProviders(); if ( settings.enableCBAuthIntegration ) { diff --git a/changelog.md b/changelog.md index 4cae4fc..529bc00 100644 --- a/changelog.md +++ b/changelog.md @@ -11,6 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `MicrosoftSAMLProvider.setFederationMetadataURL()` no longer initialises OpenSAML or fetches the IdP's + metadata. Every provider setter runs inside `ProviderService.registerProviders()`, i.e. inside the + module's `onLoad()`, so doing that work there made application boot load a 17MB jar and make one + outbound HTTPS call per configured provider. Both now happen on first use. +- `MicrosoftSAMLProvider` throws `MicrosoftSAMLProvider.MissingConfiguration` when initialised without a + `federationMetadataURL`, rather than letting `cacheCerts()` fail against an empty string. Reachable now + that initialisation is lazy: it used to run only from the setter, which by definition had a value. +- `ProviderService.registerProviders()` isolates each definition, so one provider that cannot be built no + longer aborts the remaining providers, `onLoad()`, or the application boot it runs inside. The failure + is logged and that provider is left unregistered, where `missing()` reports it and `Auth` redirects to + `errorRedirect`. - **BREAKING** `SSOAuthorizationResponse.getName()` returned `FirstName` instead of `Name`, so the value written by `setName()` could never be read back - `GitHubProvider` sets only `Name`, making its display name unreachable. It now returns `Name`, falling back to `FirstName LastName` for providers that set @@ -21,6 +32,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- The bundled OpenSAML jar was not placed on any classpath, so `MicrosoftSAMLProvider` could not be + built at all: `createObject( "java", "cbsso.opensaml.AuthNRequestGenerator" )` searches the server + classpath, while `cbjavaloader` was no longer a module dependency and `onLoad()` no longer handed it + this module's `/lib`. `cbjavaloader` is restored to `this.dependencies`, `onLoad()` appends `/lib` + again, and the classes resolve through the `javaloader:` DSL, which is the loader those paths were + added to. A module cannot substitute `this.javaSettings` for this - it is an `Application.cfc` setting + read before any module registers, and ColdBox does not merge a module's copy - so the alternative would + be every consuming application adding cbsso's own lib path to its `Application.cfc`. +- OpenSAML initialisation and signature validation now run with the thread context classloader set to + cbjavaloader's, and restore the original afterwards. `InitializationService` discovers its providers + through `ServiceLoader`, which reads the thread context classloader rather than the one the classes were + loaded from, so on BoxLang discovery found nothing and initialisation failed. Adobe ColdFusion resolves + it without help and is unaffected. +- `initializeOpenSAMLLib()` guarded both the library initialisation and the certificate fetch on + `AuthNRequestGenerator` being set, and the generator is published before `cacheCerts()` runs. One + transient metadata failure - or an unreachable IdP on first sign-in - therefore satisfied the guard on + every later call, leaving a validator holding no certificates that never fetched them again for the life + of the application, so every subsequent login failed on a signature that could not be checked. The two + conditions are now tracked separately: the library initialises once, and the certificates are re-fetched + on each call until a fetch succeeds. `setFederationMetadataURL()` clears the flag, so re-registering a + provider against a different IdP refetches rather than validating against the old IdP's certificates. +- `SAMLParsingService` matched `StatusCode` and `StatusMessage` by the `samlp:` prefix. Because + `extractUserInfo()` strips only the default namespace declaration, and BoxLang's `xmlSearch` does not + resolve a prefixed XPath against a prefix declared in the document, `detectSuccess()` returned false for + a valid, signed, successful assertion - failing every login closed. Both now match on `local-name()`, + which behaves identically on every engine. +- `MicrosoftSAMLProvider.processAuthorizationEvent()` referenced an `xmlData` variable that no longer + exists when signature or issuer validation fails, so the one path handling an invalid response threw on + an undefined variable and reported that instead of the validation failure. It now reports the IdP's own + status message where there is one and the validation error otherwise. The private `extractErrorMessage()` + it called is removed: it was unreachable, declared `boolean` while returning a string, and duplicated + `SAMLParsingService.extractErrorMessage()`. - [#16](https://github.com/coldbox-modules/cbSSO/issues/16) An unregistered provider name threw a `KeyNotFoundException` from `ProviderService.get()` before the handler's `isNull()` guard could run, so `CBSSOMissingProvider` was never announced from `Auth.start()` or `Auth.authorize()`. The diff --git a/models/ProviderService.cfc b/models/ProviderService.cfc index 8d8251e..024876d 100644 --- a/models/ProviderService.cfc +++ b/models/ProviderService.cfc @@ -10,23 +10,37 @@ component accessors="true" singleton threadsafe { variables.providers = {}; + /** + * One provider failing to build no longer aborts the rest, or the module's onLoad(), or the application + * boot that onLoad() runs inside. A definition can fail for reasons wholly outside this module - + * an unreachable IdP, a missing jar, a provider type that is not installed - and losing every other + * provider plus the whole application to it is out of all proportion. The failure is logged and that + * provider is left unregistered, so missing() reports it and Auth redirects to errorRedirect. + */ ProviderService function registerProviders(){ variables.moduleSettings.providers.each( function( providerDefinition ){ - var provider = wirebox.getInstance( providerDefinition.type ); - - for ( var setting in providerDefinition ) { - if ( !structKeyExists( provider, "set#setting#" ) ) { - continue; + try { + var provider = wirebox.getInstance( providerDefinition.type ); + + for ( var setting in providerDefinition ) { + if ( !structKeyExists( provider, "set#setting#" ) ) { + continue; + } + + invoke( + provider, + "set#setting#", + [ providerDefinition[ setting ] ] + ); } - invoke( - provider, - "set#setting#", - [ providerDefinition[ setting ] ] + providers[ provider.getName() ] = provider; + } catch ( any e ) { + log.error( + "Could not register SSO provider [#providerDefinition.name ?: providerDefinition.type ?: "unnamed"#] - it will be unavailable until the next successful registration", + { "error" : e.message, "detail" : e.detail ?: "" } ); } - - providers[ provider.getName() ] = provider; } ); return this; } diff --git a/models/providers/MicrosoftSAMLProvider.cfc b/models/providers/MicrosoftSAMLProvider.cfc index 50009a2..19acb32 100644 --- a/models/providers/MicrosoftSAMLProvider.cfc +++ b/models/providers/MicrosoftSAMLProvider.cfc @@ -12,24 +12,30 @@ component property name="federationMetadataURL"; property name="expectedIssuer"; - property name="wirebox" inject="wirebox"; + property name="wirebox" inject="wirebox"; + property name="javaLoader" inject="loader@cbjavaloader"; property name="AuthNRequestGenerator"; property name="responseValidator"; property name="SAMLParsingService" inject="SAMLParsingService@cbsso"; variables.name = "Entra"; variables.federationMetadataURL = ""; + variables.certificatesCached = false; public string function getName(){ return variables.name; } + /** + * Stores the URL and nothing more. It used to initialise OpenSAML and fetch the IdP's metadata here, + * but every setter on a provider runs inside ProviderService.registerProviders(), i.e. inside the + * module's onLoad() - so that turned application boot into "load a 17MB jar and make an outbound HTTPS + * call per configured provider", and any failure in either took the whole application down before it + * could serve a request. Both now happen on first use, via initializeOpenSAMLLib(). + */ public any function setFederationMetadataURL( required string federationMetadataURL ){ - variables.federationMetadataURL = federationMetadataURL; - - initializeOpenSAMLLib(); - - responseValidator.cacheCerts( variables.federationMetadataURL ); + variables.federationMetadataURL = arguments.federationMetadataURL; + variables.certificatesCached = false; return this; } @@ -53,16 +59,26 @@ component authResponse.setRawResponseData( data ); try { - variables.AuthNRequestGenerator.initOpenSAML(); - variables.responseValidator.parseAndValidate( - javacast( "string", data ), - variables.expectedIssuer - ); + // initializeOpenSAMLLib() above has already initialised OpenSAML; this needs the classloader + // context for verification specifically. SignatureValidator.validate resolves crypto + // providers through the thread context classloader, which on BoxLang is not the one the + // OpenSAML classes came from. Marshalling alone does not need it - getRawSAMLRequest() goes + // through the same OpenSAML registry unwrapped and works - so do not widen this to "any + // OpenSAML call" or narrow it away on the assumption that initialisation covered it. + runWithClassLoader( function(){ + variables.responseValidator.parseAndValidate( + javacast( "string", data ), + variables.expectedIssuer + ); + } ); } catch ( any e ) { + // A validation failure is our verdict on the response, not the IdP's, so the exception is + // what explains it. samlData.errorMessage is only populated when the IdP itself returned a + // failure status, and in that case its own wording is the more useful of the two. return authResponse .setWasSuccessful( false ) .setRawResponseData( data ) - .setErrorMessage( extractErrorMessage( xmlData ) ) + .setErrorMessage( len( samlData.errorMessage ) ? samlData.errorMessage : e.message ); } @@ -107,20 +123,74 @@ component return binaryEncode( output, "base64" ); } - private boolean function extractErrorMessage( required xmlDoc ){ - return xmlSearch( xmlDoc, "//samlp:StatusMessage" )[ 1 ].xmlchildren[ 1 ].xmltext; - } - + /** + * Resolved through cbjavaloader rather than createObject( "java", ... ): ModuleConfig hands the bundled + * jar to cbjavaloader's URLClassLoader, which createObject does not consult - it searches the server + * classpath and reports "has not been located in the [java] resolver". + * + * Two readiness conditions, guarded separately. The library is initialised once for the life of the + * application, but the certificates come from an outbound fetch that can fail on its own, so guarding + * both on the generator meant one transient metadata failure - which happens after the generator is + * published - left this provider short-circuiting on every later call with a validator holding no + * certificates, and no way back short of an application restart. + */ private void function initializeOpenSAMLLib(){ - if ( !isNull( variables.AuthNRequestGenerator ) ) { + if ( isNull( variables.AuthNRequestGenerator ) ) { + runWithClassLoader( function(){ + var generator = wirebox.getInstance( "javaloader:cbsso.opensaml.AuthNRequestGenerator" ); + var validator = wirebox.getInstance( "javaloader:cbsso.opensaml.AuthResponseValidator" ); + + generator.initOpenSAML(); + + // Published only once initOpenSAML() has returned. Assigning beforehand would let a failed + // initialisation leave a provider whose guard above short-circuits, so it could never + // initialise again for the life of the application. + variables.AuthNRequestGenerator = generator; + variables.responseValidator = validator; + } ); + } + + if ( variables.certificatesCached ) { return; } - variables.AuthNRequestGenerator = createObject( "java", "cbsso.opensaml.AuthNRequestGenerator" ); - variables.responseValidator = createObject( "java", "cbsso.opensaml.AuthResponseValidator" ); + // Reached lazily now rather than from setFederationMetadataURL(), so an unset URL arrives here + // instead of never getting this far. Named rather than left to fail inside cacheCerts, where an + // empty URL surfaces as an opaque fetch error on the user's first sign-in. + if ( !len( trim( variables.federationMetadataURL ) ) ) { + throw( + type = "MicrosoftSAMLProvider.MissingConfiguration", + message = "federationMetadataURL is required but not set", + detail = "Set it on the provider definition; it is the source of the signing certificates." + ); + } + + variables.responseValidator.cacheCerts( variables.federationMetadataURL ); + + variables.certificatesCached = true; + } + + /** + * OpenSAML's InitializationService discovers its providers through ServiceLoader, which reads the + * *thread context* classloader. On BoxLang that is not cbjavaloader's URLClassLoader, so discovery finds + * nothing and initialisation fails; Adobe ColdFusion resolves it without help. Swapped only for the + * duration of the call, and restored in a finally so a failure cannot leak the wrong loader into the + * request thread. + */ + private any function runWithClassLoader( required function callback ){ + if ( !structKeyExists( server, "BoxLang" ) ) { + return callback(); + } + + var currentThread = createObject( "java", "java.lang.Thread" ).currentThread(); + var originalClassLoader = currentThread.getContextClassLoader(); - variables.AuthNRequestGenerator.initOpenSAML(); - responseValidator.cacheCerts( variables.federationMetadataURL ); + try { + currentThread.setContextClassLoader( variables.javaLoader.getURLClassLoader() ); + return callback(); + } finally { + currentThread.setContextClassLoader( originalClassLoader ); + } } } diff --git a/models/utility/SAMLParsingService.cfc b/models/utility/SAMLParsingService.cfc index f3e6805..ea2079f 100644 --- a/models/utility/SAMLParsingService.cfc +++ b/models/utility/SAMLParsingService.cfc @@ -42,13 +42,23 @@ component singleton { return data; } + /** + * Matched on local-name() rather than the `samlp:` prefix. extractUserInfo() strips only the default + * namespace declaration, so `xmlns:samlp` survives on the document - but BoxLang's xmlSearch does not + * resolve a prefixed XPath against a prefix declared in the document, so `//samlp:StatusCode` finds + * nothing there and a valid, signed, successful assertion is reported as a failure. local-name() is + * the form that behaves the same on every engine. + */ private boolean function detectSuccess( required xmlDoc ){ - return xmlSearch( xmlDoc, "//samlp:StatusCode[@Value='urn:oasis:names:tc:SAML:2.0:status:Success']" ).len() == 1; + return xmlSearch( + xmlDoc, + "//*[local-name()='StatusCode' and @Value='urn:oasis:names:tc:SAML:2.0:status:Success']" + ).len() == 1; } private string function extractErrorMessage( required xmlDoc ){ try { - return xmlSearch( xmlDoc, "//samlp:StatusMessage" )[ 1 ].xmlchildren[ 1 ].xmltext; + return xmlSearch( xmlDoc, "//*[local-name()='StatusMessage']" )[ 1 ].xmlchildren[ 1 ].xmltext; } catch ( any e ) { try { var nodes = xmlSearch( xmlDoc, "//*" );