Skip to content

Commit e41add3

Browse files
shwstpprshwstppr
committed
saml: signature check improvements
Adminstrators should ensure that IDP configuration has a signing certificate for the actual signature check to be performed. In addition to this, this change introduces a new global setting saml2.check.signature, with the default value of true, which can deliberately fail a SAML login attempt when the SAML response has a missing signature. Purges the SAML token upon handling the first SAML response. Authored-by: Rohit Yadav <[email protected]> Signed-off-by: Abhishek Kumar <[email protected]>
1 parent 8a00e25 commit e41add3

File tree

4 files changed

+72
-27
lines changed

4 files changed

+72
-27
lines changed

‎plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmd.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,14 @@ public Response processSAMLResponse(String responseMessage) {
144144
return responseObject;
145145
}
146146

147+
protected void checkAndFailOnMissingSAMLSignature(Signature signature) {
148+
if (signature == null && SAML2AuthManager.SAMLCheckSignature.value()) {
149+
s_logger.error("Failing SAML login due to missing signature in the SAML response and signature check is enforced. " +
150+
"Please check and ensure the IDP configuration has signing certificate or relax the saml2.check.signature setting.");
151+
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Signature is missing from the SAML Response. Please contact the Administrator");
152+
}
153+
}
154+
147155
@Override
148156
public String authenticate(final String command, final Map<String, Object[]> params, final HttpSession session, final InetAddress remoteAddress, final String responseType, final StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException {
149157
try {
@@ -220,11 +228,13 @@ public String authenticate(final String command, final Map<String, Object[]> par
220228
"Received SAML response for a SSO request that we may not have made or has expired, please try logging in again",
221229
params, responseType));
222230
}
231+
samlAuthManager.purgeToken(token);
223232

224233
// Set IdpId for this session
225234
session.setAttribute(SAMLPluginConstants.SAML_IDPID, issuer.getValue());
226235

227236
Signature sig = processedSAMLResponse.getSignature();
237+
checkAndFailOnMissingSAMLSignature(sig);
228238
if (idpMetadata.getSigningCertificate() != null && sig != null) {
229239
BasicX509Credential credential = new BasicX509Credential();
230240
credential.setEntityCertificate(idpMetadata.getSigningCertificate());
@@ -238,9 +248,8 @@ public String authenticate(final String command, final Map<String, Object[]> par
238248
params, responseType));
239249
}
240250
}
241-
if (username == null) {
242-
username = SAMLUtils.getValueFromAssertions(processedSAMLResponse.getAssertions(), SAML2AuthManager.SAMLUserAttributeName.value());
243-
}
251+
252+
username = SAMLUtils.getValueFromAssertions(processedSAMLResponse.getAssertions(), SAML2AuthManager.SAMLUserAttributeName.value());
244253

245254
for (Assertion assertion: processedSAMLResponse.getAssertions()) {
246255
if (assertion!= null && assertion.getSubject() != null && assertion.getSubject().getNameID() != null) {
@@ -272,6 +281,7 @@ public String authenticate(final String command, final Map<String, Object[]> par
272281
continue;
273282
}
274283
Signature encSig = assertion.getSignature();
284+
checkAndFailOnMissingSAMLSignature(encSig);
275285
if (idpMetadata.getSigningCertificate() != null && encSig != null) {
276286
BasicX509Credential sigCredential = new BasicX509Credential();
277287
sigCredential.setEntityCertificate(idpMetadata.getSigningCertificate());

‎plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManager.java‎

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,59 +25,63 @@
2525

2626
public interface SAML2AuthManager extends PluggableAPIAuthenticator, PluggableService {
2727

28-
public static final ConfigKey<Boolean> SAMLIsPluginEnabled = new ConfigKey<Boolean>("Advanced", Boolean.class, "saml2.enabled", "false",
28+
ConfigKey<Boolean> SAMLIsPluginEnabled = new ConfigKey<Boolean>("Advanced", Boolean.class, "saml2.enabled", "false",
2929
"Indicates whether SAML SSO plugin is enabled or not", true);
3030

31-
public static final ConfigKey<String> SAMLServiceProviderID = new ConfigKey<String>("Advanced", String.class, "saml2.sp.id", "org.apache.cloudstack",
31+
ConfigKey<String> SAMLServiceProviderID = new ConfigKey<String>("Advanced", String.class, "saml2.sp.id", "org.apache.cloudstack",
3232
"SAML2 Service Provider Identifier String", true);
3333

34-
public static final ConfigKey<String> SAMLServiceProviderContactPersonName = new ConfigKey<String>("Advanced", String.class, "saml2.sp.contact.person", "CloudStack Developers",
34+
ConfigKey<String> SAMLServiceProviderContactPersonName = new ConfigKey<String>("Advanced", String.class, "saml2.sp.contact.person", "CloudStack Developers",
3535
"SAML2 Service Provider Contact Person Name", true);
3636

37-
public static final ConfigKey<String> SAMLServiceProviderContactEmail = new ConfigKey<String>("Advanced", String.class, "saml2.sp.contact.email", "[email protected]",
37+
ConfigKey<String> SAMLServiceProviderContactEmail = new ConfigKey<String>("Advanced", String.class, "saml2.sp.contact.email", "[email protected]",
3838
"SAML2 Service Provider Contact Email Address", true);
3939

40-
public static final ConfigKey<String> SAMLServiceProviderOrgName = new ConfigKey<String>("Advanced", String.class, "saml2.sp.org.name", "Apache CloudStack",
40+
ConfigKey<String> SAMLServiceProviderOrgName = new ConfigKey<String>("Advanced", String.class, "saml2.sp.org.name", "Apache CloudStack",
4141
"SAML2 Service Provider Organization Name", true);
4242

43-
public static final ConfigKey<String> SAMLServiceProviderOrgUrl = new ConfigKey<String>("Advanced", String.class, "saml2.sp.org.url", "http://cloudstack.apache.org",
43+
ConfigKey<String> SAMLServiceProviderOrgUrl = new ConfigKey<String>("Advanced", String.class, "saml2.sp.org.url", "http://cloudstack.apache.org",
4444
"SAML2 Service Provider Organization URL", true);
4545

46-
public static final ConfigKey<String> SAMLServiceProviderSingleSignOnURL = new ConfigKey<String>("Advanced", String.class, "saml2.sp.sso.url", "http://localhost:8080/client/api?command=samlSso",
46+
ConfigKey<String> SAMLServiceProviderSingleSignOnURL = new ConfigKey<String>("Advanced", String.class, "saml2.sp.sso.url", "http://localhost:8080/client/api?command=samlSso",
4747
"SAML2 CloudStack Service Provider Single Sign On URL", true);
4848

49-
public static final ConfigKey<String> SAMLServiceProviderSingleLogOutURL = new ConfigKey<String>("Advanced", String.class, "saml2.sp.slo.url", "http://localhost:8080/client/",
49+
ConfigKey<String> SAMLServiceProviderSingleLogOutURL = new ConfigKey<String>("Advanced", String.class, "saml2.sp.slo.url", "http://localhost:8080/client/",
5050
"SAML2 CloudStack Service Provider Single Log Out URL", true);
5151

52-
public static final ConfigKey<String> SAMLCloudStackRedirectionUrl = new ConfigKey<String>("Advanced", String.class, "saml2.redirect.url", "http://localhost:8080/client",
52+
ConfigKey<String> SAMLCloudStackRedirectionUrl = new ConfigKey<String>("Advanced", String.class, "saml2.redirect.url", "http://localhost:8080/client",
5353
"The CloudStack UI url the SSO should redirected to when successful", true);
5454

55-
public static final ConfigKey<String> SAMLUserAttributeName = new ConfigKey<String>("Advanced", String.class, "saml2.user.attribute", "uid",
55+
ConfigKey<String> SAMLUserAttributeName = new ConfigKey<String>("Advanced", String.class, "saml2.user.attribute", "uid",
5656
"Attribute name to be looked for in SAML response that will contain the username", true);
5757

58-
public static final ConfigKey<String> SAMLIdentityProviderMetadataURL = new ConfigKey<String>("Advanced", String.class, "saml2.idp.metadata.url", "https://openidp.feide.no/simplesaml/saml2/idp/metadata.php",
58+
ConfigKey<String> SAMLIdentityProviderMetadataURL = new ConfigKey<String>("Advanced", String.class, "saml2.idp.metadata.url", "https://openidp.feide.no/simplesaml/saml2/idp/metadata.php",
5959
"SAML2 Identity Provider Metadata XML Url", true);
6060

61-
public static final ConfigKey<String> SAMLDefaultIdentityProviderId = new ConfigKey<String>("Advanced", String.class, "saml2.default.idpid", "https://openidp.feide.no",
61+
ConfigKey<String> SAMLDefaultIdentityProviderId = new ConfigKey<String>("Advanced", String.class, "saml2.default.idpid", "https://openidp.feide.no",
6262
"The default IdP entity ID to use only in case of multiple IdPs", true);
6363

64-
public static final ConfigKey<String> SAMLSignatureAlgorithm = new ConfigKey<>(String.class, "saml2.sigalg", "Advanced", "SHA1",
64+
ConfigKey<String> SAMLSignatureAlgorithm = new ConfigKey<>(String.class, "saml2.sigalg", "Advanced", "SHA1",
6565
"The algorithm to use to when signing a SAML request. Default is SHA1, allowed algorithms: SHA1, SHA256, SHA384, SHA512", true, ConfigKey.Scope.Global, null, null, null, null, null, ConfigKey.Kind.Select, "SHA1,SHA256,SHA384,SHA512");
6666

67-
public static final ConfigKey<Boolean> SAMLAppendDomainSuffix = new ConfigKey<Boolean>("Advanced", Boolean.class, "saml2.append.idpdomain", "false",
67+
ConfigKey<Boolean> SAMLAppendDomainSuffix = new ConfigKey<Boolean>("Advanced", Boolean.class, "saml2.append.idpdomain", "false",
6868
"If enabled, create account/user dialog with SAML SSO enabled will append the IdP domain to the user or account name in the UI dialog", true);
6969

70-
public static final ConfigKey<Integer> SAMLTimeout = new ConfigKey<Integer>("Advanced", Integer.class, "saml2.timeout", "1800",
70+
ConfigKey<Integer> SAMLTimeout = new ConfigKey<Integer>("Advanced", Integer.class, "saml2.timeout", "1800",
7171
"SAML2 IDP Metadata refresh interval in seconds, minimum value is set to 300", true);
7272

73-
public SAMLProviderMetadata getSPMetadata();
74-
public SAMLProviderMetadata getIdPMetadata(String entityId);
75-
public Collection<SAMLProviderMetadata> getAllIdPMetadata();
73+
ConfigKey<Boolean> SAMLCheckSignature = new ConfigKey<Boolean>("Advanced", Boolean.class, "saml2.check.signature", "true",
74+
"When enabled (default and recommended), SAML2 signature checks are enforced and lack of signature in the SAML SSO response will cause login exception. Disabling this is not advisable but provided for backward compatibility for users who are able to accept the risks.", false);
7675

77-
public boolean isUserAuthorized(Long userId, String entityId);
78-
public boolean authorizeUser(Long userId, String entityId, boolean enable);
76+
SAMLProviderMetadata getSPMetadata();
77+
SAMLProviderMetadata getIdPMetadata(String entityId);
78+
Collection<SAMLProviderMetadata> getAllIdPMetadata();
7979

80-
public void saveToken(String authnId, String domain, String entity);
81-
public SAMLTokenVO getToken(String authnId);
82-
public void expireTokens();
80+
boolean isUserAuthorized(Long userId, String entityId);
81+
boolean authorizeUser(Long userId, String entityId, boolean enable);
82+
83+
void saveToken(String authnId, String domain, String entity);
84+
SAMLTokenVO getToken(String authnId);
85+
void purgeToken(SAMLTokenVO token);
86+
void expireTokens();
8387
}

‎plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,6 +487,13 @@ public SAMLTokenVO getToken(String authnId) {
487487
return _samlTokenDao.findByUuid(authnId);
488488
}
489489

490+
@Override
491+
public void purgeToken(SAMLTokenVO token) {
492+
if (token != null) {
493+
_samlTokenDao.remove(token.getId());
494+
}
495+
}
496+
490497
@Override
491498
public void expireTokens() {
492499
_samlTokenDao.expireTokens();
@@ -535,6 +542,6 @@ public ConfigKey<?>[] getConfigKeys() {
535542
SAMLServiceProviderSingleSignOnURL, SAMLServiceProviderSingleLogOutURL,
536543
SAMLCloudStackRedirectionUrl, SAMLUserAttributeName,
537544
SAMLIdentityProviderMetadataURL, SAMLDefaultIdentityProviderId,
538-
SAMLSignatureAlgorithm, SAMLAppendDomainSuffix, SAMLTimeout};
545+
SAMLSignatureAlgorithm, SAMLAppendDomainSuffix, SAMLTimeout, SAMLCheckSignature};
539546
}
540547
}

‎plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/api/command/SAML2LoginAPIAuthenticatorCmdTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,30 @@ public void whenFailToAuthenticateThrowExceptionOrRedirectToUrlTestSaml2FailedLo
271271
verifyTestWhenFailToAuthenticateThrowExceptionOrRedirectToUrl(false, hasThrownServerApiException, 0, 0);
272272
}
273273

274+
private void overrideDefaultConfigValue(final ConfigKey configKey, final String name, final Object o) throws IllegalAccessException, NoSuchFieldException {
275+
Field f = ConfigKey.class.getDeclaredField(name);
276+
f.setAccessible(true);
277+
f.set(configKey, o);
278+
}
279+
280+
@Test
281+
public void testFailOnSAMLSignatureCheckWhenFalse() throws NoSuchFieldException, IllegalAccessException {
282+
overrideDefaultConfigValue(SAML2AuthManager.SAMLCheckSignature, "_value", false);
283+
SAML2LoginAPIAuthenticatorCmd cmd = new SAML2LoginAPIAuthenticatorCmd();
284+
try {
285+
cmd.checkAndFailOnMissingSAMLSignature(null);
286+
} catch(Exception e) {
287+
Assert.fail("This shouldn't throw any exception");
288+
}
289+
}
290+
291+
@Test(expected = ServerApiException.class)
292+
public void testFailOnSAMLSignatureCheckWhenTrue() throws NoSuchFieldException, IllegalAccessException {
293+
overrideDefaultConfigValue(SAML2AuthManager.SAMLCheckSignature, "_value", true);
294+
SAML2LoginAPIAuthenticatorCmd cmd = new SAML2LoginAPIAuthenticatorCmd();
295+
cmd.checkAndFailOnMissingSAMLSignature(null);
296+
}
297+
274298
private UserAccountVO configureTestWhenFailToAuthenticateThrowExceptionOrRedirectToUrl(String entity, String configurationValue, Boolean isUserAuthorized)
275299
throws IOException {
276300
Mockito.when(samlAuthManager.isUserAuthorized(nullable(Long.class), nullable(String.class))).thenReturn(isUserAuthorized);

0 commit comments

Comments
 (0)