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

rumqttc: TLS Key encoding fix #752

Merged
merged 9 commits into from
Nov 21, 2023

Conversation

ijager
Copy link
Contributor

@ijager ijager commented Nov 12, 2023

Type of change

Breaking Change

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.

Description

This PR lets users of rumqttc use any of the Rustls supported key encodings [RSA (PKCS#1), PKCS#8, SEC1].

This issue addresses #751

I left the Key enum as is to prevent a breaking change, however it does no longer matter if the user choses RSA or ECC. For a cleaner API I think the Key enum could be removed.

@ijager
Copy link
Contributor Author

ijager commented Nov 12, 2023

  • Commit 1 implemented this by adding a new variant.
  • Commit 2 uses rustls_pemfile::read_one to avoid breaking changes.

@ijager ijager changed the title rumqttc: Key encoding fix rumqttc: TLS Key encoding fix Nov 12, 2023
@ijager
Copy link
Contributor Author

ijager commented Nov 12, 2023

Tested with

  • RSA (PKCS#1) key for RSA
  • PKCS#8 key for RSA
  • SEC1 key for ECDSA

@ijager
Copy link
Contributor Author

ijager commented Nov 16, 2023

As discussed in #751 I have removed the Key enum. This is documented in the changelog. Examples are also updated.

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.

Thank you for the PR! Looks like Key is still there in lib.rs ( that is why CI is failing ), please fix it as well : )

PS: This is a breaking change, so can we please update the PR description to clarify it?

rumqttc/CHANGELOG.md Outdated Show resolved Hide resolved
rumqttc/src/tls.rs Outdated Show resolved Hide resolved
rumqttc/src/tls.rs Outdated Show resolved Hide resolved
rumqttc/src/tls.rs Outdated Show resolved Hide resolved
rumqttc/src/tls.rs Outdated Show resolved Hide resolved
@ijager
Copy link
Contributor Author

ijager commented Nov 16, 2023

Not sure why that test failed, seems unrelated?

@swanandx
Copy link
Member

Not sure why that test failed, seems unrelated?

Yup, it keeps failing randomly 😜

@swanandx swanandx merged commit d8b0ba0 into bytebeamio:main Nov 21, 2023
2 of 3 checks passed
@swanandx
Copy link
Member

Thank you for the PR!

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