Skip to content

Adding a deprecation info API check for saml nameid_format setting #77276

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Adding a deprecation info API check for saml nameid_format setting #77276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1ee1f4
Ading a check for saml nameid_format setting
masseyke Sep 3, 2021
5d49dde
Merge branch '7.x' into feature/deprecation-info-saml-name-id-format
elasticmachine Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ private DeprecationChecks() {
NodeDeprecationChecks::checkAcceptRolesCacheMaxSizeSetting,
NodeDeprecationChecks::checkRolesCacheTTLSizeSetting,
NodeDeprecationChecks::checkMaxLocalStorageNodesSetting,
NodeDeprecationChecks::checkSamlNameIdFormatSetting,
NodeDeprecationChecks::checkClusterRoutingAllocationIncludeRelocationsSetting
)
).collect(Collectors.toList());
Expand Down
33 changes: 33 additions & 0 deletions .../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import static org.elasticsearch.xpack.cluster.routing.allocation.DataTierAllocationDecider.CLUSTER_ROUTING_INCLUDE_SETTING;
import static org.elasticsearch.xpack.cluster.routing.allocation.DataTierAllocationDecider.CLUSTER_ROUTING_REQUIRE_SETTING;
import static org.elasticsearch.xpack.core.security.authc.RealmSettings.RESERVED_REALM_NAME_PREFIX;
import static org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings.PRINCIPAL_ATTRIBUTE;

class NodeDeprecationChecks {

Expand Down Expand Up @@ -998,4 +999,36 @@ static DeprecationIssue checkMaxLocalStorageNodesSetting(final Settings settings
DeprecationIssue.Level.CRITICAL
);
}

static DeprecationIssue checkSamlNameIdFormatSetting(final Settings settings,
final PluginsAndModules pluginsAndModules,
final ClusterState clusterState,
final XPackLicenseState licenseState) {
final String principalKeySuffix = ".attributes.principal";
List<String> detailsList =
PRINCIPAL_ATTRIBUTE.getAttribute().getAllConcreteSettings(settings).sorted(Comparator.comparing(Setting::getKey))
.map(concreteSamlPrincipalSetting -> {
String concreteSamlPrincipalSettingKey = concreteSamlPrincipalSetting.getKey();
int principalKeySuffixIndex = concreteSamlPrincipalSettingKey.indexOf(principalKeySuffix);
if (principalKeySuffixIndex > 0) {
String realm = concreteSamlPrincipalSettingKey.substring(0, principalKeySuffixIndex);
String concreteNameIdFormatSettingKey = realm + ".nameid_format";
if (settings.get(concreteNameIdFormatSettingKey) == null) {
return String.format(Locale.ROOT, "no value for [%s] set in realm [%s]",
concreteNameIdFormatSettingKey, realm);
}
}
return null;
})
.filter(detail -> detail != null).collect(Collectors.toList());
if (detailsList.isEmpty()) {
return null;
} else {
String message = "if nameid_format is not explicitly set, the previous default of " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we either need some more text here or a section in the breaking release notes and link from here ( I think the latter gives us more flexibility ). I can open a PR for the docs and reference the logs here once we have a link

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jkakavas -- just confirming that this is still on your to-do list, and you're not waiting on anything from me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the ping, you are right, this slipped my mind. I'll work on something today or tomorrow and get @lockewritesdocs to review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is it: #77785

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll go ahead and merge this, and update its URL along with several others in an upcoming PR.

"'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' is no longer used";
String url = "https://www.elastic.co/guide/en/elasticsearch/reference/master/saml-guide.html";
String details = detailsList.stream().collect(Collectors.joining(","));
return new DeprecationIssue(DeprecationIssue.Level.WARNING, message, url, details, false, null);
}
}
}
67 changes: 65 additions & 2 deletions ...ecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,8 +1271,6 @@ public void testCheckDelayClusterStateRecoverySettings() {
}

