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

Added RSA key file support #82

Merged
merged 8 commits into from
May 5, 2022
Merged

Conversation

ggruening
Copy link
Contributor

Hello everybody,

some applications (like mysql for example) need key files in RSA format. I prepared RSA key conversion using openssl. You can enable it by setting CONVERT_KEY2RSA=yes.
If you think this is useful I'd update the README, too.

Best regards, and thanks for this fantastic tool.

Gregor

@kereis kereis self-assigned this Apr 26, 2022
@kereis kereis added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Apr 26, 2022
Copy link
Owner

@kereis kereis left a comment

Choose a reason for hiding this comment

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

Hey, thank you for your contribution.

I have 2 suggestions:

  1. Renaming CONVERT_KEY2RSA to CONVERT_KEY_TO_RSA.
  2. As the new flag will affect all keys involved in the extraction process, we should consider renaming it into CONVERT_KEYS_TO_RSA (with KEYS having a plural 's')

@ggruening
Copy link
Contributor Author

I gladly took up your suggestions, see my second commit.

@ggruening ggruening requested a review from kereis April 26, 2022 17:09
Copy link
Owner

@kereis kereis 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 applying the suggestions. 👍

Another change that I have to request: #78 was merged recently which introduces a new way of specifying file names and extensions for both certificate and private key file. So you may adjust convert_keys_to_rsa() accordingly. See my review comments.

@kereis kereis added this to the Release 1.6.0 milestone May 1, 2022
@ggruening
Copy link
Contributor Author

Hey ho, I did it similar to #78, should be consistent again.

@ggruening ggruening requested a review from kereis May 3, 2022 14:56
Copy link
Owner

@kereis kereis left a comment

Choose a reason for hiding this comment

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

Thanks.

One tiny thing though: Can you add your new environment variables to the table of README.md? Afterwards, we can go ahead and merge your changes.

@ggruening
Copy link
Contributor Author

I just added the new vars to the table of README.md. Furthermore I added an usage example. Hope this helps.

@ggruening ggruening requested a review from kereis May 4, 2022 19:26
Copy link
Owner

@kereis kereis left a comment

Choose a reason for hiding this comment

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

LGTM.

I will merge this one. Thank you for your contribution. 😄 👍

@kereis kereis merged commit 0cb8194 into kereis:develop May 5, 2022
@ggruening
Copy link
Contributor Author

You're welcome. Thanks again for your work. :)

@ggruening ggruening deleted the rsa-key-support branch May 6, 2022 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants