Skip to content

Commit 7671713

Browse files
sguilhenhmlnarik
sguilhen
authored and
hmlnarik
committed
[KEYCLOAK-12998] Prevent duplicate resources from being added to the keycloak-saml subsystem
 - Fixes an issue in parser where the closing tag of the IDP element was in the wrong place, which could break the server configuration - Parser now checks for duplicates of elements described with maxOccurs=1 in the schema - Add handler for SP and IDP now check for existing SPs or IDPs in the config, preventing addition of a duplicate resource via CLI - Subsystem test was enhanced so it now tests some invalid configs with duplicate elements
1 parent 753c21e commit 7671713

File tree

10 files changed

+383
-30
lines changed

10 files changed

+383
-30
lines changed

‎adapters/saml/as7-eap6/subsystem/src/main/java/org/keycloak/subsystem/saml/as7/Configuration.java

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
*/
1717
package org.keycloak.subsystem.saml.as7;
1818

19+
import java.util.List;
20+
21+
import org.jboss.as.controller.OperationFailedException;
1922
import org.jboss.as.server.deployment.DeploymentUnit;
2023
import org.jboss.as.web.deployment.WarMetaData;
2124
import org.jboss.dmr.ModelNode;
@@ -34,21 +37,29 @@ public class Configuration {
3437
private Configuration() {
3538
}
3639

37-
void updateModel(ModelNode operation, ModelNode model) {
40+
void updateModel(ModelNode operation, ModelNode model) throws OperationFailedException {
41+
this.updateModel(operation, model, false);
42+
}
43+
44+
void updateModel(final ModelNode operation, final ModelNode model, final boolean checkSingleton) throws OperationFailedException {
3845
ModelNode node = config;
39-
ModelNode addr = operation.get("address");
40-
for (Property item : addr.asPropertyList()) {
41-
node = getNodeForAddressElement(node, item);
46+
47+
final List<Property> addressNodes = operation.get("address").asPropertyList();
48+
final int lastIndex = addressNodes.size() - 1;
49+
for (int i = 0; i < addressNodes.size(); i++) {
50+
Property addressNode = addressNodes.get(i);
51+
// if checkSingleton is true, we verify if the key for the last element (e.g. SP or IDP) in the address path is already defined
52+
if (i == lastIndex && checkSingleton) {
53+
if (node.get(addressNode.getName()).isDefined()) {
54+
// found an existing resource, throw an exception
55+
throw new OperationFailedException("Duplicate resource: " + addressNode.getName());
56+
}
57+
}
58+
node = node.get(addressNode.getName()).get(addressNode.getValue().asString());
4259
}
4360
node.set(model);
4461
}
4562

46-
private ModelNode getNodeForAddressElement(ModelNode node, Property item) {
47-
String key = item.getValue().asString();
48-
ModelNode keymodel = node.get(item.getName());
49-
return keymodel.get(key);
50-
}
51-
5263
public ModelNode getSecureDeployment(DeploymentUnit deploymentUnit) {
5364
String name = preferredDeploymentName(deploymentUnit);
5465
ModelNode secureDeployment = config.get("subsystem").get("keycloak-saml").get(Constants.Model.SECURE_DEPLOYMENT);

‎adapters/saml/as7-eap6/subsystem/src/main/java/org/keycloak/subsystem/saml/as7/IdentityProviderAddHandler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ class IdentityProviderAddHandler extends AbstractAddStepHandler {
3636

3737
@Override
3838
protected void performRuntime(OperationContext context, ModelNode operation, ModelNode model, ServiceVerificationHandler verificationHandler, List<ServiceController<?>> newControllers) throws OperationFailedException {
39-
Configuration.INSTANCE.updateModel(operation, model);
39+
Configuration.INSTANCE.updateModel(operation, model, true);
4040
}
4141

4242
@Override

‎adapters/saml/as7-eap6/subsystem/src/main/java/org/keycloak/subsystem/saml/as7/KeycloakSubsystemParser.java

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,13 +76,19 @@ void readSecureDeployment(XMLExtendedStreamReader reader, List<ModelNode> list)
7676
ModelNode addSecureDeployment = Util.createAddOperation(addr);
7777
list.add(addSecureDeployment);
7878

79+
Set<String> parsedElements = new HashSet<>();
7980
while (reader.hasNext() && nextTag(reader) != END_ELEMENT) {
8081
String tagName = reader.getLocalName();
82+
if (parsedElements.contains(tagName)) {
83+
// all sub-elements of the secure deployment type should occur only once.
84+
throw ParseUtils.unexpectedElement(reader);
85+
}
8186
if (tagName.equals(Constants.XML.SERVICE_PROVIDER)) {
8287
readServiceProvider(reader, list, addr);
8388
} else {
8489
throw ParseUtils.unexpectedElement(reader);
8590
}
91+
parsedElements.add(tagName);
8692
}
8793
}
8894

@@ -109,8 +115,13 @@ void readServiceProvider(XMLExtendedStreamReader reader, List<ModelNode> list, P
109115
attr.parseAndSetParameter(value, addServiceProvider, reader);
110116
}
111117

118+
Set parsedElements = new HashSet<>();
112119
while (reader.hasNext() && nextTag(reader) != END_ELEMENT) {
113120
String tagName = reader.getLocalName();
121+
if (parsedElements.contains(tagName)) {
122+
// all sub-elements of the service provider type should occur only once.
123+
throw ParseUtils.unexpectedElement(reader);
124+
}
114125

115126
if (Constants.XML.KEYS.equals(tagName)) {
116127
readKeys(list, reader, addr);
@@ -125,6 +136,7 @@ void readServiceProvider(XMLExtendedStreamReader reader, List<ModelNode> list, P
125136
} else {
126137
throw ParseUtils.unexpectedElement(reader);
127138
}
139+
parsedElements.add(tagName);
128140
}
129141
}
130142

@@ -152,8 +164,13 @@ void readIdentityProvider(List<ModelNode> list, XMLExtendedStreamReader reader,
152164
attr.parseAndSetParameter(value, addIdentityProvider, reader);
153165
}
154166

167+
Set<String> parsedElements = new HashSet<>();
155168
while (reader.hasNext() && nextTag(reader) != END_ELEMENT) {
156169
String tagName = reader.getLocalName();
170+
if (parsedElements.contains(tagName)) {
171+
// all sub-elements of the identity provider type should occur only once.
172+
throw ParseUtils.unexpectedElement(reader);
173+
}
157174

158175
if (Constants.XML.SINGLE_SIGN_ON.equals(tagName)) {
159176
readSingleSignOn(addIdentityProvider, reader);
@@ -168,6 +185,7 @@ void readIdentityProvider(List<ModelNode> list, XMLExtendedStreamReader reader,
168185
} else {
169186
throw ParseUtils.unexpectedElement(reader);
170187
}
188+
parsedElements.add(tagName);
171189
}
172190
}
173191

@@ -265,8 +283,13 @@ void readKey(List<ModelNode> list, XMLExtendedStreamReader reader, PathAddress p
265283
attr.parseAndSetParameter(value, addKey, reader);
266284
}
267285

286+
Set<String> parsedElements = new HashSet<>();
268287
while (reader.hasNext() && nextTag(reader) != END_ELEMENT) {
269288
String tagName = reader.getLocalName();
289+
if (parsedElements.contains(tagName)) {
290+
// all sub-elements of the key type should occur only once.
291+
throw ParseUtils.unexpectedElement(reader);
292+
}
270293

271294
if (Constants.XML.KEY_STORE.equals(tagName)) {
272295
readKeyStore(addKey, reader);
@@ -278,6 +301,7 @@ void readKey(List<ModelNode> list, XMLExtendedStreamReader reader, PathAddress p
278301
} else {
279302
throw ParseUtils.unexpectedElement(reader);
280303
}
304+
parsedElements.add(tagName);
281305
}
282306
}
283307

@@ -308,15 +332,21 @@ void readKeyStore(ModelNode addKey, XMLExtendedStreamReader reader) throws XMLSt
308332
throw ParseUtils.missingRequired(reader, asSet(Constants.XML.PASSWORD));
309333
}
310334

335+
Set<String> parsedElements = new HashSet<>();
311336
while (reader.hasNext() && nextTag(reader) != END_ELEMENT) {
312337
String tagName = reader.getLocalName();
338+
if (parsedElements.contains(tagName)) {
339+
// all sub-elements of the keystore type should occur only once.
340+
throw ParseUtils.unexpectedElement(reader);
341+
}
313342
if (Constants.XML.PRIVATE_KEY.equals(tagName)) {
314343
readPrivateKey(reader, addKeyStore);
315344
} else if (Constants.XML.CERTIFICATE.equals(tagName)) {
316345
readCertificate(reader, addKeyStore);
317346
} else {
318347
throw ParseUtils.unexpectedElement(reader);
319348
}
349+
parsedElements.add(tagName);
320350
}
321351
}
322352

@@ -500,8 +530,8 @@ void writeIdentityProvider(XMLExtendedStreamWriter writer, ModelNode model) thro
500530
writeKeys(writer, idpAttributes.get(Constants.Model.KEY));
501531
writeHttpClient(writer, idpAttributes.get(Constants.Model.HTTP_CLIENT));
502532
writeAllowedClockSkew(writer, idpAttributes.get(Constants.Model.ALLOWED_CLOCK_SKEW));
533+
writer.writeEndElement();
503534
}
504-
writer.writeEndElement();
505535
}
506536