public void testCheckFixedAutoQueueSizeThreadpool() {
String settingKey = "thread_pool.search.min_queue_size";
String settingValue = "";
Settings settings = Settings.builder()
.put("thread_pool.search.min_queue_size", randomIntBetween(30, 100))
.put("thread_pool.search.max_queue_size", randomIntBetween(1, 25))
Expand Down Expand Up @@ -1432,4 +1430,69 @@ public void testCheckMaxLocalStorageNodesSetting() {
String url = "https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-8.0.html#breaking_80_node_changes";
checkSimpleSetting(settingKey, settingValue, url, NodeDeprecationChecks::checkMaxLocalStorageNodesSetting);
}

public void testCheckSamlNameIdFormatSetting() {
Settings settings = Settings.builder()
.put("xpack.security.authc.realms.saml.saml1.attributes.principal", randomIntBetween(30, 100))
.put("xpack.security.authc.realms.saml.saml1.nameid_format", randomIntBetween(1, 25))
.build();
final ClusterState clusterState = ClusterState.EMPTY_STATE;
final XPackLicenseState licenseState = mock(XPackLicenseState.class);
when(licenseState.getOperationMode())
.thenReturn(randomValueOtherThanMany((m -> m.equals(License.OperationMode.BASIC) || m.equals(License.OperationMode.TRIAL)),
() -> randomFrom(License.OperationMode.values())));
assertThat(
NodeDeprecationChecks.checkSamlNameIdFormatSetting(settings, null, clusterState, licenseState),
equalTo(null)
);

settings = Settings.builder()
.put("xpack.security.authc.realms.saml.saml1.attributes.principal", randomIntBetween(30, 100))
.build();
DeprecationIssue expectedIssue = new DeprecationIssue(DeprecationIssue.Level.WARNING,
"if nameid_format is not explicitly set, the previous default of 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' is no " +
"longer used",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/saml-guide.html",
"no value for [xpack.security.authc.realms.saml.saml1.nameid_format] set in realm [xpack.security.authc.realms.saml.saml1]",
false, null
);
assertThat(
NodeDeprecationChecks.checkSamlNameIdFormatSetting(settings, null, clusterState, licenseState),
equalTo(expectedIssue)
);

settings = Settings.builder()
.put("xpack.security.authc.realms.saml.saml1.attributes.principal", randomIntBetween(30, 100))
.put("xpack.security.authc.realms.saml.saml2.attributes.principal", randomIntBetween(30, 100))
.put("xpack.security.authc.realms.saml.saml2.nameid_format", randomIntBetween(1, 25))
.build();
expectedIssue = new DeprecationIssue(DeprecationIssue.Level.WARNING,
"if nameid_format is not explicitly set, the previous default of 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' is no " +
"longer used",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/saml-guide.html",
"no value for [xpack.security.authc.realms.saml.saml1.nameid_format] set in realm [xpack.security.authc.realms.saml.saml1]",
false, null
);
assertThat(
NodeDeprecationChecks.checkSamlNameIdFormatSetting(settings, null, clusterState, licenseState),
equalTo(expectedIssue)
);

settings = Settings.builder()
.put("xpack.security.authc.realms.saml.saml1.attributes.principal", randomIntBetween(30, 100))
.put("xpack.security.authc.realms.saml.saml2.attributes.principal", randomIntBetween(30, 100))
.build();
expectedIssue = new DeprecationIssue(DeprecationIssue.Level.WARNING,
"if nameid_format is not explicitly set, the previous default of 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' is no " +
"longer used",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/saml-guide.html",
"no value for [xpack.security.authc.realms.saml.saml1.nameid_format] set in realm [xpack.security.authc.realms.saml.saml1]," +
"no value for [xpack.security.authc.realms.saml.saml2.nameid_format] set in realm [xpack.security.authc.realms.saml.saml2]",
false, null
);
assertThat(
NodeDeprecationChecks.checkSamlNameIdFormatSetting(settings, null, clusterState, licenseState),
equalTo(expectedIssue)
);
}
}