Skip to content

Commit

Permalink
Fix so non super users can also create API keys (#40028) (#40288)
Browse files Browse the repository at this point in the history 
When creating API keys we check for if API key with
the same key name already exists and fail the request if it does.
The check should have been performed with XPackSecurityUser
instead of the authenticated user. This caused the request to fail
in case of the non-super user trying to create an API key.
This commit fixes by executing search action with SECURITY_ORIGIN
so it can be executed with XPackSecurityUser.
Also fixed the Rest test to avoid using a user with `super_user` role.

Closes #40029
  • Loading branch information
@bizybot
bizybot authored Mar 21, 2019
1 parent e9ab087 commit 145e10b
Show file tree
Hide file tree
Showing 2 changed files with 139 additions and 84 deletions.
183 changes: 108 additions & 75 deletions ...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.action.index.IndexAction;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.search.SearchAction;
import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.action.support.WriteRequest.RefreshPolicy;
import org.elasticsearch.action.update.UpdateRequest;
Expand Down Expand Up @@ -94,9 +95,11 @@

import javax.crypto.SecretKeyFactory;

import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME;
import static org.elasticsearch.search.SearchService.DEFAULT_KEEPALIVE_SETTING;
import static org.elasticsearch.xpack.core.ClientHelper.SECURITY_ORIGIN;
import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_INDEX_NAME;

public class ApiKeyService {

Expand Down Expand Up @@ -190,85 +193,115 @@ public void createApiKey(Authentication authentication, CreateApiKeyRequest requ
* this check is best effort as there could be two nodes executing search and
* then index concurrently allowing a duplicate name.
*/
findApiKeyForApiKeyName(request.getName(), true, true, ActionListener.wrap(apiKeyIds -> {
if (apiKeyIds.isEmpty()) {
final Instant created = clock.instant();
final Instant expiration = getApiKeyExpiration(created, request);
final SecureString apiKey = UUIDs.randomBase64UUIDSecureString();
final Version version = clusterService.state().nodes().getMinNodeVersion();
if (version.before(Version.V_6_7_0)) {
logger.warn(
"nodes prior to the minimum supported version for api keys {} exist in the cluster;"
+ " these nodes will not be able to use api keys",
Version.V_6_7_0);
}

final char[] keyHash = hasher.hash(apiKey);
try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
builder.startObject()
.field("doc_type", "api_key")
.field("creation_time", created.toEpochMilli())
.field("expiration_time", expiration == null ? null : expiration.toEpochMilli())
.field("api_key_invalidated", false);

byte[] utf8Bytes = null;
try {
utf8Bytes = CharArrays.toUtf8Bytes(keyHash);
builder.field("api_key_hash").utf8Value(utf8Bytes, 0, utf8Bytes.length);
} finally {
if (utf8Bytes != null) {
Arrays.fill(utf8Bytes, (byte) 0);
}
}
checkDuplicateApiKeyNameAndCreateApiKey(authentication, request, roleDescriptorSet, listener);
}
}

// Save role_descriptors
builder.startObject("role_descriptors");
if (request.getRoleDescriptors() != null && request.getRoleDescriptors().isEmpty() == false) {
for (RoleDescriptor descriptor : request.getRoleDescriptors()) {
builder.field(descriptor.getName(),
(contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true));
private void checkDuplicateApiKeyNameAndCreateApiKey(Authentication authentication, CreateApiKeyRequest request,
Set<RoleDescriptor> roleDescriptorSet,
ActionListener<CreateApiKeyResponse> listener) {
final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery()
.filter(QueryBuilders.termQuery("doc_type", "api_key"))
.filter(QueryBuilders.termQuery("name", request.getName()))
.filter(QueryBuilders.termQuery("api_key_invalidated", false));
final BoolQueryBuilder expiredQuery = QueryBuilders.boolQuery()
.should(QueryBuilders.rangeQuery("expiration_time").lte(Instant.now().toEpochMilli()))
.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time")));
boolQuery.filter(expiredQuery);

final SearchRequest searchRequest = client.prepareSearch(SECURITY_INDEX_NAME)
.setScroll(DEFAULT_KEEPALIVE_SETTING.get(settings))
.setQuery(boolQuery)
.setVersion(false)
.setSize(1)
.request();
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
executeAsyncWithOrigin(client, SECURITY_ORIGIN, SearchAction.INSTANCE, searchRequest,
ActionListener.wrap(
indexResponse -> {
if (indexResponse.getHits().getTotalHits() > 0) {
listener.onFailure(traceLog("create api key", new ElasticsearchSecurityException(
"Error creating api key as api key with name [{}] already exists", request.getName())));
} else {
createApiKeyAndIndexIt(authentication, request, roleDescriptorSet, listener);
}
}
builder.endObject();
},
listener::onFailure)));
}

private void createApiKeyAndIndexIt(Authentication authentication, CreateApiKeyRequest request, Set<RoleDescriptor> roleDescriptorSet,
ActionListener<CreateApiKeyResponse> listener) {
final Instant created = clock.instant();
final Instant expiration = getApiKeyExpiration(created, request);
final SecureString apiKey = UUIDs.randomBase64UUIDSecureString();
final Version version = clusterService.state().nodes().getMinNodeVersion();
if (version.before(Version.V_6_7_0)) {
logger.warn(
"nodes prior to the minimum supported version for api keys {} exist in the cluster;"
+ " these nodes will not be able to use api keys",
Version.V_6_7_0);
}

final char[] keyHash = hasher.hash(apiKey);
try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
builder.startObject()
.field("doc_type", "api_key")
.field("creation_time", created.toEpochMilli())
.field("expiration_time", expiration == null ? null : expiration.toEpochMilli())
.field("api_key_invalidated", false);

byte[] utf8Bytes = null;
try {
utf8Bytes = CharArrays.toUtf8Bytes(keyHash);
builder.field("api_key_hash").utf8Value(utf8Bytes, 0, utf8Bytes.length);
} finally {
if (utf8Bytes != null) {
Arrays.fill(utf8Bytes, (byte) 0);
}
}

// Save limited_by_role_descriptors
builder.startObject("limited_by_role_descriptors");
for (RoleDescriptor descriptor : roleDescriptorSet) {
builder.field(descriptor.getName(),
(contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true));
}
builder.endObject();

builder.field("name", request.getName())
.field("version", version.id)
.startObject("creator")
.field("principal", authentication.getUser().principal())
.field("metadata", authentication.getUser().metadata())
.field("realm", authentication.getLookedUpBy() == null ?
authentication.getAuthenticatedBy().getName() : authentication.getLookedUpBy().getName())
.endObject()
.endObject();
final IndexRequest indexRequest =
client.prepareIndex(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE)
.setSource(builder)
.setRefreshPolicy(request.getRefreshPolicy())
.request();
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
executeAsyncWithOrigin(client, SECURITY_ORIGIN, IndexAction.INSTANCE, indexRequest,
ActionListener.wrap(
indexResponse -> listener.onResponse(
new CreateApiKeyResponse(request.getName(), indexResponse.getId(), apiKey, expiration)),
listener::onFailure)));
} catch (IOException e) {
listener.onFailure(e);
} finally {
Arrays.fill(keyHash, (char) 0);
}
} else {
listener.onFailure(traceLog("create api key", new ElasticsearchSecurityException(
"Error creating api key as api key with name [{}] already exists", request.getName())));
// Save role_descriptors
builder.startObject("role_descriptors");
if (request.getRoleDescriptors() != null && request.getRoleDescriptors().isEmpty() == false) {
for (RoleDescriptor descriptor : request.getRoleDescriptors()) {
builder.field(descriptor.getName(),
(contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true));
}
}, listener::onFailure));
}
builder.endObject();

// Save limited_by_role_descriptors
builder.startObject("limited_by_role_descriptors");
for (RoleDescriptor descriptor : roleDescriptorSet) {
builder.field(descriptor.getName(),
(contentBuilder, params) -> descriptor.toXContent(contentBuilder, params, true));
}
builder.endObject();

builder.field("name", request.getName())
.field("version", version.id)
.startObject("creator")
.field("principal", authentication.getUser().principal())
.field("metadata", authentication.getUser().metadata())
.field("realm", authentication.getLookedUpBy() == null ?
authentication.getAuthenticatedBy().getName() : authentication.getLookedUpBy().getName())
.endObject()
.endObject();
final IndexRequest indexRequest =
client.prepareIndex(SECURITY_INDEX_NAME, SINGLE_MAPPING_NAME)
.setSource(builder)
.setRefreshPolicy(request.getRefreshPolicy())
.request();
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
executeAsyncWithOrigin(client, SECURITY_ORIGIN, IndexAction.INSTANCE, indexRequest,
ActionListener.wrap(
indexResponse -> listener.onResponse(
new CreateApiKeyResponse(request.getName(), indexResponse.getId(), apiKey, expiration)),
listener::onFailure)));
} catch (IOException e) {
listener.onFailure(e);
} finally {
Arrays.fill(keyHash, (char) 0);
}
}

