fix(saml): require signed SAML responses by default

core-web/libs/portlets/dot-auth/src/lib/dot-auth-config/components/dot-auth-saml-config.component.html

@@ -144,8 +144,10 @@ <h3>
                         tooltipPosition="top"></i>
                 </label>
                 <div class="toggle-row flex items-center gap-2.5">
+                    <!-- at least one signature is always required: the last one on can not be switched off -->
                     <p-toggleswitch
                         [ngModel]="saml().wantAssertionsSigned"
+                        [disabled]="saml().wantAssertionsSigned && !saml().wantResponseSigned"
                         (ngModelChange)="onChange('wantAssertionsSigned', $event)" />
                 </div>
             </div>
@@ -158,8 +160,10 @@ <h3>
                         tooltipPosition="top"></i>
                 </label>
                 <div class="toggle-row flex items-center gap-2.5">
+                    <!-- at least one signature is always required: the last one on can not be switched off -->
                     <p-toggleswitch
                         [ngModel]="saml().wantResponseSigned"
+                        [disabled]="saml().wantResponseSigned && !saml().wantAssertionsSigned"
                         (ngModelChange)="onChange('wantResponseSigned', $event)" />
                 </div>
             </div>

core-web/libs/portlets/dot-auth/src/lib/dot-auth-config/store/dot-auth-config.mappers.spec.ts

