Skip to content

Type-safe Security Configuration #88

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 18, 2023
Merged

Type-safe Security Configuration #88

merged 4 commits into from
Jul 18, 2023

Conversation

felixschlegel
Copy link
Contributor

Note: this PR sits on top of #87.

Adds some of the missing properties from #46.

Motivation:

We were lacking some of the security protocol configuration options and
want to provide them in a type-safe manner.

Modifications:

  • add Kerberos support
  • add OAuthBearer support
  • Created a new file KafkaConfiguration+Security.swift
    • add public struct SSLConfiguration
    • add public struct SASLMechanism
    • add public struct KerberosConfiguration
    • add public struct OAuthBearerMethod
    • add public struct SecurityProtocol
  • integrate KafkaConfiguration.SSLOptions in new types
  • integrate KafkaConfiguration.SASLOptions in new types

Example:

var config = KafkaConsumerConfiguration(
    consumptionStrategy: .partition(.unassigned, topic: "some topic")
)
config.securityProtocol = .saslSSL(
    saslMechanism: .gssapi(kerberosConfiguration: .init(keytab: "/path/to/keytab")),
    sslConfiguaration: .keyPair(
        privateKey: .init(
            location: .pem("jfklaffalk"),
            password: "iLoveApple"
        ),
        publicKeyCertificate: .file(location: "/path/to/file"),
        caCertificate: .pem("jfkaljfl"),
        crlLocation: nil
    )
)

@felixschlegel felixschlegel force-pushed the fs-kafka-ssl-sasl-configuration branch from 81621d3 to 1d10d99 Compare July 14, 2023 17:26
@felixschlegel felixschlegel mentioned this pull request Jul 14, 2023
Merged
@felixschlegel
Type-safe Security Configurations
b7d57fb 
> Adds some of the missing properties from swift-server#46.

Motivation:

We were lacking some of the security protocol configuration options and
want to provide them in a type-safe manner.

Modifications:

* add Kerberos support
* add OAuthBearer support
* Created a new file `KafkaConfiguration+Security.swift`
    * add `public struct` `SSLConfiguration`
    * add `public struct` `SASLMechanism`
    * add `public struct` `KerberosConfiguration`
    * add `public struct` `OAuthBearerMethod`
    * add `public struct` `SecurityProtocol`
* integrate `KafkaConfiguration.SSLOptions` in new types
* integrate `KafkaConfiguration.SASLOptions` in new types
@felixschlegel felixschlegel force-pushed the fs-kafka-ssl-sasl-configuration branch from 1d10d99 to b7d57fb Compare July 17, 2023 12:23
dnadoba
dnadoba reviewed Jul 17, 2023
View reviewed changes
Sources/SwiftKafka/Configuration/KafkaConfiguration+Security.swift Outdated
public static func keyPair(
privateKey: PrivateKey,
publicKeyCertificate: Key,
caCertificate: Key,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should provide an easy way to use the "default" i.e. "probe" that searches for locally trusted CAs.

dnadoba
dnadoba reviewed Jul 17, 2023
View reviewed changes
Sources/SwiftKafka/Configuration/KafkaConfiguration+Security.swift Outdated
/// Use to configure an SSL connection.
public struct SSLConfiguration: Sendable, Hashable {
/// A key either located in a file or directly as a key String (PEM format).
public struct Key: Sendable, Hashable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the right term here would be Chain as it not only contains the public key but at least one certificate and possibly multiple certificates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... maybe even two types.

  • One of them is kind of a LeafAndIntermediates type as the first certificate in the chain will be used as the identity of the client and the remaining, in no particular order, will be used as potential intermediate certificates and send to the broker to verify the client.
  • The other type could be RootCertificates as it is an unordered list of trusted CAs that is used by the client to verifier the identity of the broker.

dnadoba
dnadoba reviewed Jul 17, 2023
View reviewed changes
Sources/SwiftKafka/Configuration/KafkaConfiguration+Security.swift Outdated
Comment on lines 96 to 97
publicKeyCertificate: Key,
caCertificate: Key,
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to use different types here so we can provide a reasonable default for caCertificate.

@felixschlegel
Fix failing test
0cef685
@felixschlegel
Changes David
4fe1af9 
Modifications:

* go from `KafkaConfiguration.Key` type into more specific types:
    * `LeafAndIntermediates` to extract the public Key
    * `PrivateKey`
    * `RootCertificate`
* `RootCertificate`: add two new options
    * `probe` (default): probe list of standard paths and make first
     certificate found root default root certificate location path
    * `disableBrokerVerification`: disable broker verification entirely
* `KafkaConfiguration+Security`:
    * turn `static func`s without parameters into `static let`s for our
      enum-like `public structs`
@felixschlegel felixschlegel requested a review from dnadoba July 17, 2023 16:13
dnadoba
dnadoba reviewed Jul 18, 2023
View reviewed changes
Copy link
Contributor

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

Do we allow users to modify the config dictionary? e.g. to allow users using config options the swift wrappers do not yet support?

Sources/SwiftKafka/Configuration/KafkaConfiguration+Security.swift
/// Available SASL mechanisms that can be used for authentication.
public struct SASLMechanism: Sendable, Hashable {
/// Used to configure Kerberos.
public struct KerberosConfiguration: Sendable, Hashable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a public init missing

Sources/SwiftKafka/Configuration/KafkaConfiguration+Security.swift
)

/// Use the Secure Sockets Layer (SSL) protocol.
public static func ssl(configuration: SSLConfiguration) -> SecurityProtocol {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Lukasa should this be called TLS instead of SSL?

@felixschlegel
Review David
276fa2b 
Modifications:

* add `public init` to `KafkaConfiguration.SASLMechanism.KerberosConfiguration`
@felixschlegel felixschlegel requested a review from dnadoba July 18, 2023 10:30
dnadoba
dnadoba approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

Awesome! Looks good to me

@FranzBusch FranzBusch merged commit 1024f29 into swift-server:main Jul 18, 2023
@FranzBusch FranzBusch deleted the fs-kafka-ssl-sasl-configuration branch July 18, 2023 12:14
@felixschlegel felixschlegel mentioned this pull request Jul 18, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FranzBusch FranzBusch FranzBusch approved these changes

@dnadoba dnadoba dnadoba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felixschlegel @FranzBusch @dnadoba