Expand Down
40 changes: 31 additions & 9 deletions x-pack/plugin/src/test/resources/rest-api-spec/test/api_key/10_basic.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,34 @@ setup:
cluster.health:
wait_for_status: yellow

- do:
xpack.security.put_role:
name: "admin_role"
body: >
{
"cluster": ["all"],
"indices": [
{
"names": "*",
"privileges": ["all"]
}
],
"applications": [
{
"application": "myapp",
"privileges": ["*"],
"resources": ["*"]
}
]
}
- do:
xpack.security.put_user:
username: "api_key_user"
body: >
{
"password" : "x-pack-test-password",
"roles" : [ "superuser" ],
"roles" : [ "admin_role" ],
"full_name" : "API key user"
}
Expand All @@ -38,6 +59,11 @@ setup:
---
teardown:
- do:
xpack.security.delete_role:
name: "admin_role"
ignore: 404

- do:
xpack.security.delete_user:
username: "api_key_user"
Expand All @@ -54,7 +80,7 @@ teardown:

- do:
headers:
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk"
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user
security.create_api_key:
body: >
{
Expand Down Expand Up @@ -105,7 +131,7 @@ teardown:

- do:
headers:
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk"
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user
security.create_api_key:
body: >
{
Expand Down Expand Up @@ -140,8 +166,6 @@ teardown:
- set: { name: api_key_name }

- do:
headers:
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk"
security.get_api_key:
id: "$api_key_id"
- match: { "api_keys.0.id": "$api_key_id" }
Expand All @@ -157,7 +181,7 @@ teardown:

- do:
headers:
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk"
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user
security.create_api_key:
body: >
{
Expand All @@ -174,8 +198,6 @@ teardown:
- transform_and_set: { login_creds: "#base64EncodeCredentials(id,api_key)" }

- do:
headers:
Authorization: Apikey ${login_creds}
security.invalidate_api_key:
body: >
{
Expand All @@ -193,7 +215,7 @@ teardown:

- do:
headers:
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk"
Authorization: "Basic YXBpX2tleV91c2VyOngtcGFjay10ZXN0LXBhc3N3b3Jk" # api_key_user
security.create_api_key:
body: >
{
Expand Down

0 comments on commit 145e10b

Please sign in to comment.