Skip to content

Create KafkaCredentialStore - #530

Merged
bplommer merged 19 commits into
typelevel:series/1.xfrom
agustafson:certificate-loading
Apr 6, 2021
Merged

Create KafkaCredentialStore#530
bplommer merged 19 commits into
typelevel:series/1.xfrom
agustafson:certificate-loading

Conversation

@agustafson

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

Thanks for this! I'll take a look soon

Comment thread docs/src/main/mdoc/certificates.md Outdated
Comment thread docs/src/main/mdoc/certificates.md Outdated
Comment thread docs/src/main/mdoc/certificates.md Outdated
Comment thread docs/src/main/mdoc/certificates.md Outdated
Comment thread docs/src/main/mdoc/certificates.md
Comment thread modules/core/src/main/scala/fs2/kafka/security/TrustStoreFile.scala Outdated
@bplommer
bplommer self-requested a review February 16, 2021 09:26
Comment thread modules/vulcan/src/main/scala/fs2/kafka/vulcan/AvroSettings.scala Outdated
Comment thread modules/vulcan/src/main/scala/fs2/kafka/vulcan/SchemaRegistryClientSettings.scala Outdated
@bplommer
bplommer requested a review from vlovgr February 22, 2021 16:41
@bplommer

Copy link
Copy Markdown
Contributor

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

@vlovgr

vlovgr commented Feb 25, 2021

Copy link
Copy Markdown
Member

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

Absolutely. 👍

@bplommer

bplommer commented Mar 1, 2021

Copy link
Copy Markdown
Contributor

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

@agustafson

Copy link
Copy Markdown
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

bplommer commented Mar 8, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

bplommer commented Apr 1, 2021

Copy link
Copy Markdown
Contributor

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

bplommer commented Apr 5, 2021

Copy link
Copy Markdown
Contributor

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants