Skip to content
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.

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

Expose cluster-state role mappings in APIs #114951

Merged
merged 43 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
492837d
WIP
n1v0lg Oct 15, 2024
e8e6b05
Clean up
n1v0lg Oct 16, 2024
78aaec2
Merge
n1v0lg Oct 16, 2024
d85b0d7
More
n1v0lg Oct 16, 2024
fe846e2
Tweaks
n1v0lg Oct 16, 2024
ba1531e
Remove sout
n1v0lg Oct 16, 2024
c63e6b0
Better logs
n1v0lg Oct 16, 2024
02cb783
Nits
n1v0lg Oct 16, 2024
bf80273
Still not it...
n1v0lg Oct 17, 2024
e512004
Maybe
n1v0lg Oct 17, 2024
5cab323
API
n1v0lg Oct 17, 2024
ba07fd2
Tests
n1v0lg Oct 18, 2024
08f8866
Tests and more clean up
n1v0lg Oct 18, 2024
7ea2c1b
More
n1v0lg Oct 18, 2024
0242b70
More
n1v0lg Oct 18, 2024
4519ddd
Javadoc
n1v0lg Oct 18, 2024
5fb90de
Test fallback
n1v0lg Oct 18, 2024
363ab47
Merge branch 'main' into cluster-state-role-mapping
n1v0lg Oct 18, 2024
fd3cb5a
Header warnings
n1v0lg Oct 18, 2024
d79f473
WIP
n1v0lg Oct 18, 2024
8ae8492
Spaces too painful
n1v0lg Oct 18, 2024
9c2cba5
Tests
n1v0lg Oct 19, 2024
74c5674
Rename
n1v0lg Oct 19, 2024
4e77c66
Merge branch 'main' into cluster-state-role-mapping
n1v0lg Oct 21, 2024
c4c3a48
REST its
n1v0lg Oct 21, 2024
6ea654f
Merge
n1v0lg Oct 21, 2024
b7a676f
More clean up
n1v0lg Oct 21, 2024
3c1064f
More tests
n1v0lg Oct 21, 2024
d9f15c4
Nit
n1v0lg Oct 21, 2024
eb8a9c5
Moar tests
n1v0lg Oct 21, 2024
d5be5a1
Fix test
n1v0lg Oct 21, 2024
fc11ea1
Get tests
n1v0lg Oct 21, 2024
01213ff
Tests and javadoc
n1v0lg Oct 21, 2024
4be17a8
Upgrade
n1v0lg Oct 21, 2024
6f63d22
Merge branch 'main' into cluster-state-role-mapping
n1v0lg Oct 21, 2024
f0f2f98
Test nit
n1v0lg Oct 21, 2024
8136af1
Merge branch 'main' into cluster-state-role-mapping
elasticmachine Oct 21, 2024
de5cf69
WIP address review feedback
n1v0lg Oct 22, 2024
d5fee36
Merge branch 'cluster-state-role-mapping' of github.com:n1v0lg/elasti…
n1v0lg Oct 22, 2024
57f53aa
Fix test
n1v0lg Oct 22, 2024
05f5f55
Merge branch 'main' into cluster-state-role-mapping
n1v0lg Oct 22, 2024
b1b8ad7
Merge branch 'main' into cluster-state-role-mapping
elasticmachine Oct 22, 2024
e7cd5a8
Update docs/changelog/114951.yaml
n1v0lg Oct 22, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@

import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
Expand Down Expand Up @@ -106,6 +109,10 @@ public void testRoleMappingsAppliedOnUpgrade() throws IOException {
);
assertThat(roleMappings, is(not(nullValue())));
assertThat(roleMappings.size(), equalTo(1));
assertThat(roleMappings, is(instanceOf(Map.class)));
@SuppressWarnings("unchecked")
Map<String, Object> roleMapping = (Map<String, Object>) roleMappings;
assertThat(roleMapping.keySet(), contains("everyone_kibana"));
n1v0lg marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,8 @@ static boolean checkMetadataVersion(
namespace,
newVersion,
switch (versionCheck) {
case ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION -> "less than";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Backport PRs fail compilation when the switch-case uses a qualified value; fixing it here to stay consistent with older branches. I did not dig into why this is not an issue for main (the PR merged and passed CI just fine).

case ReservedStateVersionCheck.HIGHER_VERSION_ONLY -> "less than or equal to";
case HIGHER_OR_SAME_VERSION -> "less than";
case HIGHER_VERSION_ONLY -> "less than or equal to";
},
currentVersion
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.core.security.authc.support.mapper;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.xcontent.XContentParser;

import java.io.IOException;
import java.util.HashMap;
import java.util.Map;

/**
* Helper class to avoid name erasure when cluster-state role mappings on disk.
* When cluster-state role mappings are written to disk, their XContent format is used.
* This format omits the role mapping's name.
* This means that if CS role mappings are ever recovered from disk (e.g., during a master-node restart), their names are erased.
* To address this, this class augments CS role mapping serialization to persist the name of a mapping in a reserved metadata field,
* and recover it from metadata during de-serialization.
* Storing the name in metadata allows us to persist the name without BWC-breaks in role mapping `XContent` format (as opposed to changing
* XContent serialization to persist the top-level name field).
* It also ensures that role mappings are re-written to cluster state in the new,
* name-preserving format the first time operator file settings are processed.
* {@link #copyWithNameInMetadata(ExpressionRoleMapping)}} is used to copy the name of a role mapping into its metadata field.
* This is called when the role mappings are read from operator file settings (at which point the correct name is still available).
* {@link #parseWithNameFromMetadata(XContentParser)} is used to parse a role mapping from XContent restoring the name from metadata.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that everything related to this fix is in one class, but I think it becomes a little hard to follow what's going on and it would be nice to not have to do a helper class if it's not needed. A short comment for each of the methods and putting them in the class where they're used would be better.

I also think the constants belong in RoleMappingMetadata since that class is already responsible for representing a role mapping as xcontent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! I'll move the code into RoleMappingMetadata

Copy link
Contributor Author

@n1v0lg n1v0lg Oct 22, 2024

Choose a reason for hiding this comment

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

I do want to keep copyWithNameInMetadata and parseWithNameFromMetadata in the same class though since they really only make sense together. Let me know if you have strong objections.

public final class ReservedRoleMappingXContentNameFieldHelper {
private static final Logger logger = LogManager.getLogger(ReservedRoleMappingXContentNameFieldHelper.class);

public static final String METADATA_NAME_FIELD = "_es_reserved_role_mapping_name";
public static final String FALLBACK_NAME = "name_not_available_after_deserialization";

private ReservedRoleMappingXContentNameFieldHelper() {}

public static ExpressionRoleMapping copyWithNameInMetadata(ExpressionRoleMapping roleMapping) {
Map<String, Object> metadata = new HashMap<>(roleMapping.getMetadata());
// note: can't use Maps.copyWith... since these create maps that don't support `null` values in map entries
if (metadata.put(METADATA_NAME_FIELD, roleMapping.getName()) != null) {
logger.error(
"Metadata field [{}] is reserved and will be overwritten with an internal system value. "
+ "Rename this field in your role mapping configuration.",
METADATA_NAME_FIELD
);
}
return new ExpressionRoleMapping(
roleMapping.getName(),
roleMapping.getExpression(),
roleMapping.getRoles(),
roleMapping.getRoleTemplates(),
metadata,
roleMapping.isEnabled()
);
}

public static boolean hasFallbackName(ExpressionRoleMapping expressionRoleMapping) {
return expressionRoleMapping.getName().equals(FALLBACK_NAME);
}

public static void removeNameFromMetadata(Map<String, Object> metadata) {
n1v0lg marked this conversation as resolved.
Show resolved Hide resolved
assert metadata instanceof HashMap<String, Object>;
metadata.remove(METADATA_NAME_FIELD);
}

public static ExpressionRoleMapping parseWithNameFromMetadata(XContentParser parser) throws IOException {
n1v0lg marked this conversation as resolved.
Show resolved Hide resolved
ExpressionRoleMapping roleMapping = ExpressionRoleMapping.parse(FALLBACK_NAME, parser);
return new ExpressionRoleMapping(
getNameFromMetadata(roleMapping),
roleMapping.getExpression(),
roleMapping.getRoles(),
roleMapping.getRoleTemplates(),
roleMapping.getMetadata(),
roleMapping.isEnabled()
);
}

private static String getNameFromMetadata(ExpressionRoleMapping roleMapping) {
Map<String, Object> metadata = roleMapping.getMetadata();
if (metadata.containsKey(METADATA_NAME_FIELD) && metadata.get(METADATA_NAME_FIELD) instanceof String name) {
return name;
} else {
// This is valid the first time we recover from cluster-state: the old format metadata won't have a name stored in metadata yet
n1v0lg marked this conversation as resolved.
Show resolved Hide resolved
return FALLBACK_NAME;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
import org.elasticsearch.xpack.core.security.authc.support.mapper.ReservedRoleMappingXContentNameFieldHelper;

import java.io.IOException;
import java.util.Collection;
Expand All @@ -48,8 +49,7 @@ public final class RoleMappingMetadata extends AbstractNamedDiffable<Metadata.Cu
static {
PARSER.declareObjectArray(
constructorArg(),
// role mapping names are lost when the role mapping metadata is serialized
(p, c) -> ExpressionRoleMapping.parse("name_not_available_after_deserialization", p),
(p, c) -> ReservedRoleMappingXContentNameFieldHelper.parseWithNameFromMetadata(p),
new ParseField(TYPE)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.core.Tuple;
import org.elasticsearch.test.TestSecurityClient;
import org.elasticsearch.test.cluster.ElasticsearchCluster;
import org.elasticsearch.test.cluster.local.LocalClusterConfigProvider;
import org.elasticsearch.test.cluster.local.distribution.DistributionType;
import org.elasticsearch.test.cluster.util.resource.Resource;
import org.elasticsearch.test.rest.ESRestTestCase;
Expand All @@ -41,9 +42,7 @@
public abstract class SecurityOnTrialLicenseRestTestCase extends ESRestTestCase {
private TestSecurityClient securityClient;

@ClassRule
public static ElasticsearchCluster cluster = ElasticsearchCluster.local()
.nodes(2)
public static LocalClusterConfigProvider commonTrialSecurityClusterConfig = cluster -> cluster.nodes(2)
.distribution(DistributionType.DEFAULT)
.setting("xpack.ml.enabled", "false")
.setting("xpack.license.self_generated.type", "trial")
Expand All @@ -62,8 +61,10 @@ public abstract class SecurityOnTrialLicenseRestTestCase extends ESRestTestCase
.user("admin_user", "admin-password", ROOT_USER_ROLE, true)
.user("security_test_user", "security-test-password", "security_test_role", false)
.user("x_pack_rest_user", "x-pack-test-password", ROOT_USER_ROLE, true)
.user("cat_test_user", "cat-test-password", "cat_test_role", false)
.build();
.user("cat_test_user", "cat-test-password", "cat_test_role", false);

@ClassRule
public static ElasticsearchCluster cluster = ElasticsearchCluster.local().apply(commonTrialSecurityClusterConfig).build();

@Override
protected String getTestRestCluster() {
Expand Down
Loading