Skip to content

Added initial support for CMAC #69

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 10 commits into
base: main
Choose a base branch
from
Open

Conversation

NinjaRat84
Copy link

Introduces initial support for the CMAC (Cipher-based Message Authentication Code) algorithm to the cryptography-kotlin library.

@NinjaRat84 NinjaRat84 marked this pull request as draft March 27, 2025 09:51
@NinjaRat84
Copy link
Author

This pull request is up for discussion, since I have a need for CMAC support in this library.
But not comfortable with the structure yet, so therefore a draft.

All input is welcome :)

@whyoleg
Copy link
Owner

whyoleg commented Mar 30, 2025

Hey!
Thanks for the PR, do you mind making the API similar to HMAC?

@NinjaRat84
Copy link
Author

Glad to be of service :)
Similar to this then, or any other things that looks weird?

@whyoleg
Copy link
Owner

whyoleg commented Apr 11, 2025

I was thinking that it should have the same SignatureVerifier and SignatureGenerator based API there instead of update/reset.

Something like this:

@SubclassOptInRequired(CryptographyProviderApi::class)
public interface CMAC : CryptographyAlgorithm {
    override val id: CryptographyAlgorithmId< CMAC > get() = Companion

    public companion object : CryptographyAlgorithmId<CMAC>("CMAC")

    public fun keyDecoder(): KeyDecoder<Key.Format, Key>
    public fun keyGenerator(keySize: BinarySize): KeyGenerator<Key>

    @SubclassOptInRequired(CryptographyProviderApi::class)
    public interface Key : EncodableKey<Key.Format> {
        public fun signatureGenerator(): SignatureGenerator
        public fun signatureVerifier(): SignatureVerifier

        public enum class Format : KeyFormat { RAW }
    }
}

The only problem I see for now in this API is that we don't declare that it is AES based CMAC.
I don't know if in this case it might be better to call the interface AESCMAC (or even move it under the AES.CMAC).
At this point, I'm not sure, what will be the best API wise, but AES.CMAC placement is my favorite on current moment. In this case, it's only needed to declare the following in AES interface I believe:

    @SubclassOptInRequired(CryptographyProviderApi::class)
    public interface CMAC : AES<CMAC.Key> {
        override val id: CryptographyAlgorithmId<CMAC> get() = Companion

        public companion object : CryptographyAlgorithmId<CMAC>("AES-CMAC")

        @SubclassOptInRequired(CryptographyProviderApi::class)
        public interface Key : CMAC.Key {
            public fun signatureGenerator(): SignatureGenerator
            public fun signatureVerifier(): SignatureVerifier
        }
    }

If you still understand the idea (I know, that my description is a bit messy) and ready to continue working on the PR, I would be glad to help and will try in future to answer questions faster :)

In any case, thanks for the PR!

@NinjaRat84
Copy link
Author

So basically this approach instead?

// Dummy data
val key = CryptographyRandom.nextBytes(16)
val salt = CryptographyRandom.nextBytes(16)

// getting default provider
val provider = CryptographyProvider.Default

// getting CMAC algorithm
val cmacProvider = provider.get(AES.CMAC)

// initializing CMAC
val decodedKey = cmacProvider.keyDecoder().decodeFromByteArrayBlocking(Key.Format.RAW, key)
val signFunction = decodedKey.signatureGenerator().createSignFunction()

// Update salt
signFunction.update(salt)

// Finalize CMAC
val derivedKey = signFunction.signToByteArray()

@whyoleg
Copy link
Owner

whyoleg commented Apr 15, 2025

So basically this approach instead?

Yes, nice, thank you! I know that the naming with sign/verify is not the best (borrowed from WebCrypto), but still it's better to be uniform.

Feel free to convert the PR from draft when it will be ready, so I will do a proper API/implementation review.

@NinjaRat84 NinjaRat84 marked this pull request as ready for review April 17, 2025 06:31
@NinjaRat84
Copy link
Author

Note: CMAC is only available for OpenSSL3 and JDK with BouncyCastle, not sure what I need to do in order to fix JDK tests to only run for BC.

But should be close to merge-ready now. :)

@whyoleg
Copy link
Owner

whyoleg commented Apr 24, 2025

Note: CMAC is only available for OpenSSL3 and JDK with BouncyCastle, not sure what I need to do in order to fix JDK tests to only run for BC.

You need to add an additional branch here, saying that it's not supported on specific providers

Also, could you please AES.CMAC here?

Copy link
Owner

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

One more time, thanks for the PR and a great work!
I've left several comments. Could you take a look?
If you need any help, just ping me!

@@ -87,3 +87,10 @@ data class DerivedSecretData(
val input: ByteStringAsString,
val secret: ByteStringAsString,
) : TestData

@Serializable
data class CmacData(
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be needed, and you should be able to reuse SignatureData. You can take a look on HmacCompatibilityTest on how it's used there

import dev.whyoleg.cryptography.providers.tests.api.*
import kotlin.test.*

abstract class AesCmacTestvectorsTest(provider: CryptographyProvider) : AlgorithmTest<AES.CMAC>(AES.CMAC, provider) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please provide some information, on where those testvectors are coming from?
If they are from RFC, then leave a link like in HMAC tests or if not, a bit of explanation on why those specific values are generated


abstract class AesCmacTestvectorsTest(provider: CryptographyProvider) : AlgorithmTest<AES.CMAC>(AES.CMAC, provider) {

private fun testCase(key: ByteArray, salt: ByteArray, expected: ByteArray) {
Copy link
Owner

Choose a reason for hiding this comment

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

minor: it would be nice to make the testCase accept hexes and make methods more declarative, easier to read and with less noise (similar to other testvectors tests):

    @Test
     fun testDiversifyKeyCase1() = testCase(
         keyHex = "2b7e151628aed2a6abf7158809cf4f3c",
         saltHex = "6bc1bee22e409f96e93d7e117393172a",
         resultHex = "070a16b46b4d4144f79bdd9dd04a287c"
     )

@@ -59,6 +59,7 @@ For additional limitation please consult provider specific documentation.
| | SHA3 family | ✅ | ❌ | ❌ | ✅ |
| | ⚠️ RIPEMD160 | ✅ | ❌ | ❌ | ✅ |
| **MAC** | HMAC | ✅ | ✅ | ✅ | ✅ |
| | ⚠️ CMAC | ✅ | ❌ | ❌ | ✅ |
Copy link
Owner

Choose a reason for hiding this comment

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

I believe there is no need to add ⚠️ as it's not that delicate algorithm AFAIK

@@ -54,7 +54,7 @@ kotlin {
}
commonTest.dependencies {
implementation(kotlin("test"))
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.9.0")
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:2.1.20")
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert this, as there is no such coroutines version

}

@Test
fun verifyResult() = runTestWithScope {
Copy link
Owner

Choose a reason for hiding this comment

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

minor: it would be nice in default tests to test at least that the size of signature(mac) is 128 bits

AesBasedCompatibilityTest<AES.CMAC.Key, AES.CMAC>(AES.CMAC, provider) {

@Serializable
private data class SignatureParameters(
Copy link
Owner

Choose a reason for hiding this comment

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

I believe, that there should be no parameters for CMAC here, so you can just use TestParameters.Empty in place (as in here)

logger.log { "signature.size = ${signature.size}" }

assertTrue(verifier.tryVerifySignatureBlocking(data, signature), "Initial Verify")
api.ciphers.saveData(cipherParametersId, SignatureData(keyReference, data, signature))
Copy link
Owner

Choose a reason for hiding this comment

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

api.signatures should be used here (and below in validate)
While at this point it doesn't really matter, I'm planning to implement more type-safe DSL for tests and this will be an issue :)

Comment on lines +32 to +41
val parametersList = buildList {
// size of IV = 16
(List(ivIterations) { ByteString(CryptographyRandom.nextBytes(16)) }).forEach { iv ->
generateBoolean { padding ->
val parameters = SignatureParameters(padding)
val id = api.ciphers.saveParameters(parameters)
add(id to parameters)
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
val parametersList = buildList {
// size of IV = 16
(List(ivIterations) { ByteString(CryptographyRandom.nextBytes(16)) }).forEach { iv ->
generateBoolean { padding ->
val parameters = SignatureParameters(padding)
val id = api.ciphers.saveParameters(parameters)
add(id to parameters)
}
}
}
val signatureParametersId = api.signatures.saveParameters(TestParameters.Empty)

Comment on lines +23 to +30
val cipherIterations = when {
isStressTest -> 10
else -> 5
}
val ivIterations = when {
isStressTest -> 10
else -> 5
}
Copy link
Owner

Choose a reason for hiding this comment

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

Should be just:

val dataIterations = when {
  isStressTest -> 10
  else         -> 5
}

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