Skip to content

Commit

Permalink
Improve messages for assertions in API Key Service (elastic#109229)
Browse files Browse the repository at this point in the history 
The API Key Service has a lot of assertions, but many of them did not
have useful messages. If they were tripped it was very difficult to
determine exactly what conditions failed.

This commit makes sure each assertion has a reasonable message.
  • Loading branch information
@tvernum
tvernum committed Jun 13, 2024
1 parent b4fdfbb commit 0fd9505
Showing 1 changed file with 105 additions and 22 deletions.
127 changes: 105 additions & 22 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 @@ -343,9 +343,21 @@ public void createApiKey(
ActionListener<CreateApiKeyResponse> listener
) {
assert request.getType() != ApiKey.Type.CROSS_CLUSTER || false == authentication.isApiKey()
: "cannot create derived cross-cluster API keys";
: "cannot create derived cross-cluster API keys (name=["
+ request.getName()
+ "], type=["
+ request.getType()
+ "], auth=["
+ authentication
+ "])";
assert request.getType() != ApiKey.Type.CROSS_CLUSTER || userRoleDescriptors.isEmpty()
: "owner user role descriptor must be empty for cross-cluster API keys";
: "owner user role descriptor must be empty for cross-cluster API keys (name=["
+ request.getName()
+ "], type=["
+ request.getType()
+ "], roles=["
+ userRoleDescriptors
+ "])";
ensureEnabled();
if (authentication == null) {
listener.onFailure(new IllegalArgumentException("authentication must be provided"));
Expand Down Expand Up @@ -492,7 +504,8 @@ private void createApiKeyAndIndexIt(
final Instant created = clock.instant();
final Instant expiration = getApiKeyExpiration(created, request.getExpiration());
final SecureString apiKey = UUIDs.randomBase64UUIDSecureString();
assert ApiKey.Type.CROSS_CLUSTER != request.getType() || API_KEY_SECRET_LENGTH == apiKey.length();
assert ApiKey.Type.CROSS_CLUSTER != request.getType() || API_KEY_SECRET_LENGTH == apiKey.length()
: "Invalid API key (name=[" + request.getName() + "], type=[" + request.getType() + "], length=[" + apiKey.length() + "])";

computeHashForApiKey(apiKey, listener.delegateFailure((l, apiKeyHashChars) -> {
try (
Expand Down Expand Up @@ -528,8 +541,16 @@ private void createApiKeyAndIndexIt(
TransportBulkAction.TYPE,
bulkRequest,
TransportBulkAction.<IndexResponse>unwrappingSingleItemBulkResponse(ActionListener.wrap(indexResponse -> {
assert request.getId().equals(indexResponse.getId());
assert indexResponse.getResult() == DocWriteResponse.Result.CREATED;
assert request.getId().equals(indexResponse.getId())
: "Mismatched API key (request=["
+ request.getId()
+ "](name=["
+ request.getName()
+ "]) index=["
+ indexResponse.getId()
+ "])";
assert indexResponse.getResult() == DocWriteResponse.Result.CREATED
: "Index response was [" + indexResponse.getResult() + "]";
final ListenableFuture<CachedApiKeyHashResult> listenableFuture = new ListenableFuture<>();
listenableFuture.onResponse(new CachedApiKeyHashResult(true, apiKey));
apiKeyAuthCache.put(request.getId(), listenableFuture);
Expand All @@ -552,7 +573,15 @@ public void updateApiKeys(
final ActionListener<BulkUpdateApiKeyResponse> listener
) {
assert request.getType() != ApiKey.Type.CROSS_CLUSTER || userRoleDescriptors.isEmpty()
: "owner user role descriptor must be empty for cross-cluster API keys";
: "owner user role descriptor must be empty for cross-cluster API keys (ids=["
+ (request.getIds().size() <= 10
? request.getIds()
: (request.getIds().size() + " including " + request.getIds().subList(0, 10)))
+ "], type=["
+ request.getType()
+ "], roles=["
+ userRoleDescriptors
+ "])";
ensureEnabled();
if (authentication == null) {
listener.onFailure(new IllegalArgumentException("authentication must be provided"));
Expand Down Expand Up @@ -628,10 +657,11 @@ private void updateApiKeys(
) {
logger.trace("Found [{}] API keys of [{}] requested for update", targetVersionedDocs.size(), request.getIds().size());
assert targetVersionedDocs.size() <= request.getIds().size()
: "more docs were found for update than were requested. found: "
: "more docs were found for update than were requested. found ["
+ targetVersionedDocs.size()
+ " requested: "
+ request.getIds().size();
+ "] requested ["
+ request.getIds().size()
+ "]";

final BulkUpdateApiKeyResponse.Builder responseBuilder = BulkUpdateApiKeyResponse.builder();
final BulkRequestBuilder bulkRequestBuilder = client.prepareBulk();
Expand Down Expand Up @@ -682,7 +712,14 @@ void validateForUpdate(
final Authentication authentication,
final ApiKeyDoc apiKeyDoc
) {
assert authentication.getEffectiveSubject().getUser().principal().equals(apiKeyDoc.creator.get("principal"));
assert authentication.getEffectiveSubject().getUser().principal().equals(apiKeyDoc.creator.get("principal"))
: "Authenticated user should be owner (authentication=["
+ authentication
+ "], owner=["
+ apiKeyDoc.creator
+ "], id=["
+ apiKeyId
+ "])";

if (apiKeyDoc.invalidated) {
throw new IllegalArgumentException("cannot update invalidated API key [" + apiKeyId + "]");
Expand Down Expand Up @@ -862,7 +899,14 @@ static XContentBuilder maybeBuildUpdatedDocument(
final Set<RoleDescriptor> userRoleDescriptors,
final Clock clock
) throws IOException {
assert currentApiKeyDoc.type == request.getType();
assert currentApiKeyDoc.type == request.getType()
: "API Key doc does not match request type (key-id=["
+ apiKeyId
+ "], doc=["
+ currentApiKeyDoc.type
+ "], request=["
+ request.getType()
+ "])";
if (isNoop(apiKeyId, currentApiKeyDoc, targetDocVersion, authentication, request, userRoleDescriptors)) {
return null;
}
Expand All @@ -888,7 +932,7 @@ static XContentBuilder maybeBuildUpdatedDocument(
logger.trace(() -> format("Building API key doc with updated role descriptors [%s]", keyRoles));
addRoleDescriptors(builder, keyRoles);
} else {
assert currentApiKeyDoc.roleDescriptorsBytes != null;
assert currentApiKeyDoc.roleDescriptorsBytes != null : "Role descriptors for [" + apiKeyId + "] are null";
builder.rawField("role_descriptors", currentApiKeyDoc.roleDescriptorsBytes.streamInput(), XContentType.JSON);
}

Expand All @@ -899,7 +943,7 @@ static XContentBuilder maybeBuildUpdatedDocument(
assert currentApiKeyDoc.metadataFlattened == null
|| MetadataUtils.containsReservedMetadata(
XContentHelper.convertToMap(currentApiKeyDoc.metadataFlattened, false, XContentType.JSON).v2()
) == false : "API key doc to be updated contains reserved metadata";
) == false : "API key doc [" + apiKeyId + "] to be updated contains reserved metadata";
final Map<String, Object> metadata = request.getMetadata();
if (metadata != null) {
logger.trace(() -> format("Building API key doc with updated metadata [%s]", metadata));
Expand Down Expand Up @@ -999,7 +1043,7 @@ private static boolean isNoop(
}
}

assert userRoleDescriptors != null;
assert userRoleDescriptors != null : "API Key [" + apiKeyId + "] has null role descriptors";
final List<RoleDescriptor> currentLimitedByRoleDescriptors = parseRoleDescriptorsBytes(
apiKeyId,
apiKeyDoc.limitedByRoleDescriptorsBytes,
Expand Down Expand Up @@ -1456,8 +1500,15 @@ public void crossClusterApiKeyUsageStats(ActionListener<Map<String, Object>> lis
findApiKeys(boolQuery, true, true, this::convertSearchHitToApiKeyInfo, ActionListener.wrap(apiKeyInfos -> {
int ccsKeys = 0, ccrKeys = 0, ccsCcrKeys = 0;
for (ApiKey apiKeyInfo : apiKeyInfos) {
assert apiKeyInfo.getType() == ApiKey.Type.CROSS_CLUSTER;
assert apiKeyInfo.getRoleDescriptors().size() == 1;
assert apiKeyInfo.getType() == ApiKey.Type.CROSS_CLUSTER
: "Incorrect API Key type for [" + apiKeyInfo + "] should be [" + ApiKey.Type.CROSS_CLUSTER + "]";
assert apiKeyInfo.getRoleDescriptors().size() == 1
: "API Key ["
+ apiKeyInfo
+ "] has ["
+ apiKeyInfo.getRoleDescriptors().size()
+ "] role descriptors, but should be 1";

final List<String> clusterPrivileges = Arrays.asList(
apiKeyInfo.getRoleDescriptors().iterator().next().getClusterPrivileges()
);
Expand Down Expand Up @@ -1614,7 +1665,14 @@ private IndexRequest maybeBuildIndexRequest(
}
final var targetDocVersion = ApiKey.CURRENT_API_KEY_VERSION;
final var currentDocVersion = new ApiKey.Version(currentVersionedDoc.doc().version);
assert currentDocVersion.onOrBefore(targetDocVersion) : "current API key doc version must be on or before target version";
assert currentDocVersion.onOrBefore(targetDocVersion)
: "API key ["
+ currentVersionedDoc.id()
+ "] has version ["
+ currentDocVersion
+ " which is greater than current version ["
+ ApiKey.CURRENT_API_KEY_VERSION
+ "]";
if (logger.isDebugEnabled() && currentDocVersion.before(targetDocVersion)) {
logger.debug(
"API key update for [{}] will update version from [{}] to [{}]",
Expand Down Expand Up @@ -1769,7 +1827,7 @@ private void findVersionedApiKeyDocsForSubject(
final String[] apiKeyIds,
final ActionListener<Collection<VersionedApiKeyDoc>> listener
) {
assert authentication.isApiKey() == false;
assert authentication.isApiKey() == false : "Authentication [" + authentication + "] is an API key, but should not be";
findApiKeysForUserRealmApiKeyIdAndNameCombination(
getOwnersRealmNames(authentication),
authentication.getEffectiveSubject().getUser().principal(),
Expand Down Expand Up @@ -1859,12 +1917,28 @@ private void indexInvalidation(
apiKeyIdsToInvalidate.add(apiKeyId);
}
}
assert false == apiKeyIdsToInvalidate.isEmpty() || false == crossClusterApiKeyIdsToSkip.isEmpty();

// noinspection ConstantValue
assert false == apiKeyIdsToInvalidate.isEmpty() || false == crossClusterApiKeyIdsToSkip.isEmpty()
: "There are no API keys but that should never happen, original=["
+ (apiKeys.size() > 10 ? ("size=" + apiKeys.size() + " including " + apiKeys.iterator().next()) : apiKeys)
+ "], to-invalidate=["
+ apiKeyIdsToInvalidate
+ "], to-skip=["
+ crossClusterApiKeyIdsToSkip
+ "]";

if (apiKeyIdsToInvalidate.isEmpty()) {
listener.onResponse(new InvalidateApiKeyResponse(Collections.emptyList(), Collections.emptyList(), failedRequestResponses));
return;
}
assert bulkRequestBuilder.numberOfActions() > 0;
assert bulkRequestBuilder.numberOfActions() > 0
: "Bulk request has ["
+ bulkRequestBuilder.numberOfActions()
+ "] actions, but there are ["
+ apiKeyIdsToInvalidate.size()
+ "] api keys to invalidate";

bulkRequestBuilder.setRefreshPolicy(defaultCreateDocRefreshPolicy(settings));
securityIndex.prepareIndexIfNeededThenExecute(
ex -> listener.onFailure(traceLog("prepare security index", ex)),
Expand Down Expand Up @@ -1932,7 +2006,14 @@ private void buildResponseAndClearCache(
);
} else {
// Since we made an index request against an existing document, we can't get a NOOP or CREATED here
assert bulkItemResponse.getResponse().getResult() == DocWriteResponse.Result.UPDATED;
assert bulkItemResponse.getResponse().getResult() == DocWriteResponse.Result.UPDATED
: "Bulk Item ["
+ bulkItemResponse.getId()
+ "] is ["
+ bulkItemResponse.getResponse().getResult()
+ "] but should be ["
+ DocWriteResponse.Result.UPDATED
+ "]";
responseBuilder.updated(apiKeyId);
}
}
Expand Down Expand Up @@ -2279,7 +2360,9 @@ public static String[] getOwnersRealmNames(final Authentication authentication)
// is no owner information to return here
if (effectiveSubjectRealm == null) {
final var message =
"Cannot determine owner realms without an effective subject realm for non-API key authentication object";
"Cannot determine owner realms without an effective subject realm for non-API key authentication object ["
+ authentication
+ "]";
assert false : message;
throw new IllegalArgumentException(message);
}
Expand Down

0 comments on commit 0fd9505

Please sign in to comment.