Skip to content
This repository was archived by the owner on Dec 18, 2024. It is now read-only.

Add TLS support #518

Merged

Conversation

argerus
Copy link
Contributor

@argerus argerus commented Mar 25, 2023

Basic functionality is there, but more work is needed in terms of documentation, ease of use.

@argerus argerus force-pushed the topic/add_tls_databroker branch 3 times, most recently from c8c9040 to 2a5d91c Compare March 26, 2023 13:04
@argerus argerus marked this pull request as ready for review March 26, 2023 15:19
Copy link

@int0x27 int0x27 left a comment

Choose a reason for hiding this comment

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

It would be nice to have some warnings if pub keys, ca-cert, etc. does not start with expected lines
-----BEGIN XXX-----

@argerus argerus force-pushed the topic/add_tls_databroker branch from 2a5d91c to c04dfd4 Compare April 12, 2023 14:08
@argerus argerus requested a review from int0x27 April 13, 2023 08:05
Copy link

@int0x27 int0x27 left a comment

Choose a reason for hiding this comment

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

looks good

@@ -247,14 +247,15 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
.value_name("PORT")
.required(false)
.env("KUKSA_DATA_BROKER_PORT")
.value_parser(clap::value_parser!(u16).range(1..))
.value_parser(clap::value_parser!(u16))
Copy link
Contributor

Choose a reason for hiding this comment

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

my understanding is that the port number 0 can be used to bind to any free port? If so, then I think this is worth mentioning in the property's description both in the source code as well as in the README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct (and the reason I removed the limit of > 0).

With that said, it doesn't really do much good at the moment as databroker isn't printing which port it binds to (when using port 0) as it's just printing the port provided verbatim..

I have a plan to fix it, but as it's a bit involved I didn't do it as part of this PR.
Regarding documenting this behaviour, I mean sure it doesn't hurt, but it's standard unix behaviour, right?

@SebastianSchildt SebastianSchildt merged commit 7be4669 into eclipse-archived:master Apr 17, 2023
@SebastianSchildt SebastianSchildt mentioned this pull request Apr 27, 2023
@erikbosch erikbosch deleted the topic/add_tls_databroker branch October 31, 2024 12:55
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants