Skip to content

Commit

Permalink
fix: improve handling of KeyPairResource state
Browse files Browse the repository at this point in the history 
issue warning if new key is inactive, and no other active ones

add endpoint to KeyPairResourceApi

added tests
  • Loading branch information
@paullatzelsperger
paullatzelsperger committed Feb 29, 2024
1 parent 159eb0f commit 7285aad
Show file tree
Hide file tree
Showing 11 changed files with 350 additions and 67 deletions.
34 changes: 33 additions & 1 deletion ...y-hub-keypairs/src/main/java/org/eclipse/edc/identityhub/keypairs/KeyPairServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@

import java.time.Instant;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

Expand All @@ -63,10 +65,23 @@ public ServiceResult<Void> addKeyPair(String participantId, KeyDescriptor keyDes
return ServiceResult.badRequest(key.getFailureDetail());
}

// check if the new key is not active, and no other active key exists
if (!keyDescriptor.isActive()) {
var hasActiveKeys = keyPairResourceStore.query(QuerySpec.Builder.newInstance().filter(new Criterion("participantId", "=", participantId)).build())
.orElse(failure -> Collections.emptySet())
.stream().filter(kpr -> kpr.getState() == KeyPairState.ACTIVE.code())
.findAny()
.isEmpty();

if (!hasActiveKeys) {
monitor.warning("Participant '%s' has no active key pairs, and adding an inactive one will prevent the participant from becoming operational.");
}
}

var newResource = KeyPairResource.Builder.newInstance()
.id(keyDescriptor.getKeyId())
.keyId(keyDescriptor.getKeyId())
.state(KeyPairState.CREATED)
.state(keyDescriptor.isActive() ? KeyPairState.ACTIVE : KeyPairState.CREATED)
.isDefaultPair(makeDefault)
.privateKeyAlias(keyDescriptor.getPrivateKeyAlias())
.serializedPublicKey(key.getContent())
Expand Down Expand Up @@ -132,6 +147,23 @@ public ServiceResult<Collection<KeyPairResource>> query(QuerySpec querySpec) {
return ServiceResult.from(keyPairResourceStore.query(querySpec));
}

@Override
public ServiceResult<Void> activate(String keyPairResourceId) {
var oldKey = findById(keyPairResourceId);
if (oldKey == null) {
return ServiceResult.notFound("A KeyPairResource with ID '%s' does not exist.".formatted(keyPairResourceId));
}

var allowedStates = List.of(KeyPairState.ACTIVE.code(), KeyPairState.CREATED.code());
if (!allowedStates.contains(oldKey.getState())) {
return ServiceResult.badRequest("The key pair resource is expected to be in %s, but was %s".formatted(allowedStates, oldKey.getState()));
}

oldKey.activate();

return ServiceResult.from(keyPairResourceStore.update(oldKey));
}

@Override
public <E extends Event> void on(EventEnvelope<E> eventEnvelope) {
var payload = eventEnvelope.getPayload();
Expand Down
75 changes: 72 additions & 3 deletions ...b-keypairs/src/test/java/org/eclipse/edc/identityhub/keypairs/KeyPairServiceImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.junit.jupiter.params.provider.ValueSource;

import java.time.Duration;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.UUID;
Expand All @@ -41,6 +42,7 @@
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
Expand All @@ -55,7 +57,7 @@ class KeyPairServiceImplTest {


@ParameterizedTest(name = "make default: {0}")
@ValueSource(booleans = {true, false})
@ValueSource(booleans = { true, false })
void addKeyPair_publicKeyGiven(boolean makeDefault) {

when(keyPairResourceStore.create(any())).thenReturn(success());
Expand All @@ -69,7 +71,7 @@ void addKeyPair_publicKeyGiven(boolean makeDefault) {
}

@ParameterizedTest(name = "make default: {0}")
@ValueSource(booleans = {true, false})
@ValueSource(booleans = { true, false })
void addKeyPair_shouldGenerate_storesInVault(boolean makeDefault) {
when(keyPairResourceStore.create(any())).thenReturn(success());

Expand All @@ -81,7 +83,34 @@ void addKeyPair_shouldGenerate_storesInVault(boolean makeDefault) {
assertThat(keyPairService.addKeyPair("some-participant", key, makeDefault)).isSucceeded();

verify(vault).storeSecret(eq(key.getPrivateKeyAlias()), anyString());
verify(keyPairResourceStore).create(argThat(kpr -> kpr.isDefaultPair() == makeDefault && kpr.getParticipantId().equals("some-participant")));
verify(keyPairResourceStore).create(argThat(kpr -> kpr.isDefaultPair() == makeDefault &&
kpr.getParticipantId().equals("some-participant") &&
kpr.getState() == KeyPairState.ACTIVE.code()));
verify(observableMock).invokeForEach(any());
verifyNoMoreInteractions(keyPairResourceStore, vault, observableMock);
}

@ParameterizedTest(name = "make active: {0}")
@ValueSource(booleans = { true, false })
void addKeyPair_assertActiveState(boolean isActive) {
when(keyPairResourceStore.query(any())).thenReturn(success(Collections.emptySet()));
when(keyPairResourceStore.create(any())).thenReturn(success());

var key = createKey().publicKeyJwk(null).publicKeyPem(null)
.active(isActive)
.keyGeneratorParams(Map.of(
"algorithm", "EdDSA",
"curve", "Ed25519"
)).build();

assertThat(keyPairService.addKeyPair("some-participant", key, true)).isSucceeded();

verify(vault).storeSecret(eq(key.getPrivateKeyAlias()), anyString());
//expect the query for other active keys at least once, if the new key is inactive
verify(keyPairResourceStore, isActive ? never() : times(1)).query(any());
verify(keyPairResourceStore).create(argThat(kpr -> kpr.isDefaultPair() &&
kpr.getParticipantId().equals("some-participant") &&
kpr.getState() == (isActive ? KeyPairState.ACTIVE.code() : KeyPairState.CREATED.code())));
verify(observableMock).invokeForEach(any());
verifyNoMoreInteractions(keyPairResourceStore, vault, observableMock);
}
Expand Down Expand Up @@ -268,6 +297,46 @@ void revokeKey_notfound() {
verifyNoMoreInteractions(keyPairResourceStore, vault, observableMock);
}

@ParameterizedTest(name = "Valid state = {0}")
// cannot use enum literals and the .code() method -> needs to be compile constant
@ValueSource(ints = { 100, 200 })
void activate(int validState) {
var oldId = "old-id";
var oldKey = createKeyPairResource().id(oldId).state(validState).build();

when(keyPairResourceStore.query(any())).thenReturn(success(List.of(oldKey)));
when(keyPairResourceStore.update(any())).thenReturn(success());

assertThat(keyPairService.activate(oldId)).isSucceeded();
}

@ParameterizedTest(name = "Valid state = {0}")
// cannot use enum literals and the .code() method -> needs to be compile constant
@ValueSource(ints = { 0, 30, 400, -10 })
void activate_invalidState(int validState) {
var oldId = "old-id";
var oldKey = createKeyPairResource().id(oldId).state(validState).build();

when(keyPairResourceStore.query(any())).thenReturn(success(List.of(oldKey)));
when(keyPairResourceStore.update(any())).thenReturn(success());

assertThat(keyPairService.activate(oldId))
.isFailed()
.detail()
.isEqualTo("The key pair resource is expected to be in [200, 100], but was %s".formatted(validState));
}

@Test
void activate_notExists() {

when(keyPairResourceStore.query(any())).thenReturn(success(List.of()));

assertThat(keyPairService.activate("notexists"))
.isFailed()
.detail()
.isEqualTo("A KeyPairResource with ID 'notexists' does not exist.");
}

private KeyPairResource.Builder createKeyPairResource() {
return KeyPairResource.Builder.newInstance()
.id(UUID.randomUUID().toString())
Expand Down
73 changes: 73 additions & 0 deletions ...tests/src/test/java/org/eclipse/edc/identityhub/tests/KeyPairResourceApiEndToEndTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.eclipse.edc.identityhub.spi.events.keypair.KeyPairAdded;
import org.eclipse.edc.identityhub.spi.events.keypair.KeyPairRotated;
import org.eclipse.edc.identityhub.spi.model.KeyPairResource;
import org.eclipse.edc.identityhub.spi.model.KeyPairState;
import org.eclipse.edc.identityhub.spi.model.participant.KeyDescriptor;
import org.eclipse.edc.identityhub.spi.model.participant.ParticipantContext;
import org.eclipse.edc.junit.annotations.EndToEndTest;
Expand Down Expand Up @@ -413,6 +414,78 @@ void getAll_notAuthorized() {
.statusCode(403);
}

@Test
void activate() {
var superUserKey = createSuperUser();
var user1 = "user1";
var token = createParticipant(user1);
var keyId = createKeyPair(user1);

assertThat(Arrays.asList(token, superUserKey))
.allSatisfy(t -> {
RUNTIME_CONFIGURATION.getManagementEndpoint().baseRequest()
.contentType(JSON)
.header(new Header("x-api-key", t))
.post("/v1/participants/%s/keypairs/%s/activate".formatted(toBase64(user1), keyId))
.then()
.log().ifValidationFails()
.statusCode(204)
.body(notNullValue());

assertThat(getDidForParticipant(user1))
.hasSize(1)
.allSatisfy(dd -> assertThat(dd.getVerificationMethod()).noneMatch(vm -> vm.getId().equals(keyId)));
});
}

@Test
void activate_notAuthorized() {
var user1 = "user1";
createParticipant(user1);
var keyId = createKeyPair(user1);
var attackerToken = createParticipant("attacker");

RUNTIME_CONFIGURATION.getManagementEndpoint().baseRequest()
.contentType(JSON)
.header(new Header("x-api-key", attackerToken))
.post("/v1/participants/%s/keypairs/%s/activate".formatted(toBase64(user1), keyId))
.then()
.log().ifValidationFails()
.statusCode(403)
.body(notNullValue());

assertThat(getKeyPairsForParticipant(user1))
.hasSize(2)
.allMatch(keyPairResource -> keyPairResource.getState() == KeyPairState.ACTIVE.code());
}

@Test
void activate_illegalState() {
var user1 = "user1";
var token = createParticipant(user1);
var keyId = createKeyPair(user1);

// first revoke the key, which puts it in the REVOKED state
RUNTIME_CONFIGURATION.getManagementEndpoint().baseRequest()
.contentType(JSON)
.header(new Header("x-api-key", token))
.post("/v1/participants/%s/keypairs/%s/revoke".formatted(toBase64(user1), keyId))
.then()
.log().ifValidationFails()
.statusCode(204)
.body(notNullValue());

// now attempt to activate
RUNTIME_CONFIGURATION.getManagementEndpoint().baseRequest()
.contentType(JSON)
.header(new Header("x-api-key", token))
.post("/v1/participants/%s/keypairs/%s/activate".formatted(toBase64(user1), keyId))
.then()
.log().ifValidationFails()
.statusCode(400)
.body(notNullValue());
}

private String createKeyPair(String participantId) {

var descriptor = createKeyDescriptor(participantId).build();
Expand Down
24 changes: 14 additions & 10 deletions .../api-tests/src/test/java/org/eclipse/edc/identityhub/tests/ManagementApiEndToEndTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.eclipse.edc.spi.query.Criterion;
import org.eclipse.edc.spi.query.QuerySpec;
import org.eclipse.edc.spi.security.Vault;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.extension.RegisterExtension;

import java.util.Collection;
Expand All @@ -50,23 +51,26 @@ public abstract class ManagementApiEndToEndTest {
@RegisterExtension
protected static final EdcRuntimeExtension RUNTIME = new EdcRuntimeExtension(":launcher", "identity-hub", RUNTIME_CONFIGURATION.controlPlaneConfiguration());

protected static ParticipantManifest createNewParticipant() {
protected static ParticipantManifest.Builder createNewParticipant() {
var manifest = ParticipantManifest.Builder.newInstance()
.participantId("another-participant")
.active(false)
.did("did:web:another:participant")
.serviceEndpoint(new Service("test-service", "test-service-type", "https://test.com"))
.key(KeyDescriptor.Builder.newInstance()
.privateKeyAlias("another-alias")
.keyGeneratorParams(Map.of("algorithm", "EdDSA", "curve", "Ed25519"))
.keyId("another-keyid")
.build())
.build();
.key(createKeyDescriptor().build());
return manifest;
}

@NotNull
public static KeyDescriptor.Builder createKeyDescriptor() {
return KeyDescriptor.Builder.newInstance()
.privateKeyAlias("another-alias")
.keyGeneratorParams(Map.of("algorithm", "EdDSA", "curve", "Ed25519"))
.keyId("another-keyid");
}

protected String createSuperUser() {
return createParticipant("super-user", List.of(ServicePrincipal.ROLE_ADMIN));
return createParticipant(SUPER_USER, List.of(ServicePrincipal.ROLE_ADMIN));
}

protected String storeParticipant(ParticipantContext pc) {
Expand All @@ -91,9 +95,9 @@ protected <T> T getService(Class<T> type) {
return RUNTIME.getContext().getService(type);
}

protected Collection<KeyPairResource> getKeyPairsForParticipant(ParticipantManifest manifest) {
protected Collection<KeyPairResource> getKeyPairsForParticipant(String participantId) {
return getService(KeyPairResourceStore.class).query(QuerySpec.Builder.newInstance()
.filter(new Criterion("participantId", "=", manifest.getParticipantId()))
.filter(new Criterion("participantId", "=", participantId))
.build()).getContent();
}

Expand Down
Loading

0 comments on commit 7285aad

Please sign in to comment.