Skip to content

Add support for CMAC #891

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

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

Add support for CMAC #891

wants to merge 2 commits into from

Conversation

kmfukuda
Copy link

Follow-up to #806.

Implements #802 by adding the OpenSSL::MAC and OpenSSL::MAC::CMAC classes.

The classes are defined if compiled against OpenSSL 3.

@kmfukuda kmfukuda mentioned this pull request May 21, 2025
@rhenium
Copy link
Member

rhenium commented May 21, 2025

Thanks for working on this series.

The base class OpenSSL::MAC doesn't seem to be useful on its own currently. The EVP_MAC API seems quite generic, so I wonder if we can expose the OSSL_PARAM construction part and the EVP_MAC_init() call as a method under OpenSSL::MAC that accepts arbitrary (parameter, value) pairs. That way, OpenSSL::MAC::CMAC (or other MACs supported by EVP_MAC) could be implemented entirely in lib/openssl/mac.rb without requiring a specialized extension method.

Regarding the method naming, is there particular reason for choosing #{,hex,base64}mac instead of #*digest? Since OpenSSL::HMAC already uses #*digest, I think copying it would have less friction, though I agree it might not have been the best choice originally.

The #*mac naming seems redundant, especially when used with the shorthand methods like OpenSSL::MAC::CMAC.mac(ciph, key, data). Another idea would be to use #final taken from the C API, which is what OpenSSL::Cipher does. That said I'm not sure what name I'd give to the variant that returns the hexadecimal string.

Also, we need docs.

@@ -0,0 +1,174 @@
#include "ossl.h"

#if OSSL_OPENSSL_PREREQ(3, 0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I believe LibreSSL and AWS-LC will eventually implement the EVP_MAC API too. Please use an extconf.rb check instead of relying on the version number.

@@ -187,6 +187,7 @@ extern VALUE dOSSL;
#include "ossl_digest.h"
#include "ossl_engine.h"
#include "ossl_hmac.h"
#include "ossl_mac.h"
#include "ossl_kdf.h"
Copy link
Member

Choose a reason for hiding this comment

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

Please sort them in alphabetical order.

EVP_MAC_free(mac);
ossl_raise(eMACError, "EVP_MAC_CTX_new");
}
SetMAC(self, ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Technically, #initialize and #initialize_copy can be called twice, and in that case EVP_MAC_CTX already stored in RTYPEDDATA_DATA(self) will leak. It should be prevented.

I suggest taking a look at ossl_pkey*.c which deals with this issue by raising an exception.

def hexmac
mac.unpack1('H*')
end
alias inspect hexmac
Copy link
Member

Choose a reason for hiding this comment

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

I think people would expect #inspect to show the class name or the algorithm rather than the calculated MAC.

return false unless self.mac.bytesize == other.mac.bytesize

OpenSSL.fixed_length_secure_compare(self.mac, other.mac)
end
Copy link
Member

Choose a reason for hiding this comment

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

Comparing MACs generated by different algorithms or parameters would be pointless, but this doesn't prevent it and it's probably not possible unless we store parameters or sensitive keys ourselves, which would be a bad idea.

Honestly, I'm not sure if this should be provided by this library.

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.

2 participants