snowflake-jdbc icon indicating copy to clipboard operation
snowflake-jdbc copied to clipboard

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

Open meastham opened this issue 3 years ago • 6 comments

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 #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
    • [X] 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)

meastham avatar Apr 15 '22 18:04 meastham

@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.

sfc-gh-mknister avatar May 10 '22 00:05 sfc-gh-mknister

@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.

meastham avatar May 10 '22 14:05 meastham

@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?

sfc-gh-igarish avatar Oct 21 '22 18:10 sfc-gh-igarish

@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.

meastham avatar Oct 24 '22 14:10 meastham

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

sfc-gh-igarish avatar Mar 10 '23 01:03 sfc-gh-igarish

@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.

malthe avatar Mar 14 '23 09:03 malthe