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

Allow rustls configuration in rumqttd #748

Closed
wants to merge 9 commits into from
Closed

Allow rustls configuration in rumqttd #748

wants to merge 9 commits into from

Conversation

nathansizemore
Copy link
Contributor

@nathansizemore nathansizemore commented Nov 9, 2023

  • Allows private keys to be any flavor supported by rustls_pemfile.
  • Allows TLS connections without client certs.

Removes setting the tenant id by client cert (keeps native and rustls impls the same).

closes #745, #747

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Copy link
Member

@swanandx swanandx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Removes setting the tenant id by client cert

We do need a way to specify tenant_id though, rather than removing this, we can make it optional? more on it in review comment.

can we tackle both mentioned issues in different PRs? easier to review / keep track of changes. Anyways, choice is up to you haha

rumqttd/src/server/tls.rs Outdated Show resolved Hide resolved
rumqttd/src/server/tls.rs Outdated Show resolved Hide resolved
rumqttd/src/server/tls.rs Outdated Show resolved Hide resolved
@nathansizemore
Copy link
Contributor Author

Thanks for the PR!

Removes setting the tenant id by client cert

We do need a way to specify tenant_id though, rather than removing this, we can make it optional? more on it in review comment.

can we tackle both mentioned issues in different PRs? easier to review / keep track of changes. Anyways, choice is up to you haha

Yeah, the tenant id seems like a bigger change than what this is intended for and it just unifies the way tenant id is used across all connection types. Getting this merged, and a publish to crates.io would be wonderful so that we can track against crates.io instead of our fork.

Let me know if there is anything else!

@swanandx
Copy link
Member

swanandx commented Nov 12, 2023

Hey, can we please this PR only related to allowing TLS connections without client certs ( with changes suggested in review comments )

as issue #745 is already tackled by PR #743. My apologies for messing up 😬

Thank you for understanding!

@nathansizemore
Copy link
Contributor Author

Hey, can we please this PR only related to allowing TLS connections without client certs ( with changes suggested in review comments )

as issue #745 is already tackled by PR #743. My apologies for messing up 😬

Thank you for understanding!

So the impetus for this PR was we need both. This now contains both changes, ready, and the other is still in draft.

@swanandx
Copy link
Member

It's not just because there is other PR 😅 Separate PRs are helpful as mentioned in this comment.

As these two changes are quite different, merging them in single commit won't be much helpful right. By having em separately will make it easier to undo/revert/reason about in future!

wdyt?

@nathansizemore
Copy link
Contributor Author

Thanks for the PR!

Removes setting the tenant id by client cert

We do need a way to specify tenant_id though, rather than removing this, we can make it optional? more on it in review comment.

can we tackle both mentioned issues in different PRs? easier to review / keep track of changes. Anyways, choice is up to you haha

Its not removed. It is feature gated as requested in the previous comment if you look I made a comment saying so. I mean wanting it in separate PRs is up to you. This was free work and free contribution. I'm OK with maintaining a fork. THe code is yours do with what you want, but I won't be contributing or splitting up any more of this work. Feel free to tackle that if you want!

@swanandx
Copy link
Member

Its not removed. It is feature gated as requested in the previous comment if you look I made a comment saying so.

I wanted to quote the separate PR part only.

I mean wanting it in separate PRs is up to you. This was free work and free contribution. I'm OK with maintaining a fork

I didn't mean to disrespect your/anyone's work/contribution 😕 my apologies if it came out that way!

@nathansizemore
Copy link
Contributor Author

Its not removed. It is feature gated as requested in the previous comment if you look I made a comment saying so.

I wanted to quote the separate PR part only.

I mean wanting it in separate PRs is up to you. This was free work and free contribution. I'm OK with maintaining a fork

I didn't mean to disrespect your/anyone's work/contribution 😕 my apologies if it came out that way!

No disrespect taken, all good here :). I needed something fixed, so instead of just creating an issue, I submitted the changes back to the project. If you want them, feel free to merge them. They work for us, I have a fork, if they get merged cool, if not, cool. We needed rustls without client certs, we now have that. Its yours to do with what you want!

@swanandx
Copy link
Member

will be closing this PR in favor of #756 & #743 !

Thank you so much for your contribution and pointing out the issues 🚀

@swanandx swanandx closed this Nov 23, 2023
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.

rumqttd: Configurable or "smart" RusTLS options.
3 participants