Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -38,6 +38,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -767,6 +768,25 @@ public static boolean isDeploymentIdTakenByAnotherDeploymentType(Config config,
.anyMatch(entry -> entry.getKey() != type && entry.getValue().containsKey(shortName));
}

/**
* Returns {@code true} if {@code candidate}'s secret is already used by a different entry of
* the folded {@code Config.keys} map — file-sourced entries keyed by their raw secret,
* blob-sourced entries keyed by canonical id. The entry stored under {@code selfMapKey} is
* the candidate's own and never counts as a collision.
*/
public static boolean isKeySecretTakenByAnotherKey(Config config, String selfMapKey, Key candidate) {
String secret = candidate.getKey();
if (secret == null || secret.isBlank()) {
return false;
}
return config.getKeys().entrySet().stream()
.filter(entry -> !entry.getKey().equals(selfMapKey))
.map(Map.Entry::getValue)
.filter(Objects::nonNull)
.map(Key::getKey)
.anyMatch(secret::equals);
}

private static boolean isValidResourceKey(String resourceKey) {
return RESOURCE_KEY_PATTERN.matcher(resourceKey).matches();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,11 @@ void applyReplicaEvent(ResourceDescriptor descriptor, ResourceEvent.Action actio
if (oldSecret != null && !oldSecret.isBlank() && !oldSecret.equals(secret)) {
apiKeyStore.removeKey(oldSecret);
}
if (snapshot != null
&& ConfigPostProcessor.isKeySecretTakenByAnotherKey(snapshot, mapKey, key)) {
log.warn("Replica key event carries a secret already used by a different key entity: {}",
descriptor.getUrl());
}
}
applyEntityWrite(type, mapKey, entity);
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,11 @@ private void collectKeys(Config fileConfig, Config scratch, boolean dryRun,
// serialization regardless of the Java object's field state — inject it directly.
ObjectNode specNode = ProxyUtil.MAPPER.valueToTree(fileKey);
specNode.put("key", secret);
// The duplicate-secret guard rejects a key write whose secret is already used by a
// different scratch entry. Since scratch starts as a copy of the merged/live config,
// the file-sourced key being migrated still sits under its raw secret there — remove
// the shadow so validation sees a genuinely new blob entry, not a collision with itself.
scratch.getKeys().remove(secret);
collect(new AdminManifest("Key", canonicalId, specNode), null, scratch, dryRun, toApply, results);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1625,7 +1625,8 @@ private Future<?> handlePut() {
}
if (spec.isKey()) {
keyEntity = (Key) entity;
validateKeyForApiWrite(keyEntity, "PUT");
validateKeyForApiWrite(keyEntity);
rejectDuplicateKeySecret(keyEntity, oldSecret, requestNode, descriptor);
}
if (spec.hasEncryptedFields()) {
secretFieldProcessor.encryptFields(entity, descriptor);
Expand Down Expand Up @@ -1807,16 +1808,37 @@ private void rejectDuplicateDeploymentId(ResourceTypes type, String shortName) {
}
}

/**
* Rejects a key write whose secret is already used by a different key entity in the live
* merged Config — ApiKeyStore indexes keys by plaintext secret, so a duplicate would silently
* collapse the two entities' auth, roles and project attribution. Skipped when the request
* omits the secret (preserve-on-omit) or re-supplies the entity's own current secret.
*/
private void rejectDuplicateKeySecret(Key keyEntity, String oldSecret, JsonNode requestNode,
ResourceDescriptor descriptor) {
if (!requestNode.hasNonNull("key") || keyEntity.getKey().equals(oldSecret)) {
return;
}
Config snapshot = mergedConfigStore.get();
if (snapshot == null) {
return;
}
if (ConfigPostProcessor.isKeySecretTakenByAnotherKey(snapshot,
MergedConfigStore.resolveMapKeyFor(descriptor), keyEntity)) {
throw new HttpException(HttpStatus.CONFLICT,
"Key secret is already used by a different key entity");
}
}

private static ApiKeyData apiKeyData(Key key) {
ApiKeyData data = new ApiKeyData();
data.setOriginalKey(key);
return data;
}

private static void validateKeyForApiWrite(Key key, String method) {
private static void validateKeyForApiWrite(Key key) {
if (StringUtils.isBlank(key.getKey())) {
throw new HttpException(HttpStatus.BAD_REQUEST,
"Key.key must be provided explicitly on " + method);
throw new HttpException(HttpStatus.BAD_REQUEST, "Key.key must be provided explicitly on PUT");
}
validateProjectKey(key);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ private EntityResult applySingle(ConfigManifestSupport.ParsedManifest parsed, Co
applyManagedEntity(role.spec(), id, parsed.name(), ResourceTypes.ROLE, scratch, pending);
case AdminRouteManifest route ->
applyManagedEntity(route.spec(), id, parsed.name(), ResourceTypes.ROUTE, scratch, pending);
case AdminKeyManifest key -> applyKey(key.spec(), id, parsed.name(), pending);
case AdminKeyManifest key -> applyKey(key.spec(), id, parsed.name(), scratch, pending);
case AdminModelManifest model -> applyModel(model.spec(), id, parsed.name(), scratch, pending);
case AdminToolSetManifest toolSet -> applyToolSet(toolSet.spec(), id, parsed.name(), scratch, pending);
case AdminApplicationManifest application ->
Expand Down Expand Up @@ -226,7 +226,7 @@ private EntityResult applyTranslator(Translator translator, String id, ParsedNam
return new EntityResult(id, AdminApplyStatus.APPLIED, null);
}

private EntityResult applyKey(Key key, String id, ParsedName parsed, List<EntityChange> pending) {
private EntityResult applyKey(Key key, String id, ParsedName parsed, Config scratch, List<EntityChange> pending) {
if (StringUtils.isBlank(key.getKey())) {
return new EntityResult(id, AdminApplyStatus.FAILED, "Key.key must be provided explicitly");
}
Expand Down Expand Up @@ -256,6 +256,11 @@ private EntityResult applyKey(Key key, String id, ParsedName parsed, List<Entity
+ "proceeding with new secret as authoritative", descriptor.getUrl());
}
}
if (!secret.equals(oldSecret)
&& ConfigPostProcessor.isKeySecretTakenByAnotherKey(scratch, MergedConfigStore.canonicalId(descriptor), key)) {
return new EntityResult(id, AdminApplyStatus.FAILED,
"Key secret is already used by a different key entity");
}
secretFieldProcessor.encryptFields(key, descriptor);
String blobBody = ConfigEntityCodec.serializeForBlob(key);
resourceService.putResource(descriptor, blobBody, EtagHeader.ANY);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.epam.aidial.core.config.Model;
import com.epam.aidial.core.config.Translator;
import com.epam.aidial.core.server.config.ConfigPostProcessor;
import com.epam.aidial.core.server.config.MergedConfigStore;
import com.epam.aidial.core.server.config.ValidationWarning;
import com.epam.aidial.core.server.data.config.manifest.AdminApplicationManifest;
import com.epam.aidial.core.server.data.config.manifest.AdminCatalogSchemaManifest;
Expand Down Expand Up @@ -112,6 +113,15 @@ public ValidationResult validateOnly(AdminManifest entry, Config scratch) {
return new ValidationResult(id, ValidationStatus.FAILED,
"Invalid key: at least one role must be assigned to the key " + key.getProject());
}
String canonicalId = MergedConfigStore.canonicalId(
ResourceTypes.PROJECT_KEY, parsed.name().bucket(), parsed.name().name());
Key prior = scratch.getKeys().get(canonicalId);
String oldSecret = prior == null ? null : prior.getKey();
if (!key.getKey().equals(oldSecret)
&& ConfigPostProcessor.isKeySecretTakenByAnotherKey(scratch, canonicalId, key)) {
return new ValidationResult(id, ValidationStatus.FAILED,
"Key secret is already used by a different key entity");
}
}
case AdminApplicationManifest applicationManifest -> {
List<ValidationWarning> warnings = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,150 @@ void batchKeyRotationRemovesOldSecret() {
verify(send(HttpMethod.GET, "/v1/bucket", null, "", "Api-key", "apply-secret-new"), 200);
}

@Test
@SneakyThrows
void applyKeyDuplicateSecretFailsSecondEntity() {
// ApiKeyStore indexes keys by plaintext secret, so two key entities sharing one secret
// would silently collapse their auth, roles and project attribution. The second entity
// in the batch must fail and leave nothing behind.
String body = """
{
"precheck": false,
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-dup-key-a",
"spec": {"key": "apply-dup-secret", "project": "projA", "roles": ["admin"]}
},
{
"kind": "Key",
"name": "keys/platform/apply-dup-key-b",
"spec": {"key": "apply-dup-secret", "project": "projB", "roles": ["admin"]}
}
]
}
""";
Response response = send(HttpMethod.POST, "/v1/admin/apply", null, body, "authorization", "admin");
verify(response, 200);
JsonNode parsed = ProxyUtil.MAPPER.readTree(response.body());
assertEquals(1, parsed.get("applied").asInt(), () -> "Body: " + response.body());
assertEquals(1, parsed.get("failed").asInt(), () -> "Body: " + response.body());
assertTrue(parsed.get("results").get(1).get("error").asText()
.contains("already used by a different key entity"), () -> "Body: " + response.body());
assertFalse(response.body().contains("apply-dup-secret"),
() -> "Response must not echo the secret: " + response.body());
verify(send(HttpMethod.GET, "/v1/keys/platform/apply-dup-key-a", null, "",
"authorization", "admin"), 200);
verify(send(HttpMethod.GET, "/v1/keys/platform/apply-dup-key-b", null, "",
"authorization", "admin"), 404);
verify(send(HttpMethod.GET, "/v1/bucket", null, "", "Api-key", "apply-dup-secret"), 200);
}

@Test
@SneakyThrows
void applyKeyRotateToExistingSecretFailsEntity() {
String bodyA = """
{
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-rotate-dup-key-a",
"spec": {"key": "apply-rotate-dup-a", "project": "projA", "roles": ["admin"]}
}
]
}
""";
String bodyB = """
{
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-rotate-dup-key-b",
"spec": {"key": "apply-rotate-dup-b", "project": "projB", "roles": ["admin"]}
}
]
}
""";
String bodyRotate = """
{
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-rotate-dup-key-a",
"spec": {"key": "apply-rotate-dup-b", "project": "projA", "roles": ["admin"]}
}
]
}
""";
verify(send(HttpMethod.POST, "/v1/admin/apply", null, bodyA, "authorization", "admin"), 200);
verify(send(HttpMethod.POST, "/v1/admin/apply", null, bodyB, "authorization", "admin"), 200);

Response response = send(HttpMethod.POST, "/v1/admin/apply", null, bodyRotate, "authorization", "admin");
verify(response, 422);
JsonNode parsed = ProxyUtil.MAPPER.readTree(response.body());
assertEquals(1, parsed.get("failed").asInt(), () -> "Body: " + response.body());

// Both original secrets still authenticate; the rotation was refused wholesale.
verify(send(HttpMethod.GET, "/v1/bucket", null, "", "Api-key", "apply-rotate-dup-a"), 200);
verify(send(HttpMethod.GET, "/v1/bucket", null, "", "Api-key", "apply-rotate-dup-b"), 200);
}

@Test
@SneakyThrows
void applyKeyUpdateWithUnchangedSecretSucceeds() {
// An update re-supplying the entity's own current secret is not a collision with itself —
// the guard only polices new secrets.
String body = """
{
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-unchanged-key",
"spec": {"key": "apply-unchanged-secret", "project": "projA", "roles": ["admin"]}
}
]
}
""";
String bodyUpdated = """
{
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-unchanged-key",
"spec": {"key": "apply-unchanged-secret", "project": "projB", "roles": ["default"]}
}
]
}
""";
verify(send(HttpMethod.POST, "/v1/admin/apply", null, body, "authorization", "admin"), 200);
verify(send(HttpMethod.POST, "/v1/admin/apply", null, bodyUpdated, "authorization", "admin"), 200);
verify(send(HttpMethod.GET, "/v1/bucket", null, "", "Api-key", "apply-unchanged-secret"), 200);
}

@Test
@SneakyThrows
void applyKeyWithFileKeySecretFails() {
// The file→blob handoff for keys goes through the migration endpoint; a direct apply
// claiming a file-sourced key's secret must fail.
String body = """
{
"manifests": [
{
"kind": "Key",
"name": "keys/platform/apply-file-secret-key",
"spec": {"key": "proxyKey1", "project": "someone-else", "roles": ["admin"]}
}
]
}
""";
Response response = send(HttpMethod.POST, "/v1/admin/apply", null, body, "authorization", "admin");
verify(response, 422);
assertFalse(response.body().contains("proxyKey1"),
() -> "Response must not echo the secret: " + response.body());
verify(send(HttpMethod.GET, "/v1/keys/platform/apply-file-secret-key", null, "",
"authorization", "admin"), 404);
}

@Test
@SneakyThrows
void testApplyEmptyManifestsBatchOk() {
Expand Down
Loading