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

feat(keys): add functionality to extract public key to KeyParsers #4360

Merged
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 @@ -58,7 +58,7 @@ void localPublicKeyService_withValueConfig(LocalPublicKeyDefaultExtension extens
"key1.id", "key1",
"key1.value", "value");

when(keyParserRegistry.parse("value")).thenReturn(Result.success(mock(PublicKey.class)));
when(keyParserRegistry.parsePublic("value")).thenReturn(Result.success(mock(PublicKey.class)));
when(context.getConfig(EDC_PUBLIC_KEYS_PREFIX)).thenReturn(ConfigFactory.fromMap(keys));
var localPublicKeyService = extension.localPublicKeyService();
extension.initialize(context);
Expand All @@ -75,7 +75,7 @@ void localPublicKeyService_withPathConfig(LocalPublicKeyDefaultExtension extensi
"key1.id", "key1",
"key1.path", path.getPath());

when(keyParserRegistry.parse(value)).thenReturn(Result.success(mock(PublicKey.class)));
when(keyParserRegistry.parsePublic(value)).thenReturn(Result.success(mock(PublicKey.class)));
when(context.getConfig(EDC_PUBLIC_KEYS_PREFIX)).thenReturn(ConfigFactory.fromMap(keys));
var localPublicKeyService = extension.localPublicKeyService();
extension.initialize(context);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.eclipse.edc.spi.result.Result;

import java.security.Key;
import java.security.PublicKey;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -37,4 +38,12 @@ public Result<Key> parse(String encoded) {
.map(kp -> kp.parse(encoded))
.orElseGet(() -> Result.failure("No parser found that can handle that format."));
}

@Override
public Result<PublicKey> parsePublic(String encoded) {
return parsers.stream().filter(kp -> kp.canHandle(encoded))
.findFirst()
.map(kp -> kp.parsePublic(encoded).compose(k -> Result.success((PublicKey) k)))
Copy link
Member

Choose a reason for hiding this comment

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

the type validation that was in LocalPublicKeyServiceImpl is missing

Copy link
Member Author

@paullatzelsperger paullatzelsperger Jul 12, 2024

Choose a reason for hiding this comment

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

as per the contract of the parsePublic method this can never be something other than a PublicKey, even though it returns a Key. This is necessary to be able to have the parsePublic method as default in the interface. Not all key parsers support this feature.

.orElseGet(() -> Result.failure("No parser found that can handle that format."));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ public Result<PublicKey> resolveKey(String id) {
.orElseGet(() -> Result.failure("No public key could be resolved for key-ID '%s'".formatted(id)));
}

public Result<Void> addRawKey(String id, String rawKey) {
return parseKey(rawKey).onSuccess((pk) -> cachedKeys.put(id, pk)).mapEmpty();
}

private Optional<String> resolveFromVault(String id) {
return Optional.ofNullable(vault.resolveSecret(id));
}
Expand All @@ -57,16 +61,6 @@ private Optional<PublicKey> resolveFromCache(String id) {
}

private Result<PublicKey> parseKey(String encodedKey) {
return registry.parse(encodedKey).compose(pk -> {
if (pk instanceof PublicKey publicKey) {
return Result.success(publicKey);
} else {
return Result.failure("The specified resource did not contain public key material.");
}
});
}

public Result<Void> addRawKey(String id, String rawKey) {
return parseKey(rawKey).onSuccess((pk) -> cachedKeys.put(id, pk)).mapTo();
return registry.parsePublic(encodedKey);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ public boolean canHandle(String encoded) {
@Override
public Result<Key> parse(String encoded) {
try {
var jwk = JWK.parse(encoded);


var jwk = JWK.parse(encoded);
// OctetKeyPairs (OKP) need special handling, as they can't be easily converted to a Java PrivateKey
if (jwk instanceof OctetKeyPair okp) {
return parseOctetKeyPair(okp).map(key -> key);
Expand All @@ -107,6 +108,26 @@ public Result<Key> parse(String encoded) {
}
}

@Override
public Result<Key> parsePublic(String encoded) {
try {
var jwk = JWK.parse(encoded).toPublicJWK();
// OctetKeyPairs (OKP) need special handling, as they can't be easily converted to a Java PrivateKey
if (jwk instanceof OctetKeyPair okp) {
return parseOctetKeyPair(okp.toPublicJWK()).map(key -> key);
}
var list = KeyConverter.toJavaKeys(List.of(jwk)); // contains an entry each for public and private key

return list.stream()
.findFirst()
.map(Result::success)
.orElse(Result.failure(ERROR_NO_KEY));
} catch (ParseException | NoSuchAlgorithmException | IOException | InvalidKeySpecException e) {
monitor.warning("Parser error", e);
return Result.failure("Parser error: " + e.getMessage());
}
}

private Result<? extends Key> parseOctetKeyPair(OctetKeyPair okp) throws NoSuchAlgorithmException, IOException, InvalidKeySpecException {
var d = okp.getDecodedD();
var x = okp.getDecodedX();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,7 @@ public boolean canHandle(String encoded) {
@Override
public Result<Key> parse(String encoded) {

var matcher = PEM_FORMAT_REGEX.matcher(encoded);
if (!matcher.find()) {
return Result.failure("The given input is not valid PEM.");
}
var keypair = parseKeys(encoded);
var keypair = parsePem(encoded);

if (keypair.succeeded()) {

Expand All @@ -83,7 +79,38 @@ public Result<Key> parse(String encoded) {
.orElseGet(() -> Result.failure("PEM-encoded structure did not contain a private key."));
}

return keypair.mapTo();
return keypair.mapEmpty();
}

@Override
public Result<Key> parsePublic(String encoded) {

var keypair = parsePem(encoded);
if (keypair.succeeded()) {

var keyPairList = keypair.getContent();
if (keyPairList.size() > 1) {
monitor.warning("PEM expected to contain exactly 1 key(-pair), but contained %s. Will take the first one. Please consider re-structuring your PEM document.".formatted(keyPairList.size()));
}
return keyPairList
.stream()
.filter(Objects::nonNull) // PEM strings that only contain public keys would get eliminated here
.map(keyPair -> (Key) keyPair.getPublic())
.filter(Objects::nonNull)
.findFirst()
.map(Result::success)
.orElseGet(() -> Result.failure("PEM-encoded structure did not contain a public key."));
}

return keypair.mapEmpty();
}

private Result<List<KeyPair>> parsePem(String pemEncoded) {
var matcher = PEM_FORMAT_REGEX.matcher(pemEncoded);
if (!matcher.find()) {
return Result.failure("The given input is not valid PEM.");
}
return parseKeys(pemEncoded);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

import java.security.PrivateKey;
import java.security.PublicKey;

import static org.mockito.Mockito.mock;
Expand All @@ -31,9 +30,9 @@

class LocalPublicKeyServiceImplTest {

private LocalPublicKeyServiceImpl localPublicKeyService;
private final Vault vault = mock();
private final KeyParserRegistry keyParserRegistry = mock();
private LocalPublicKeyServiceImpl localPublicKeyService;

@BeforeEach
void setup() {
Expand All @@ -42,7 +41,7 @@ void setup() {

@Test
void resolve_withCache() {
when(keyParserRegistry.parse("value")).thenReturn(Result.success(mock(PublicKey.class)));
when(keyParserRegistry.parsePublic("value")).thenReturn(Result.success(mock(PublicKey.class)));
localPublicKeyService.addRawKey("id", "value");
AbstractResultAssert.assertThat(localPublicKeyService.resolveKey("id")).isSucceeded();
Mockito.verifyNoInteractions(vault);
Expand All @@ -51,7 +50,7 @@ void resolve_withCache() {
@Test
void resolve_withVault() {
when(vault.resolveSecret("id")).thenReturn("value");
when(keyParserRegistry.parse("value")).thenReturn(Result.success(mock(PublicKey.class)));
when(keyParserRegistry.parsePublic("value")).thenReturn(Result.success(mock(PublicKey.class)));
AbstractResultAssert.assertThat(localPublicKeyService.resolveKey("id")).isSucceeded();

verify(vault).resolveSecret("id");
Expand All @@ -63,19 +62,10 @@ void resolve_notFound() {
verify(vault).resolveSecret("id");
}

@Test
void resolve_wrongKeyType() {
when(vault.resolveSecret("id")).thenReturn("value");
when(keyParserRegistry.parse("value")).thenReturn(Result.success(mock(PrivateKey.class)));

AbstractResultAssert.assertThat(localPublicKeyService.resolveKey("id")).isFailed();
verify(vault).resolveSecret("id");
}

@Test
void resolve_wrongKeyFormat() {
when(vault.resolveSecret("id")).thenReturn("value");
when(keyParserRegistry.parse("value")).thenReturn(Result.failure("failure"));
when(keyParserRegistry.parsePublic("value")).thenReturn(Result.failure("failure"));

AbstractResultAssert.assertThat(localPublicKeyService.resolveKey("id")).isFailed();
verify(vault).resolveSecret("id");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,23 @@ void parse_publicKey(JWK jwk) {
assertThat(result).isSucceeded().isInstanceOf(PublicKey.class);
}


@ParameterizedTest
@ArgumentsSource(KeyProvider.class)
void parsePublic_withPublicKey(JWK jwk) {
var publickey = jwk.toPublicJWK();
var result = parser.parsePublic(publickey.toJSONString());
assertThat(result).isSucceeded().isInstanceOf(PublicKey.class);
}


@ParameterizedTest
@ArgumentsSource(KeyProvider.class)
void parsePublic_withPrivateKey(JWK jwk) {
var result = parser.parsePublic(jwk.toJSONString());
assertThat(result).isSucceeded().isInstanceOf(PublicKey.class);
}

private static class KeyProvider implements ArgumentsProvider {
@Override
public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.assertj.core.api.Assertions;
import org.eclipse.edc.junit.testfixtures.TestUtils;
import org.junit.jupiter.api.Named;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand Down Expand Up @@ -74,6 +75,38 @@ void parse_publicKey(String pem) {
.isInstanceOf(PublicKey.class);
}


@ParameterizedTest
@ArgumentsSource(PemConvertiblePrivateKeyProvider.class)
void parsePublic_withPrivateKey(String pem) {
var result = parser.parsePublic(pem);
assertThat(result)
.isSucceeded()
.isNotNull()
.isInstanceOf(PublicKey.class);

}

@Test
void parsePublic_withPrivateKey_whenEd25519_shouldFail() {
var pem = TestUtils.getResourceFileContentAsString("ed25519.pem");
var result = parser.parsePublic(pem);
assertThat(result)
.isFailed()
.detail().isEqualTo("PEM-encoded structure did not contain a public key.");
}


@ParameterizedTest
@ArgumentsSource(PemPublicKeyProvider.class)
void parsePublic_withPublicKey(String pem) {

assertThat(parser.parsePublic(pem))
.isSucceeded()
.isNotNull()
.isInstanceOf(PublicKey.class);
}

private static class PemPrivateKeyProvider implements ArgumentsProvider {

@Override
Expand All @@ -88,6 +121,19 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
}
}

private static class PemConvertiblePrivateKeyProvider implements ArgumentsProvider {

@Override
public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
return Stream.of(
Arguments.of(Named.named("RSA PrivateKey", TestUtils.getResourceFileContentAsString("rsa_2048.pem"))),
Arguments.of(Named.named("EC PrivateKey (P256)", TestUtils.getResourceFileContentAsString("ec_p256.pem"))),
Arguments.of(Named.named("EC PrivateKey (P384)", TestUtils.getResourceFileContentAsString("ec_p384.pem"))),
Arguments.of(Named.named("EC PrivateKey (P512)", TestUtils.getResourceFileContentAsString("ec_p512.pem")))
);
}
}

private static class PemPublicKeyProvider implements ArgumentsProvider {

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,20 @@ public interface KeyParser {
* @return Either a {@link java.security.PrivateKey}, a {@link java.security.PublicKey} or a failure.
*/
Result<Key> parse(String encoded);

/**
* Parses the encoded key as public key. If the encoded string is invalid, or the parser can't handle the input,
* it must return a {@link Result#failure(String)}, it must never throw an exception.
* <p>
* If the given key material contains public and private key data, the parser attempts to remove the private key data,
* returning only the public part of the key as {@link java.security.PublicKey}.
* If the given key material does not contain private key data, just public key data, returns a {@link java.security.PublicKey}. In all
* other cases, a {@link Result#failure(String)} is returned, for example, when a private key cannot be converted into a public key.
*
* @param encoded serialized/encoded key material.
* @return Either a {@link java.security.PublicKey} or a failure.
*/
default Result<Key> parsePublic(String encoded) {
return parse(encoded);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import org.eclipse.edc.spi.result.Result;

import java.security.Key;
import java.security.PrivateKey;
import java.security.PublicKey;

/**
* Registry that holds multiple {@link KeyParser} instances that are used to deserialize a private key from their
Expand All @@ -30,11 +30,21 @@ public interface KeyParserRegistry {
void register(KeyParser parser);

/**
* Attempts to parse the String representation of a private key into a {@link PrivateKey}. If no parser can handle
* Attempts to parse the String representation of a private key into a {@link Key}. If no parser can handle
* the encoded format, or it is corrupt etc. then a failure is returned.
*
* @param encoded The private key in encoded format (PEM, OpenSSH, JWK, PKCS8,...)
* @return a success result containing the private key, a failure if the encoded private key could not be deserialized.
*/
Result<Key> parse(String encoded);

/**
* Attempts to parse the String representation of a public or private key into a {@link Key}. If no parser can handle
* the encoded format, if the encoded format contains a private key that cannot be converted to a public key,
* or if the input is corrupt etc. then a failure is returned.
*
* @param encoded The private key in encoded format (PEM, OpenSSH, JWK, PKCS8,...)
* @return a success result containing the public key, a failure if the encoded public key could not be deserialized.
*/
Result<PublicKey> parsePublic(String encoded);
}
Loading