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 support for SHA3 and BLAKE digests. #282

Merged
merged 5 commits into from
Oct 31, 2019
Merged

Add support for SHA3 and BLAKE digests. #282

merged 5 commits into from
Oct 31, 2019

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Oct 31, 2019

and remove unsupported/non-existant digests.

  • DSS
  • DSS1
  • SHA
  • MDC2

I could not find any configuration of OpenSSL where it was working.

There are some interesting commands to extract all supported digests from OpenSSL: https://crypto.stackexchange.com/a/63659/74087

You can check your own implementation by running: openssl list -digest-algorithms

Feedback welcome!

@ioquatix ioquatix self-assigned this Oct 31, 2019
@ioquatix
Copy link
Member Author

@jeremyevans do you have any feedback and/or suggestions? Especially regarding removing the unsupported digests, as it means OpenSSL::Digest::MDC2 is also gone.

@jeremyevans
Copy link
Contributor

I think removing the unsupported digests is fine if we think they are already not working (if they are working, we should deprecate first). If OpenSSL supports SHA-3 and BLAKE, it makes sense for the Ruby openssl gem to support them as well. So I'm OK with merging this pull request.

@bdewater
Copy link
Contributor

bdewater commented Oct 31, 2019

Based on https://github.com/ruby/openssl/blame/v2.1.0/History.md#L7-L8 these OpenSSL versions were not supported anymore, it looks like a piece that was forgotten to clean up. LGTM 👍

We may want to introduce some helper methods to ease deprecation and clean up in the future?

@ioquatix ioquatix merged commit cac0272 into master Oct 31, 2019
@ioquatix
Copy link
Member Author

Thanks for all your feedback, @bdewater the history was super helpful.

Regarding deprecated digests, not quite sure what we should do there in the future. Defining them as classes might have been a mistake.

@ioquatix ioquatix deleted the sha3 branch October 31, 2019 20:43
@bdewater
Copy link
Contributor

bdewater commented Nov 1, 2019

Defining them as classes might have been a mistake.

Definitely something to be said for that. I only just now found out OpenSSL::Digest.hexdigest('sha256', 'a') already works after imagining what an alternative interface could look like 😄

If we promote that to the preferred interface in documentation and such we wouldn't need to have PRs like these. Sounds more flexible to me and not too hard for apps to upgrade - automatable with a regex or a Rubocop rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants