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

Create KafkaCredentialStore #530

Merged
merged 19 commits into from
Apr 6, 2021
Merged

Conversation

agustafson
Copy link
Contributor

Fix for #521

Moving the code in https://github.com/ovotech/ciris-aiven-kafka for credential loading.

Example code using ciris would now be:

def loadKafkaSetup[F[_]: Async: ContextShift]: Resource[F, KafkaCredentialStore] = {
  implicit val stringClientPrivateKeyConfigDecoder: ConfigDecoder[String, ClientPrivateKey] =
    ConfigDecoder.identity[String].mapOption("ClientPrivateKey") {
      ClientPrivateKey(_).toOption
    }
  implicit val stringClientCertificateConfigDecoder: ConfigDecoder[String, ClientCertificate] =
    ConfigDecoder.identity[String].mapOption("ClientCertificate") {
      ClientCertificate(_).toOption
    }
  implicit val stringServiceCertificateConfigDecoder: ConfigDecoder[String, ServiceCertificate] =
    ConfigDecoder.identity[String].mapOption("ServiceCertificate") {
      ServiceCertificate(_).toOption
    }

  Blocker[F].evalMap { blocker =>
    (
      env("KAFKA_CLIENT_PRIVATE_KEY").as[ClientPrivateKey],
      env("KAFKA_CLIENT_CERT").as[ClientCertificate],
      env("KAFKA_SERVER_CERT").as[ServiceCertificate]
    ).parTupled.load[F].flatMap {
      case (clientPrivateKey, clientCertificate, serviceCertificate) =>
        KafkaCredentialStore[F](clientPrivateKey, clientCertificate, serviceCertificate, blocker)
    }
  }
}

import java.security.MessageDigest
import scala.annotation.tailrec

package object security {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could consider moving it to a separate module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @LMnet, I did wonder about that. It just seemed like it wasn't that separate from the core. Happy to move it out if that's what people want 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this code is blatantly copied from Secret in ciris. Perhaps it should be its own library, because I'm sure I've seen code very similar to this in a few different places.

Copy link
Member

Choose a reason for hiding this comment

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

I initially agreed with @LMnet, but I think it's better to keep it in the same module so we can have the withCredentials helpers on the settings classes.

@bplommer
Copy link
Member

Thanks for this! I'll take a look soon

@bplommer bplommer self-requested a review February 16, 2021 09:26
@bplommer bplommer requested a review from vlovgr February 22, 2021 16:41
@bplommer
Copy link
Member

I assume you're happy for this code to be copied over @vlovgr?

@vlovgr
Copy link
Contributor

vlovgr commented Feb 25, 2021

I assume you're happy for this code to be copied over @vlovgr?

Absolutely. 👍

@bplommer
Copy link
Member

bplommer commented Mar 1, 2021

Kafka 2.7.0 adds support for textual SSL credentials, so we may not need this after all: apache/kafka#9345

@agustafson
Copy link
Contributor Author

Kafka 2.7.0 adds support for textual SSL credentials, so we may not need this after all: apache/kafka#9345

Happy to adapt this PR so that it can handle textual SSL creds or the code in this PR, if that would help

@bplommer
Copy link
Member

bplommer commented Mar 8, 2021

Happy to adapt this PR so that it can handle textual SSL creds or the code in this PR, if that would help

Thanks, that would be great! I guess you saw the snippet I posted on Slack, but if not here it is:

consumerSettings
  .withProperties(Map(
  "security.protocol" -> "SSL",
  "ssl.truststore.type" -> "PEM",
  "ssl.truststore.certificates" ->
    """
      -----BEGIN CERTIFICATE-----
      // CA certificate
      -----END CERTIFICATE-----
      |""".replace('\n', ' '),
  "ssl.keystore.type" -> "PEM",
  "ssl.keystore.key" -> 
    """
      -----BEGIN PRIVATE KEY-----
      // access key
      -----END PRIVATE KEY-----""".replace('\n', ' ')
    ,
  "ssl.keystore.certificate.chain" -> 
    """
      -----BEGIN CERTIFICATE-----
      // access certificate
      -----END CERTIFICATE-----
    """.replace('\n', ' ')
))

@agustafson
Copy link
Contributor Author

Happy to adapt this PR so that it can handle textual SSL creds or the code in this PR, if that would help

Thanks, that would be great! I guess you saw the snippet I posted on Slack, but if not here it is:

consumerSettings
  .withProperties(Map(
  "security.protocol" -> "SSL",
  "ssl.truststore.type" -> "PEM",
  "ssl.truststore.certificates" ->
    """
      -----BEGIN CERTIFICATE-----
      // CA certificate
      -----END CERTIFICATE-----
      |""".replace('\n', ' '),
  "ssl.keystore.type" -> "PEM",
  "ssl.keystore.key" -> 
    """
      -----BEGIN PRIVATE KEY-----
      // access key
      -----END PRIVATE KEY-----""".replace('\n', ' ')
    ,
  "ssl.keystore.certificate.chain" -> 
    """
      -----BEGIN CERTIFICATE-----
      // access certificate
      -----END CERTIFICATE-----
    """.replace('\n', ' ')
))

Cool, I'll add a few String-based methods and see how it looks

@bplommer
Copy link
Member

bplommer commented Apr 1, 2021

I haven’t forgotten about this! I’ll get to it this weekend.

@bplommer bplommer added this to the v1.5.0 milestone Apr 1, 2021
@bplommer
Copy link
Member

bplommer commented Apr 5, 2021

I don't think Pkcs12KafkaCredentialStore is needed now that we have PemKafkaCredentialStore - I'm going to edit the PR to remove it assuming that's ok with you.

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

Successfully merging this pull request may close these issues.

4 participants