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

Add the ability to set a custom key pair auth signing implementation #910

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

meastham
Copy link

@meastham meastham commented Apr 15, 2022

Overview

External contributors - please answer these questions before submitting a pull request. Thanks!

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR adressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-576663: Allow using a private key stored in AWS KMS for key pair auth #909

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am modyfying authorization mechanisms
    • I am adding new credentials
    • I am modyfying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

Allows library users to create their own implementation of PrivateKeySigner which calls KMS to do the required operations. Here is the implementation I'm using. It's Kotlin but I could convert to Java if it's needed for docs or whatever. Also for this to actually work it needs a zero-arg constructor to plumb in the dependencies from somewhere which I've omitted.

class KMSPrivateKeySigner(private val kmsClient: KmsClient, private val keyAlias: String) :
    PrivateKeySigner {
  val keyId: String = run {
    val describeRequest = DescribeKeyRequest.builder().keyId("alias/$keyAlias").build()
    kmsClient.describeKey(describeRequest).keyMetadata().keyId()
  }

  val publicKey: PublicKey = run {
    val request = GetPublicKeyRequest.builder().keyId(keyId).build()
    val response = kmsClient.getPublicKey(request)
    val spec = X509EncodedKeySpec(response.publicKey().asByteArray())
    val keyFactory = KeyFactory.getInstance("RSA")
    keyFactory.generatePublic(spec)
  }

  override fun sign(bytes: ByteArray): ByteArray {
    val request =
        SignRequest.builder()
            .keyId(keyId)
            .signingAlgorithm(SigningAlgorithmSpec.RSASSA_PKCS1_V1_5_SHA_256)
            .messageType(MessageType.RAW)
            .message(SdkBytes.fromByteArray(bytes))
            .build()
    return kmsClient.sign(request).signature().asByteArray()
  }

  override fun publicKey() = publicKey
}

Another option would be that I could just contribute this KMS support directly to this library, but that seems probably undesirable because it adds new dependencies.

Pre-review checklist

  • This change has passed precommit
  • I have reviewed code coverage report for my PR in (Sonarqube)

@sfc-gh-mknister
Copy link
Contributor

@meastham can you provide some sample code for how to generate this KMS key/pair and use it? We would need to add some unit tests to this PR.

@meastham
Copy link
Author

@meastham can you provide some sample code for how to generate this KMS key/pair and use it? We would need to add some unit tests to this PR.

There are instructions for creating a key here: https://docs.aws.amazon.com/kms/latest/developerguide/asymm-create-key.html#create-asymmetric-keys-console

And an example implementation for using the key for signing is in the PR description.

Would you actually want to unit test KMS in this repo though? It seems like it would be simpler to unit test an implementation of PrivateKeySigner that just uses a hard-coded key pair.

@sfc-gh-igarish
Copy link
Collaborator

@meastham Are you still interested in this old PR? If not please close it. I read your changes and this may trigger our internal security review. Did you think about how hackers can misuse it?

@meastham
Copy link
Author

@meastham Are you still interested in this old PR? If not please close it. I read your changes and this may trigger our internal security review. Did you think about how hackers can misuse it?

Yes, I'm still interested in having it.

I've thought through the security implications of this, and I'm not aware of any additional risks it introduces if used properly. In our case, it's a substantial net improvement from a security perspective because it avoids exposing a private key outside of our key store.

@sfc-gh-igarish
Copy link
Collaborator

@sfc-gh-hchaturvedi could you please review this feature changes?

@malthe
Copy link

malthe commented Mar 14, 2023

@meastham it seems to me that file- and string-based private key signing could implement the signer class to both:

  1. Simplify the code
  2. Make sure that this abstraction was used

There could be a unit test with a dummy implementation, but really, if the interface is implemented for file- and string-based private key signing, that's a pretty solid demonstration that the interface works.

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

Successfully merging this pull request may close these issues.

SNOW-576663: Allow using a private key stored in AWS KMS for key pair auth
4 participants