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 CMAC #806

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add support for CMAC #806

wants to merge 6 commits into from

Conversation

kmfukuda
Copy link

Implements #802 by adding the OpenSSL::CMAC class.

The class has a similar set of methods to OpenSSL::HMAC except that

  • it calls MACs mac s and that
  • it does not have #reset .

The class uses different sets of OpenSSL functions depending on the version that the class is compiled against.

  • EVP functions (OpenSSL 3)
  • older functions (other versions)

The functions are available in each supported version of OpenSSL and
LibreSSL, though deprecated in OpenSSL 3.0.
The functions are suggested in OpenSSL 3.0, though available only in
that version.
The function was removed in LibreSSL 3.9.
@rhenium
Copy link
Member

rhenium commented Oct 29, 2024

Thanks for working on this!

I wonder if we can get away with just implementing it with EVP_MAC. Given that OpenSSL versions older than 3.0 have reached EOL (see https://openssl-library.org/policies/releasestrat/index.html), we might not need to support older versions of OpenSSL for a completely new feature.

Having said that, I wonder if we want to further make the EVP_MAC wrapper more generic, rather than limiting it to just CMAC. EVP_MAC supports additional algorithms we may be interested in adding, such as KMAC and keyed BLAKE2. We'll eventually rewrite OpenSSL::HMAC around EVP_MAC, too, so it would be nice to avoid duplicated code.

Perhaps we can implement an algorithm-agnostic OpenSSL::MAC that also provides low-level access to those OSSL_PARAMs, along with a specialized CMAC class built around it.

What do you think?

Comment on lines +27 to +28
VALUE cCMAC;
VALUE eCMACError;
Copy link
Member

Choose a reason for hiding this comment

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

These can be static.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I thought they could be, but I wasn't sure.


/*
* call-seq:
* CMAC.new(key, cipher = "AES-128-CBC") -> new_cmac
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should make cipher a mandatory parameter without a default value.

Copy link
Author

Choose a reason for hiding this comment

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

I read your comment on #796, and now I agree we should.

@kmfukuda
Copy link
Author

kmfukuda commented Nov 8, 2024

That will be the best solution if we can stop supporting EOL versions of OpenSSL and all versions of LibreSSL, at least about new features. We are also supporting LibreSSL, and I think it has never had EVP_MAC functions.

I thought about almost the same approach when I started working on this issue, but I didn't know if we could soon stop supporting live versions or add features that wouldn't work in some environments. That is why I made that hybrid code.

I will try to implement OpenSSL::MAC and some subclasses if I am assured that we can stop supporting LibreSSL somehow.

In that case, we should add those subclasses under OpenSSL::MAC (e.g. OpenSSL::MAC::CMAC ) and leave OpenSSL::HMAC as it is, shouldn't we?

@rhenium
Copy link
Member

rhenium commented Nov 11, 2024

We also have a choice to define OpenSSL::MAC only when it's compiled against an OpenSSL or LibreSSL release which supports it. I didn't realize that LibreSSL doesn't support it yet, but I expect it'll catch up soon.

When the version number it will be implemented is unknown, we can check it at compile time in ext/openssl/extconf.rb.

In that case, we should add those subclasses under OpenSSL::MAC (e.g. OpenSSL::MAC::CMAC ) and leave OpenSSL::HMAC as it is, shouldn't we?

That sounds reasonable to me. We can't break OpenSSL::HMAC even if an old version of OpenSSL or LibreSSL is used, so rewriting it on top of the new interface will be a future task.

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

Successfully merging this pull request may close these issues.

2 participants