507537
void writeSingleSignOn(XMLExtendedStreamWriter writer, ModelNode model) throws XMLStreamException {

‎adapters/saml/as7-eap6/subsystem/src/main/java/org/keycloak/subsystem/saml/as7/ServiceProviderAddHandler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ class ServiceProviderAddHandler extends AbstractAddStepHandler {
3838

3939
@Override
4040
protected void performRuntime(OperationContext context, ModelNode operation, ModelNode model, ServiceVerificationHandler verificationHandler, List<ServiceController<?>> newControllers) throws OperationFailedException {
41-
Configuration.INSTANCE.updateModel(operation, model);
41+
Configuration.INSTANCE.updateModel(operation, model, true);
4242
}
4343

4444
@Override

‎adapters/saml/as7-eap6/subsystem/src/test/java/org/keycloak/subsystem/saml/as7/SubsystemParsingTestCase.java

Lines changed: 133 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,34 @@
1717
package org.keycloak.subsystem.saml.as7;
1818

1919
import java.io.IOException;
20+
import java.io.StringReader;
21+
import java.io.StringWriter;
22+
23+
import javax.xml.parsers.DocumentBuilder;
24+
import javax.xml.parsers.DocumentBuilderFactory;
25+
import javax.xml.parsers.ParserConfigurationException;
26+
import javax.xml.stream.XMLStreamException;
27+
import javax.xml.transform.OutputKeys;
28+
import javax.xml.transform.Transformer;
29+
import javax.xml.transform.TransformerException;
30+
import javax.xml.transform.TransformerFactory;
31+
import javax.xml.transform.dom.DOMSource;
32+
import javax.xml.transform.stream.StreamResult;
33+
import javax.xml.xpath.XPath;
34+
import javax.xml.xpath.XPathConstants;
35+
import javax.xml.xpath.XPathExpressionException;
36+
import javax.xml.xpath.XPathFactory;
2037

2138
import org.jboss.as.subsystem.test.AbstractSubsystemBaseTest;
39+
import org.junit.Before;
40+
import org.junit.Rule;
41+
import org.junit.Test;
42+
import org.junit.rules.ExpectedException;
43+
import org.w3c.dom.Document;
44+
import org.w3c.dom.Element;
45+
import org.w3c.dom.NodeList;
46+
import org.xml.sax.InputSource;
47+
import org.xml.sax.SAXException;
2248

2349

2450
/**
@@ -28,12 +54,118 @@
2854
*/
2955
public class SubsystemParsingTestCase extends AbstractSubsystemBaseTest {
3056

57+
private String subsystemXml = null;
58+
59+
private String subsystemTemplate = null;
60+
61+
private Document document = null;
62+
63+
@Rule
64+
public final ExpectedException exception = ExpectedException.none();
65+
3166
public SubsystemParsingTestCase() {
3267
super(KeycloakSamlExtension.SUBSYSTEM_NAME, new KeycloakSamlExtension());
3368
}
3469

3570
@Override
3671
protected String getSubsystemXml() throws IOException {
37-
return readResource("keycloak-saml-1.3.xml");
72+
return this.subsystemXml;
73+
}
74+
75+
@Before
76+
public void initialize() throws IOException {
77+
this.subsystemTemplate = readResource("keycloak-saml-1.3.xml");
78+
try {
79+
DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
80+
this.document = builder.parse(new InputSource(new StringReader(this.subsystemTemplate)));
81+
} catch (ParserConfigurationException | SAXException e) {
82+
throw new IOException(e);
83+
}
84+
}
85+
86+
private void buildSubsystemXml(final Element element, final String expression) throws IOException {
87+
if (element != null) {
88+
try {
89+
// locate the element and insert the node
90+
XPath xPath = XPathFactory.newInstance().newXPath();
91+
NodeList nodeList = (NodeList) xPath.compile(expression).evaluate(this.document, XPathConstants.NODESET);
92+
nodeList.item(0).appendChild(element);
93+
// transform again to XML
94+
TransformerFactory tf = TransformerFactory.newInstance();
95+
Transformer transformer = tf.newTransformer();
96+
transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes");
97+
StringWriter writer = new StringWriter();
98+
transformer.transform(new DOMSource(this.document), new StreamResult(writer));
99+
this.subsystemXml = writer.getBuffer().toString();
100+
} catch(TransformerException | XPathExpressionException e) {
101+
throw new IOException(e);
102+
}
103+
} else {
104+
this.subsystemXml = this.subsystemTemplate;
105+
}
106+
}
107+
108+
@Override
109+
public void testSubsystem() throws Exception {
110+
this.buildSubsystemXml(null, null);
111+
super.testSubsystem();
112+
}
113+
114+
@Test
115+
public void testDuplicateServiceProviders() throws Exception {
116+
// create a simple service provider element.
117+
Element spElement = this.document.createElement(Constants.XML.SERVICE_PROVIDER);
118+
spElement.setAttribute(Constants.XML.ENTITY_ID, "duplicate-sp");
119+
this.buildSubsystemXml(spElement, "/subsystem/secure-deployment[1]");
120+
121+
this.exception.expect(XMLStreamException.class);
122+
this.exception.expectMessage("JBAS014789: Unexpected element");
123+
super.testSubsystem();
124+
}
125+
126+
@Test
127+
public void testDuplicateIdentityProviders() throws Exception {
128+
// create a duplicate identity provider element.
129+
Element idpElement = this.document.createElement(Constants.XML.IDENTITY_PROVIDER);
130+
idpElement.setAttribute(Constants.XML.ENTITY_ID, "test-idp");
131+
Element singleSignOn = this.document.createElement(Constants.XML.SINGLE_SIGN_ON);
132+
singleSignOn.setAttribute(Constants.XML.BINDING_URL, "https://localhost:7887");
133+
Element singleLogout = this.document.createElement(Constants.XML.SINGLE_LOGOUT);
134+
singleLogout.setAttribute(Constants.XML.POST_BINDING_URL, "httpsL//localhost:8998");
135+
idpElement.appendChild(singleSignOn);
136+
idpElement.appendChild(singleLogout);
137+
this.buildSubsystemXml(idpElement, "/subsystem/secure-deployment[1]/SP");
138+
139+
this.exception.expect(XMLStreamException.class);
140+
this.exception.expectMessage("JBAS014789: Unexpected element");
141+
super.testSubsystem();
142+
}
143+
144+
@Test
145+
public void testDuplicateKeysInSP() throws Exception {
146+
Element keysElement = this.document.createElement(Constants.XML.KEYS);
147+
Element keyElement = this.document.createElement(Constants.XML.KEY);
148+
keyElement.setAttribute(Constants.XML.ENCRYPTION, "false");
149+
keyElement.setAttribute(Constants.XML.SIGNING, "false");
150+
keysElement.appendChild(keyElement);
151+
this.buildSubsystemXml(keysElement, "/subsystem/secure-deployment[1]/SP");
152+
153+
this.exception.expect(XMLStreamException.class);
154+
this.exception.expectMessage("JBAS014789: Unexpected element");
155+
super.testSubsystem();
156+
}
157+
158+
@Test
159+
public void testDuplicateKeysInIDP() throws Exception {
160+
Element keysElement = this.document.createElement(Constants.XML.KEYS);
161+
Element keyElement = this.document.createElement(Constants.XML.KEY);
162+
keyElement.setAttribute(Constants.XML.ENCRYPTION, "false");
163+
keyElement.setAttribute(Constants.XML.SIGNING, "false");
164+
keysElement.appendChild(keyElement);
165+
this.buildSubsystemXml(keysElement, "/subsystem/secure-deployment[1]/SP/IDP");
166+
167+
this.exception.expect(XMLStreamException.class);
168+
this.exception.expectMessage("JBAS014789: Unexpected element");
169+
super.testSubsystem();
38170
}
39171
}

‎adapters/saml/wildfly/wildfly-subsystem/src/main/java/org/keycloak/subsystem/adapter/saml/extension/Configuration.java

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
*/
1717
package org.keycloak.subsystem.adapter.saml.extension;
1818

19+
import java.util.List;
20+
21+
import org.jboss.as.controller.OperationFailedException;
1922
import org.jboss.as.server.deployment.DeploymentUnit;
2023
import org.jboss.as.web.common.WarMetaData;
2124
import org.jboss.dmr.ModelNode;
@@ -34,21 +37,29 @@ public class Configuration {
3437
private Configuration() {
3538
}
3639

37-
void updateModel(ModelNode operation, ModelNode model) {
40+
void updateModel(ModelNode operation, ModelNode model) throws OperationFailedException {
41+
this.updateModel(operation, model, false);
42+
}
43+
44+
void updateModel(final ModelNode operation, final ModelNode model, final boolean checkSingleton) throws OperationFailedException {
3845
ModelNode node = config;
39-
ModelNode addr = operation.get("address");
40-
for (Property item : addr.asPropertyList()) {
41-
node = getNodeForAddressElement(node, item);
46+
47+
final List<Property> addressNodes = operation.get("address").asPropertyList();
48+
final int lastIndex = addressNodes.size() - 1;
49+
for (int i = 0; i < addressNodes.size(); i++) {
50+
Property addressNode = addressNodes.get(i);
51+
// if checkSingleton is true, we verify if the key for the last element (e.g. SP or IDP) in the address path is already defined
52+
if (i == lastIndex && checkSingleton) {
53+
if (node.get(addressNode.getName()).isDefined()) {
54+
// found an existing resource, throw an exception
55+
throw new OperationFailedException("Duplicate resource: " + addressNode.getName());
56+
}
57+
}
58+
node = node.get(addressNode.getName()).get(addressNode.getValue().asString());
4259
}
4360
node.set(model);
4461
}
4562

46-
private ModelNode getNodeForAddressElement(ModelNode node, Property item) {
47-
String key = item.getValue().asString();
48-
ModelNode keymodel = node.get(item.getName());
49-
return keymodel.get(key);
50-
}
51-
5263
public ModelNode getSecureDeployment(DeploymentUnit deploymentUnit) {
5364
String name = preferredDeploymentName(deploymentUnit);
5465
ModelNode secureDeployment = config.get("subsystem").get("keycloak-saml").get(Constants.Model.SECURE_DEPLOYMENT);

‎adapters/saml/wildfly/wildfly-subsystem/src/main/java/org/keycloak/subsystem/adapter/saml/extension/IdentityProviderAddHandler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,6 @@ class IdentityProviderAddHandler extends AbstractAddStepHandler {
3232

3333
@Override
3434
protected void performRuntime(OperationContext context, ModelNode operation, ModelNode model) throws OperationFailedException {
35-
Configuration.INSTANCE.updateModel(operation, model);
35+
Configuration.INSTANCE.updateModel(operation, model, true);
3636
}
3737
}

0 commit comments

Comments
 (0)