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 tls support #33

Merged
merged 8 commits into from
Feb 15, 2024
Merged

Add tls support #33

merged 8 commits into from
Feb 15, 2024

Conversation

piotrpio
Copy link
Contributor

No description provided.

Jarema and others added 7 commits February 12, 2024 12:06
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio requested review from Jarema and mtmk February 15, 2024 08:28
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

Just few comments (to my own code, I know :))

@@ -70,6 +75,27 @@ public class ClientOptions {
return self
}

public func enforceTls() -> ClientOptions {
Copy link
Member

Choose a reason for hiding this comment

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

This was a placeholder name. Let's rename to requireTls

@@ -70,6 +75,27 @@ public class ClientOptions {
return self
}

public func enforceTls() -> ClientOptions {
self.withTls = true
Copy link
Member

Choose a reason for hiding this comment

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

and maybe for consistency rename then that one too?

tlsConfiguration.privateKey = .privateKey(privateKey)
}
let sslContext = try NIOSSLContext(configuration: tlsConfiguration)
// FIXME(jrm): Consider better way to pick hostname.
Copy link
Member

Choose a reason for hiding this comment

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

How do we pick the hostname in Go?
We discussed it a bit - but maybe we should pick the hostname of the URL we are currently trying to connect to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but currently we always pick the first url anyway. I'll add url randomization in the next PR and use that one.

Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@Jarema Jarema merged commit 0f03482 into main Feb 15, 2024
1 check passed
@Jarema Jarema deleted the add-tls-support branch February 15, 2024 11:53
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.

None yet

2 participants