Skip to content

Commit ea9e6a5

Browse files
tvernumtvernum
committed
Add "extension" attribute validation to IdP SPs
This extends the change from elastic#128176 to validate the "custom attributes" on a per Service Provider basis. Each Service Provider (whether registered or wildcard based) has a field "attributes.extensions" which is a list of attribute names that may be provided by the caller of "/_idp/saml/init". Service Providers that have not be configured with extension attributes will reject any custom attributes in SAML init. This necessitates a new field in the service provider index. The template has been updated, but there is no data migration because the `saml-service-provider` index does not exist in any of the environments into which we wish to deploy this change.
1 parent 70368c2 commit ea9e6a5

File tree

11 files changed

+136
-16
lines changed

11 files changed

+136
-16
lines changed

‎server/src/main/java/org/elasticsearch/TransportVersions.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ static TransportVersion def(int id) {
275275
public static final TransportVersion ESQL_LIMIT_ROW_SIZE = def(9_085_0_00);
276276
public static final TransportVersion ESQL_REGEX_MATCH_WITH_CASE_INSENSITIVITY = def(9_086_0_00);
277277
public static final TransportVersion IDP_CUSTOM_SAML_ATTRIBUTES = def(9_087_0_00);
278+
public static final TransportVersion IDP_CUSTOM_SAML_ATTRIBUTES_ALLOW_LIST = def(9_088_0_00);
278279

279280
/*
280281
* STOP! READ THIS FIRST! No, really,

‎x-pack/plugin/core/template-resources/src/main/resources/idp/saml-service-provider-template.json

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@
7575
},
7676
"roles": {
7777
"type": "keyword"
78+
},
79+
"extensions": {
80+
"type": "keyword"
7881
}
7982
}
8083
},

‎x-pack/plugin/identity-provider/qa/idp-rest-tests/src/javaRestTest/java/org/elasticsearch/xpack/idp/IdentityProviderAuthenticationIT.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,8 @@ public void testCustomAttributesInIdpInitiatedSso() throws Exception {
100100
Map.entry("principal", "https://idp.test.es.elasticsearch.org/attribute/principal"),
101101
Map.entry("name", "https://idp.test.es.elasticsearch.org/attribute/name"),
102102
Map.entry("email", "https://idp.test.es.elasticsearch.org/attribute/email"),
103-
Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles")
103+
Map.entry("roles", "https://idp.test.es.elasticsearch.org/attribute/roles"),
104+
Map.entry("extensions", List.of("department", "region"))
104105
)
105106
)
106107
);

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilder.java

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import java.util.Map;
4949
import java.util.Set;
5050

51+
import static org.elasticsearch.common.Strings.collectionToCommaDelimitedString;
5152
import static org.opensaml.saml.saml2.core.NameIDType.TRANSIENT;
5253

5354
/**
@@ -214,28 +215,42 @@ private AttributeStatement buildAttributes(
214215
final SamlServiceProvider serviceProvider = user.getServiceProvider();
215216
final AttributeStatement statement = samlFactory.object(AttributeStatement.class, AttributeStatement.DEFAULT_ELEMENT_NAME);
216217
final List<Attribute> attributes = new ArrayList<>();
217-
final Attribute roles = buildAttribute(serviceProvider.getAttributeNames().roles, "roles", user.getRoles());
218+
final SamlServiceProvider.AttributeNames attributeNames = serviceProvider.getAttributeNames();
219+
final Attribute roles = buildAttribute(attributeNames.roles, "roles", user.getRoles());
218220
if (roles != null) {
219221
attributes.add(roles);
220222
}
221-
final Attribute principal = buildAttribute(serviceProvider.getAttributeNames().principal, "principal", user.getPrincipal());
223+
final Attribute principal = buildAttribute(attributeNames.principal, "principal", user.getPrincipal());
222224
if (principal != null) {
223225
attributes.add(principal);
224226
}
225-
final Attribute email = buildAttribute(serviceProvider.getAttributeNames().email, "email", user.getEmail());
227+
final Attribute email = buildAttribute(attributeNames.email, "email", user.getEmail());
226228
if (email != null) {
227229
attributes.add(email);
228230
}
229-
final Attribute name = buildAttribute(serviceProvider.getAttributeNames().name, "name", user.getName());
231+
final Attribute name = buildAttribute(attributeNames.name, "name", user.getName());
230232
if (name != null) {
231233
attributes.add(name);
232234
}
233235
// Add custom attributes if provided
234236
if (customAttributes != null && customAttributes.getAttributes().isEmpty() == false) {
235237
for (Map.Entry<String, List<String>> entry : customAttributes.getAttributes().entrySet()) {
236-
Attribute attribute = buildAttribute(entry.getKey(), null, entry.getValue());
237-
if (attribute != null) {
238-
attributes.add(attribute);
238+
final String attributeName = entry.getKey();
239+
if (attributeNames.isAllowedExtension(attributeName)) {
240+
Attribute attribute = buildAttribute(attributeName, null, entry.getValue());
241+
if (attribute != null) {
242+
attributes.add(attribute);
243+
}
244+
} else {
245+
throw new IllegalArgumentException(
246+
"the custom attribute ["
247+
+ attributeName
248+
+ "] is not permitted for the Service Provider ["
249+
+ serviceProvider.getName()
250+
+ "], allowed attribute names are ["
251+
+ collectionToCommaDelimitedString(attributeNames.allowedExtensions)
252+
+ "]"
253+
);
239254
}
240255
}
241256
}

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProvider.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,18 @@ class AttributeNames {
3636
public final String name;
3737
public final String email;
3838
public final String roles;
39+
public final Set<String> allowedExtensions;
3940

40-
public AttributeNames(String principal, String name, String email, String roles) {
41+
public AttributeNames(String principal, String name, String email, String roles, Set<String> allowedExtensions) {
4142
this.principal = principal;
4243
this.name = name;
4344
this.email = email;
4445
this.roles = roles;
46+
this.allowedExtensions = allowedExtensions == null ? Set.of() : Set.copyOf(allowedExtensions);
47+
}
48+
49+
public boolean isAllowedExtension(String attributeName) {
50+
return this.allowedExtensions.contains(attributeName);
4551
}
4652
}
4753

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderDocument.java

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@
4242
import java.util.TreeSet;
4343
import java.util.function.BiConsumer;
4444

45+
import static org.elasticsearch.TransportVersions.IDP_CUSTOM_SAML_ATTRIBUTES_ALLOW_LIST;
46+
4547
/**
4648
* This class models the storage of a {@link SamlServiceProvider} as an Elasticsearch document.
4749
*/
@@ -87,6 +89,12 @@ public static class AttributeNames {
8789
@Nullable
8890
public String roles;
8991

92+
/**
93+
* Extensions are attributes that are provided at runtime (by the trusted client that initiates
94+
* the SAML SSO. They are sourced from the rest request rather than the user object itself.
95+
*/
96+
public Set<String> extensions = Set.of();
97+
9098
public void setPrincipal(String principal) {
9199
this.principal = principal;
92100
}
@@ -103,6 +111,10 @@ public void setRoles(String roles) {
103111
this.roles = roles;
104112
}
105113

114+
public void setExtensions(Collection<String> names) {
115+
this.extensions = names == null ? Set.of() : Set.copyOf(names);
116+
}
117+
106118
@Override
107119
public boolean equals(Object o) {
108120
if (this == o) return true;
@@ -111,7 +123,8 @@ public boolean equals(Object o) {
111123
return Objects.equals(principal, that.principal)
112124
&& Objects.equals(email, that.email)
113125
&& Objects.equals(name, that.name)
114-
&& Objects.equals(roles, that.roles);
126+
&& Objects.equals(roles, that.roles)
127+
&& Objects.equals(extensions, that.extensions);
115128
}
116129

117130
@Override
@@ -263,6 +276,10 @@ public SamlServiceProviderDocument(StreamInput in) throws IOException {
263276
attributeNames.name = in.readOptionalString();
264277
attributeNames.roles = in.readOptionalString();
265278

279+
if (in.getTransportVersion().onOrAfter(IDP_CUSTOM_SAML_ATTRIBUTES_ALLOW_LIST)) {
280+
attributeNames.extensions = in.readCollectionAsImmutableSet(StreamInput::readString);
281+
}
282+
266283
certificates.serviceProviderSigning = in.readStringCollectionAsList();
267284
certificates.identityProviderSigning = in.readStringCollectionAsList();
268285
certificates.identityProviderMetadataSigning = in.readStringCollectionAsList();
@@ -288,6 +305,10 @@ public void writeTo(StreamOutput out) throws IOException {
288305
out.writeOptionalString(attributeNames.name);
289306
out.writeOptionalString(attributeNames.roles);
290307

308+
if (out.getTransportVersion().onOrAfter(IDP_CUSTOM_SAML_ATTRIBUTES_ALLOW_LIST)) {
309+
out.writeStringCollection(attributeNames.extensions);
310+
}
311+
291312
out.writeStringCollection(certificates.serviceProviderSigning);
292313
out.writeStringCollection(certificates.identityProviderSigning);
293314
out.writeStringCollection(certificates.identityProviderMetadataSigning);
@@ -426,6 +447,7 @@ public int hashCode() {
426447
ATTRIBUTES_PARSER.declareStringOrNull(AttributeNames::setEmail, Fields.Attributes.EMAIL);
427448
ATTRIBUTES_PARSER.declareStringOrNull(AttributeNames::setName, Fields.Attributes.NAME);
428449
ATTRIBUTES_PARSER.declareStringOrNull(AttributeNames::setRoles, Fields.Attributes.ROLES);
450+
ATTRIBUTES_PARSER.declareStringArray(AttributeNames::setExtensions, Fields.Attributes.EXTENSIONS);
429451

430452
DOC_PARSER.declareObject(NULL_CONSUMER, (p, doc) -> CERTIFICATES_PARSER.parse(p, doc.certificates, null), Fields.CERTIFICATES);
431453
CERTIFICATES_PARSER.declareStringArray(Certificates::setServiceProviderSigning, Fields.Certificates.SP_SIGNING);
@@ -516,6 +538,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
516538
builder.field(Fields.Attributes.EMAIL.getPreferredName(), attributeNames.email);
517539
builder.field(Fields.Attributes.NAME.getPreferredName(), attributeNames.name);
518540
builder.field(Fields.Attributes.ROLES.getPreferredName(), attributeNames.roles);
541+
builder.field(Fields.Attributes.EXTENSIONS.getPreferredName(), attributeNames.extensions);
519542
builder.endObject();
520543

521544
builder.startObject(Fields.CERTIFICATES.getPreferredName());
@@ -553,6 +576,7 @@ interface Attributes {
553576
ParseField EMAIL = new ParseField("email");
554577
ParseField NAME = new ParseField("name");
555578
ParseField ROLES = new ParseField("roles");
579+
ParseField EXTENSIONS = new ParseField("extensions");
556580
}
557581

558582
interface Certificates {

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderFactory.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ SamlServiceProvider buildServiceProvider(SamlServiceProviderDocument document) {
3838
document.attributeNames.principal,
3939
document.attributeNames.name,
4040
document.attributeNames.email,
41-
document.attributeNames.roles
41+
document.attributeNames.roles,
42+
document.attributeNames.extensions
4243
);
4344
final Set<X509Credential> credentials = document.certificates.getServiceProviderX509SigningCertificates()
4445
.stream()

‎x-pack/plugin/identity-provider/src/main/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndex.java

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,26 @@ public class SamlServiceProviderIndex implements Closeable {
7878
static final String TEMPLATE_VERSION_STRING_DEPRECATED = "idp.template.version_deprecated";
7979
static final String FINAL_TEMPLATE_VERSION_STRING_DEPRECATED = "8.14.0";
8080

81-
static final int CURRENT_TEMPLATE_VERSION = 1;
81+
/**
82+
* The object in the index mapping metadata that contains a version field
83+
*/
84+
private static final String INDEX_META_FIELD = "_meta";
85+
/**
86+
* The field in the {@link #INDEX_META_FIELD} metadata that contains the version number
87+
*/
88+
private static final String TEMPLATE_VERSION_META_FIELD = "idp-template-version";
89+
90+
/**
91+
* The first version of this template (since it was moved to use {@link org.elasticsearch.xpack.core.template.IndexTemplateRegistry}
92+
*/
93+
private static final int VERSION_ORIGINAL = 1;
94+
/**
95+
* The version that added the {@code attributes.extensions} field to the SAML SP document
96+
*/
97+
private static final int VERSION_EXTENSION_ATTRIBUTES = 2;
98+
static final int CURRENT_TEMPLATE_VERSION = VERSION_EXTENSION_ATTRIBUTES;
99+
100+
private volatile boolean indexUpToDate = false;
82101

83102
public static final class DocumentVersion {
84103
public final String id;

‎x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/action/TransportSamlInitiateSingleSignOnActionTests.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,8 @@ private TransportSamlInitiateSingleSignOnAction setupTransportAction(boolean wit
190190
"https://saml.elasticsearch.org/attributes/principal",
191191
"https://saml.elasticsearch.org/attributes/name",
192192
"https://saml.elasticsearch.org/attributes/email",
193-
"https://saml.elasticsearch.org/attributes/roles"
193+
"https://saml.elasticsearch.org/attributes/roles",
194+
Set.of()
194195
),
195196
null,
196197
false,

‎x-pack/plugin/identity-provider/src/test/java/org/elasticsearch/xpack/idp/saml/authn/SuccessfulAuthenticationResponseMessageBuilderTests.java

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
package org.elasticsearch.xpack.idp.saml.authn;
99

10+
import org.elasticsearch.core.Nullable;
1011
import org.elasticsearch.xpack.idp.saml.idp.SamlIdentityProvider;
1112
import org.elasticsearch.xpack.idp.saml.sp.SamlServiceProvider;
1213
import org.elasticsearch.xpack.idp.saml.sp.ServiceProviderDefaults;
@@ -103,7 +104,46 @@ public void testSignedResponseWithCustomAttributes() throws Exception {
103104
assertTrue("Custom attribute 'customAttr2' not found in SAML response", foundCustomAttr2);
104105
}
105106

106-
private Response buildResponse(SamlInitiateSingleSignOnAttributes customAttributes) throws Exception {
107+
public void testRejectInvalidCustomAttributes() throws Exception {
108+
final var customAttributes = new SamlInitiateSingleSignOnAttributes(
109+
Map.of("https://idp.example.org/attribute/department", Collections.singletonList("engineering"))
110+
);
111+
112+
// Build response with custom attributes
113+
final IllegalArgumentException ex = expectThrows(
114+
IllegalArgumentException.class,
115+
() -> buildResponse(
116+
new SamlServiceProvider.AttributeNames(
117+
"https://idp.example.org/attribute/principal",
118+
null,
119+
null,
120+
null,
121+
Set.of("https://idp.example.org/attribute/avatar")
122+
),
123+
customAttributes
124+
)
125+
);
126+
assertThat(ex.getMessage(), containsString("custom attribute [https://idp.example.org/attribute/department]"));
127+
assertThat(ex.getMessage(), containsString("allowed attribute names are [https://idp.example.org/attribute/avatar]"));
128+
}
129+
130+
private Response buildResponse(@Nullable SamlInitiateSingleSignOnAttributes customAttributes) throws Exception {
131+
return buildResponse(
132+
new SamlServiceProvider.AttributeNames(
133+
"principal",
134+
null,
135+
null,
136+
null,
137+
customAttributes == null ? Set.of() : customAttributes.getAttributes().keySet()
138+
),
139+
customAttributes
140+
);
141+
}
142+
143+
private Response buildResponse(
144+
final SamlServiceProvider.AttributeNames attributes,
145+
@Nullable SamlInitiateSingleSignOnAttributes customAttributes
146+
) throws Exception {
107147
final Clock clock = Clock.systemUTC();
108148

109149
final SamlServiceProvider sp = mock(SamlServiceProvider.class);
@@ -112,7 +152,7 @@ private Response buildResponse(SamlInitiateSingleSignOnAttributes customAttribut
112152
when(sp.getEntityId()).thenReturn(baseServiceUrl);
113153
when(sp.getAssertionConsumerService()).thenReturn(URI.create(acs).toURL());
114154
when(sp.getAuthnExpiry()).thenReturn(Duration.ofMinutes(10));
115-
when(sp.getAttributeNames()).thenReturn(new SamlServiceProvider.AttributeNames("principal", null, null, null));
155+
when(sp.getAttributeNames()).thenReturn(attributes);
116156

117157
final UserServiceAuthentication user = mock(UserServiceAuthentication.class);
118158
when(user.getPrincipal()).thenReturn(randomAlphaOfLengthBetween(4, 12));

0 commit comments

Comments
 (0)