@@ -1,4 +1,9 @@
-import { DotAuthConfig, DotAuthConfigView, DotAuthDiscoveryView } from '@dotcms/dotcms-models';
+import {
+    DotAuthConfig,
+    DotAuthConfigView,
+    DotAuthDiscoveryView,
+    DotAuthSignatureValidation
+} from '@dotcms/dotcms-models';
 
 import {
     DEFAULT_CONFIG,
@@ -323,6 +328,39 @@ describe('dot-auth-config.mappers', () => {
             expect(config.saml.wantResponseSigned).toBe(true);
         });
 
+        it('maps single-signature validation types', () => {
+            const assertionOnly = fromView({
+                ...SAML_VIEW,
+                values: { ...SAML_VIEW.values, signatureValidationType: 'assertion' }
+            });
+            expect(assertionOnly.saml.wantAssertionsSigned).toBe(true);
+            expect(assertionOnly.saml.wantResponseSigned).toBe(false);
+
+            const responseOnly = fromView({
+                ...SAML_VIEW,
+                values: { ...SAML_VIEW.values, signatureValidationType: 'response' }
+            });
+            expect(responseOnly.saml.wantAssertionsSigned).toBe(false);
+            expect(responseOnly.saml.wantResponseSigned).toBe(true);
+        });
+
+        it('shows a legacy "none" or unknown validation type as both signatures required', () => {
+            // out-of-range values on purpose: what an old or hand-edited configuration may hold
+            const legacyValues = [
+                'none',
+                '',
+                'signature'
+            ] as unknown as DotAuthSignatureValidation[];
+            for (const signatureValidationType of legacyValues) {
+                const config = fromView({
+                    ...SAML_VIEW,
+                    values: { ...SAML_VIEW.values, signatureValidationType }
+                });
+                expect(config.saml.wantAssertionsSigned).toBe(true);
+                expect(config.saml.wantResponseSigned).toBe(true);
+            }
+        });
+
         it('maps SAML claim fields', () => {
             const config = fromView(SAML_VIEW);
             expect(config.saml.claimEmail).toBe('mail');
@@ -527,13 +565,14 @@ describe('dot-auth-config.mappers', () => {
                 ]
             ).toBe('response');
 
+            // never 'none': with neither box checked both signatures are required
             config.saml.wantAssertionsSigned = false;
             config.saml.wantResponseSigned = false;
             expect(
                 (toPayload(config, 'test-site-id').values as Record<string, unknown>)[
                     'signatureValidationType'
                 ]
-            ).toBe('none');
+            ).toBe('responseandassertion');
         });
 
         it('round-trips idpName and sPEndpointHostname so a save never wipes a working SP hostname', () => {

core-web/libs/portlets/dot-auth/src/lib/dot-auth-config/store/dot-auth-config.mappers.ts

@@ -80,6 +80,17 @@ export const DEFAULT_CONFIG: DotAuthConfig = {
     }
 };
 
+/** The SAML signature validation type the backend enforces for a stored value. */
+function samlValidationType(value: unknown): 'response' | 'assertion' | 'responseandassertion' {
+    const normalized = String(value ?? '')
+        .trim()
+        .toLowerCase();
+
+    return normalized === 'response' || normalized === 'assertion'
+        ? normalized
+        : 'responseandassertion';
+}
+
 export function fromView(view: DotAuthConfigView): DotAuthConfig {
     const config = clone(DEFAULT_CONFIG);
     config.protocol =
@@ -112,12 +123,10 @@ export function fromView(view: DotAuthConfigView): DotAuthConfig {
             x509cert: String(values.publicCert ?? ''),
             privateKey: String(values.privateKey ?? ''),
             signRequests: String(values['signRequests'] ?? 'true') === 'true',
-            wantAssertionsSigned:
-                values.signatureValidationType === 'assertion' ||
-                values.signatureValidationType === 'responseandassertion',
-            wantResponseSigned:
-                values.signatureValidationType === 'response' ||
-                values.signatureValidationType === 'responseandassertion',
+            // Anything other than 'response' / 'assertion' (e.g. a legacy 'none') is enforced by
+            // the backend as 'responseandassertion', so show it that way.
+            wantAssertionsSigned: samlValidationType(values.signatureValidationType) !== 'response',
+            wantResponseSigned: samlValidationType(values.signatureValidationType) !== 'assertion',
             claimEmail: String(values['attribute.email.name'] ?? config.saml.claimEmail),
             claimFirstName: String(
                 values['attribute.firstname.name'] ?? config.saml.claimFirstName
@@ -180,13 +189,13 @@ export function toPayload(config: DotAuthConfig, siteId: string): DotAuthConfigP
                 // empty string would be STORED and shadow the host-name fallback downstream.
                 sPEndpointHostname: config.saml.spEndpointHostname || undefined,
                 signRequests: String(config.saml.signRequests),
-                signatureValidationType: config.saml.wantResponseSigned
-                    ? config.saml.wantAssertionsSigned
-                        ? 'responseandassertion'
-                        : 'response'
-                    : config.saml.wantAssertionsSigned
-                      ? 'assertion'
-                      : 'none',
+                // Never 'none': with neither box checked both signatures are required.
+                signatureValidationType:
+                    config.saml.wantResponseSigned && !config.saml.wantAssertionsSigned
+                        ? 'response'
+                        : config.saml.wantAssertionsSigned && !config.saml.wantResponseSigned
+                          ? 'assertion'
+                          : 'responseandassertion',
                 idPMetadataFile: config.saml.metadataUrl,
                 publicCert: config.saml.x509cert || undefined,
                 privateKey: config.saml.privateKey || undefined,

dotCMS/src/main/java/com/dotcms/auth/dotAuth/rest/handler/SamlProtocolHandler.java

@@ -2,13 +2,15 @@
 
 import com.dotcms.auth.dotAuth.DotAuthConstants;
 import com.dotcms.auth.dotAuth.rest.DotAuthProtocol;
+import com.dotcms.saml.DotIdentityProviderConfigurationImpl;
 import com.dotcms.saml.DotSamlProxyFactory;
 import com.dotcms.security.apps.AppSecrets;
 import com.dotmarketing.util.UtilMethods;
 import com.dotcms.security.apps.Secret;
 import io.vavr.control.Try;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
@@ -40,6 +42,8 @@ public final class SamlProtocolHandler implements ProtocolHandler {
 
     private static final Set<String> BOOLEAN_KEYS = Set.of("enable");
 
+    private static final String SIGNATURE_VALIDATION_TYPE_KEY = "signatureValidationType";
+
     /**
      * Extra-key names must be identifier-shaped. Bars whitespace tricks like
      * {@code "privateKey "} (trailing space) that would otherwise slip past the
@@ -117,6 +121,8 @@ public AppSecrets buildSecrets(final Map<String, Object> incoming,
             if (raw == null) continue;
             if (BOOLEAN_KEYS.contains(key)) {
                 builder.withSecret(key, Boolean.parseBoolean(String.valueOf(raw)));
+            } else if (SIGNATURE_VALIDATION_TYPE_KEY.equals(key)) {
+                builder.withSecret(key, validSignatureValidationType(String.valueOf(raw)));
             } else {
                 builder.withSecret(key, String.valueOf(raw));
                 if ("publicCert".equals(key) && UtilMethods.isSet(String.valueOf(raw))) {
@@ -175,4 +181,17 @@ public AppSecrets buildSecrets(final Map<String, Object> incoming,
         }
         return builder.build();
     }
+
+    /**
+     * Only "response", "assertion" and "responseandassertion" can be stored: SAML responses
+     * are never accepted without an IdP signature.
+     */
+    private static String validSignatureValidationType(final String value) {
+        final String normalized = value.trim().toLowerCase(Locale.ROOT);
+        if (!normalized.equals(DotIdentityProviderConfigurationImpl.resolveSignatureValidationType(normalized))) {
+            throw new BadRequestException(SIGNATURE_VALIDATION_TYPE_KEY
+                    + " must be one of response, assertion or responseandassertion");
+        }
+        return normalized;
+    }
 }

dotCMS/src/main/java/com/dotcms/saml/DotAbstractSamlConfigurationServiceImpl.java

@@ -9,6 +9,7 @@
 import java.nio.file.Files;
 import java.util.Map;
 import java.util.Properties;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Supplier;
@@ -23,6 +24,8 @@ public abstract class DotAbstractSamlConfigurationServiceImpl implements SamlCon
     private static final String NULL = "NULL";
     private static final String UNABLE_TO_READ_FILE = "File does not exist or unable to read : ";
     private static final String NOT_FOUND_ERROR = "Property Name not Found: ";
+    private static final Set<SamlName> SIGNATURE_CHECKS = Set.of(
+            SamlName.DOT_SAML_VERIFY_SIGNATURE_CREDENTIALS, SamlName.DOT_SAML_VERIFY_SIGNATURE_PROFILE);
 
     private AtomicBoolean init = new AtomicBoolean(false);
     private final Map<String, String> defaultProperties = new ConcurrentHashMap<>();
@@ -73,7 +76,17 @@ private synchronized void internalInit(final Map<String, Object> contextMap) {
                 properties.forEach((key, value) -> {
 
                     final SamlName samlName = SamlName.findProperty((String)key);
-                    if (null != samlName) {
+                    if (null == samlName) {
+
+                        return;
+                    }
+
+                    if (SIGNATURE_CHECKS.contains(samlName)) {
+
+                        // signature checks can't be switched off: keep the default (true)
+                        Logger.warn(this, "Ignoring '" + key + "' in " + dotSamlDefaultPropertiesValue
+                                + ": SAML signatures are always verified.");
+                    } else {
 
                         this.defaultProperties.put(samlName.getPropertyName(), (String)value);
                     }

dotCMS/src/main/java/com/dotcms/saml/DotIdentityProviderConfigurationImpl.java

@@ -7,26 +7,95 @@
 import com.dotmarketing.business.APILocator;
 import com.dotmarketing.exception.DotDataException;
 import com.dotmarketing.exception.DotSecurityException;
+import com.dotmarketing.util.Logger;
 
+import java.util.Locale;
 import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Default implementation to retrieve the configuration from apps
  * @author jsanca
  */
 public class DotIdentityProviderConfigurationImpl implements IdentityProviderConfiguration {
 
+    private static final Set<String> SIGNATURE_VALIDATION_TYPES = Set.of(
+            DotSamlConstants.RESPONSE, DotSamlConstants.ASSERTION, DotSamlConstants.RESPONSE_AND_ASSERTION);
+
+    /**
+     * Extra parameters that used to switch off parts of the signature verification. Stored values are
+     * ignored, so the SAML bundle always uses its default (true).
+     */
+    private static final Set<String> IGNORED_PROPERTIES = Set.of(
+            SamlName.DOT_SAML_VERIFY_SIGNATURE_CREDENTIALS.getPropertyName(),
+            SamlName.DOT_SAML_VERIFY_SIGNATURE_PROFILE.getPropertyName());
+
+    /**
+     * Warnings already logged, keyed by site and setting. A configuration is built for every request
+     * that needs it, so each warning is logged once per site and value rather than on every request.
+     */
+    private static final Set<String> LOGGED_WARNINGS = ConcurrentHashMap.newKeySet();
+
     private final AppsAPI    appsAPI;
     private final Host       host;
     private final AppSecrets appSecrets;
 
     public DotIdentityProviderConfigurationImpl(final AppsAPI appsAPI, final Host host) throws DotSecurityException, DotDataException {
 
+        this(appsAPI, host, appsAPI.getSecrets(DotSamlProxyFactory.SAML_APP_CONFIG_KEY,
+                true, host, APILocator.systemUser()).get());
+    }
+
+    DotIdentityProviderConfigurationImpl(final AppsAPI appsAPI, final Host host, final AppSecrets appSecrets) {
+
         this.appsAPI    = appsAPI;
         this.host       = host;
-        this.appSecrets = this.appsAPI.getSecrets(DotSamlProxyFactory.SAML_APP_CONFIG_KEY,
-                true, host, APILocator.systemUser()).get();
+        this.appSecrets = appSecrets;
+
+        this.warnAboutOverriddenSettings();
+    }
+
+    /**
+     * Returns the signature validation type when it is "response", "assertion" or "responseandassertion"
+     * (ignoring case and surrounding blanks). Anything else, including blank and "none", fails closed to
+     * "responseandassertion": signatures can not be switched off through this setting.
+     *
+     * @param configured the stored value, may be null
+     * @return one of the supported validation types
+     */
+    public static String resolveSignatureValidationType(final String configured) {
+
+        final String normalized = null == configured ? "" : configured.trim().toLowerCase(Locale.ROOT);
+        return SIGNATURE_VALIDATION_TYPES.contains(normalized) ? normalized : DotSamlConstants.RESPONSE_AND_ASSERTION;
+    }
 
+    private void warnAboutOverriddenSettings() {
+
+        final Optional<Secret> validationType =
+                this.findSecret(SamlName.DOT_SAML_SIGNATURE_VALIDATION_TYPE.getPropertyName());
+        final String configured = validationType.map(Secret::getString).orElse(null);
+        if ((null == configured || !SIGNATURE_VALIDATION_TYPES.contains(configured.trim().toLowerCase(Locale.ROOT)))
+                && firstTime("validationType=" + configured)) {
+
+            Logger.warn(this, "SAML configuration for site '" + this.host.getHostname() + "' has an unsupported "
+                    + "signature validation type '" + configured + "'. A signed response and a signed assertion "
+                    + "are required until a supported Validation Type is saved.");
+        }
+
+        for (final String ignoredProperty : IGNORED_PROPERTIES) {
+
+            if (this.findSecret(ignoredProperty).isPresent() && firstTime(ignoredProperty)) {
+
+                Logger.warn(this, "SAML configuration for site '" + this.host.getHostname() + "' sets '"
+                        + ignoredProperty + "', which is ignored: SAML signatures are always verified.");
+            }
+        }
+    }
+
+    private boolean firstTime(final String warning) {
+
+        return LOGGED_WARNINGS.add(this.host.getIdentifier() + '|' + warning);
     }
 
     private Optional<Secret> findSecret (final String key) {
@@ -78,7 +147,7 @@ public String getSignatureValidationType() {
 
         final String signatureValidationTypeKey = SamlName.DOT_SAML_SIGNATURE_VALIDATION_TYPE.getPropertyName();
         final Optional<Secret> secretOpt        = this.findSecret(signatureValidationTypeKey);
-        return secretOpt.isPresent()? secretOpt.get().getString(): null;
+        return resolveSignatureValidationType(secretOpt.isPresent()? secretOpt.get().getString(): null);
     }
 
     @Override
@@ -111,6 +180,10 @@ public char[] getPrivateKey() {
     @Override
     public Object getOptionalProperty(final String propertyKey) {
 
+        if (IGNORED_PROPERTIES.contains(propertyKey)) {
+            return null;
+        }
+
         final Optional<Secret> secretOpt = this.findSecret(propertyKey);
         return secretOpt.isPresent()? secretOpt.get().getString(): null;
     }
@@ -119,7 +192,7 @@ public Object getOptionalProperty(final String propertyKey) {
     @Override
     public boolean containsOptionalProperty(final String propertyKey) {
 
-        return this.findSecret(propertyKey).isPresent();
+        return !IGNORED_PROPERTIES.contains(propertyKey) && this.findSecret(propertyKey).isPresent();
     }
 
     @Override

dotCMS/src/main/resources/apps/dotsaml-config.yml

@@ -37,13 +37,9 @@ params:
     hidden: false
     type: "SELECT"
     label: "Validation Type"
-    hint: "Depends on your IDP Configuration."
+    hint: "Select exactly the parts of the SAML Response your IDP signs. dotCMS rejects a response unless the selected parts carry a valid IDP signature."
     required: true
     value:
-      -
-        label: "None"
-        value: "none"
-        selected: true
       -
         label: "Only Response"
         value: "response"
@@ -53,6 +49,7 @@ params:
       -
         label: "Response & Assertion"
         value: "responseandassertion"
+        selected: true
   idPMetadataFile:
     value: ""
     hidden: false