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

Integration with KMS #1627

Closed
olivierayache opened this issue Feb 15, 2022 · 9 comments · Fixed by #1602
Closed

Integration with KMS #1627

olivierayache opened this issue Feb 15, 2022 · 9 comments · Fixed by #1602
Assignees
Labels
enhancement a feature request

Comments

@olivierayache
Copy link
Contributor

olivierayache commented Feb 15, 2022

I am currently working with Web3j and it seems interesting to me to add support to Key Management Services such as AWS KMS, Google Cloud KMS or Vault Enterprise to securely manipulate keys.

By analysing the code it should be feasible by inheriting ECKeyPair and making specifing implementation for each provider.

Is this feature already planned ?

If not I maybe begin to work on that

@olivierayache olivierayache added the enhancement a feature request label Feb 15, 2022
@conor10
Copy link
Contributor

conor10 commented Feb 16, 2022

Sounds good. It may be better to have it as a separate project module, but if you get the code working we can review how best to host it.

@olivierayache
Copy link
Contributor Author

olivierayache commented Feb 16, 2022

I think a web3-kms module should be a good idea too as an extension of web3j-core

Here is a first working draft for AWS KMS key pair
I think it should be nice to be more clean to have an IECKeyPair (mother class of all KeyPair implementation) but it involves modifications in web3j-crypto too


package org.web3j.crypto.kms;

import java.math.BigInteger;
import java.util.Arrays;
import org.bouncycastle.asn1.ASN1Integer;
import org.bouncycastle.asn1.ASN1Sequence;
import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo;
import org.web3j.crypto.ECDSASignature;
import org.web3j.crypto.ECKeyPair;
import software.amazon.awssdk.core.SdkBytes;
import software.amazon.awssdk.services.kms.KmsClient;
import software.amazon.awssdk.services.kms.model.MessageType;
import software.amazon.awssdk.services.kms.model.SignResponse;
import software.amazon.awssdk.services.kms.model.SigningAlgorithmSpec;

/**
 *
 * @author Olivier Ayache
 */
public class AWSECKeyPair extends ECKeyPair {

    private static final KmsClient CLIENT = KmsClient.create();
    private final String keyId; //AWS KMS KeyId;
    private final BigInteger publicKey;

    public AWSECKeyPair(String keyId) {
        super(null, null);
        this.keyId = keyId;
        byte[] derPublicKey = CLIENT
                .getPublicKey((var builder) -> {
                    builder.keyId(keyId);
                })
                .publicKey()
                .asByteArray();
        byte[] publicKey = SubjectPublicKeyInfo
                .getInstance(derPublicKey)
                .getPublicKeyData()
                .getBytes();
        this.publicKey = new BigInteger(1, Arrays.copyOfRange(publicKey, 1, publicKey.length));
    }

    @Override
    public BigInteger getPrivateKey() {
        throw new UnsupportedOperationException("Private key is not accessible when using AWS KMS");
    }

    @Override
    public BigInteger getPublicKey() {
        return publicKey;
    }

    @Override
    public ECDSASignature sign(byte[] transactionHash) {
        SignResponse sign = CLIENT.sign((var builder) -> {
            builder.keyId(keyId)
                    .messageType(MessageType.DIGEST)
                    .message(SdkBytes.fromByteArray(transactionHash))
                    .signingAlgorithm(SigningAlgorithmSpec.ECDSA_SHA_256);
        });
        ASN1Sequence instance = ASN1Sequence.getInstance(sign.signature().asByteArray());
        ASN1Integer r = (ASN1Integer) instance.getObjectAt(0);
        ASN1Integer s = (ASN1Integer) instance.getObjectAt(1);
        return new ECDSASignature(r.getValue(), s.getValue()).toCanonicalised();
    }

}

@andrii-kl andrii-kl self-assigned this Feb 16, 2022
@andrii-kl
Copy link
Contributor

andrii-kl commented Feb 16, 2022

I am currently working with Web3j and it seems interesting to me to add support to Key Management Services such as AWS KMS, Google Cloud KMS or Vault Enterprise to securely manipulate keys.

By analysing the code it should be feasible by inheriting ECKeyPair and making specifing implementation for each provider.

Is this feature already planned ?

If not I maybe begin to work on that

I would recommend you to use a new functionality from the release 4.9.0
You can take a look how it works in TxSignService implementations.

So the right way to add integrations with a KMS will be to extend HSMRequestProcessor and HSMPass.

You can take a look reference implementation over Http (HSMHTTPRequestProcessor and HSMHTTPPass).

Feel free to ask any question.

@andrii-kl andrii-kl pinned this issue Feb 16, 2022
@andrii-kl andrii-kl unpinned this issue Feb 16, 2022
@andrii-kl andrii-kl linked a pull request Feb 16, 2022 that will close this issue
@andrii-kl
Copy link
Contributor

andrii-kl commented Feb 16, 2022

#892

@olivierayache
Copy link
Contributor Author

I will checkout the last version and work on that

@olivierayache
Copy link
Contributor Author

olivierayache commented Feb 16, 2022

Thank you for the links. Just checked these improvements.
I just wonder why creating a new way of processing transaction with TxSignService and not modify ECKeyPair and add interfaces at this level to allow various KeyPair implementations (hardware wallets, KMS...), because at the end these are KeyPair (local vs remote) that could be used to do more than signatures.
Maybe I miss something but it seemed to me more straightforward.

@andrii-kl
Copy link
Contributor

andrii-kl commented Feb 17, 2022

@olivierayache If you take a look TxHSMSignService class which implement TxSignService interface.
You will see that to pass required parameters you should use/extend HSMPass entity, it can have what ever you need.
So HSMPass is much more abstract entity than KeyPair.

@andrii-kl
Copy link
Contributor

@olivierayache Do you have any additional questions?
Can we close this issues?

@gtebrean
Copy link
Contributor

closing this as we haven't got any reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants