complete work to disable COSE_Sign1 and/or COSE_Sign
laurencelundblade opened this issue · 11 comments
This was started in the Mac0 commit, but didn't go through and disable all COSE signing code.
@laurencelundblade May I ask you to provide more information about this issue?
I was looking at the TEST code (CMakeLists.txt:124), by default the Mac0 tests are disabled,
do we want to disable the sign/verification test cases as well - even when a crypro backend is available?
It is not clear for me, thanks.
When someone turns on T_COSE_DISABLE_SIGN1 it should completely remove all code for sign1, including the crypto layer. The purpose of this define (and all DISABLEs) is to have only the object you need.
It is probably better to wait until the dust settles in the dev branch to do this work. There's way to much changing for signing and verification right now.
Also, before the dev branch is considered done and ready to merge, the Mac0 tests must be turned on by default in all test environments.
Should I wait for #121 to be merged before I start this or are there other changes too?
Yes, definitely wait. Way too much churn in that area right now.
Hi Laurence,
I've returned to this topic and got a bit confused by the changes on the dev branch.
Is the T_COSE_DISABLE_SIGN1 still needed? RSA and ECDSA were separated from EDDSA (sign_main and sign_eddsa).
The Sign1 structure does not belong exclusively to the sign_main part of the code, it's independent of the signature algorithm.
If I understand it correctly the generation of COSE_Sign1 has been integrated with COSE_Sign in cose_sign_sign and
cose_sign1_sign became just a compatibility layer (as the comments say).
We can generate COSE_Sign/Sign1 structures using both the sign_main and sign_eddsa signers (and the same applies for the verifiers). Would it make sense to rename/replace the T_COSE_DISABLE_SIGN1 option with a T_COSE_DISABLE_SIGN_MAIN / T_COSE_DISABLE_SIGN option for example? Or simply remove it and rely on the T_COSE_DISABLE_<> options only?
It would be easier to use a common disabling option for the sign_main module.
Should these DISABLE options be added to the tests/examples?
Thank you,
David
Probably next week before I can get to this. One things I'll say is that what's in GitHub right now for these #ifdefs is not thought out and probably not correct. I'm pretty sure we'll want an option to disable COSE_Sign because lots of people only want COSE_Sign1 and linker dead stripping won't do it.
Hi David, I’d like to change strategy here now that I’ve thought it through.
I don’t think any #ifdef is necessary at all. Anyone serious about object code reduction will use -dead_strip and that will cause the MAC code to be removed all the way into the crypto adaptor and crypto library if they’re not calling any of the public MAC APIs.
Maybe someone might think to disable T_COSE_ALGORITHM_HMAC512 and T_COSE_ALGORITHM_HMAC384 if they are only using T_COSE_ALGORITHM_HMAC256, but all that’s going to do is save 32 bytes of stack.
That leaves the issue of disabling test code when the library or crypto layer doesn’t support HMAC. The short term answer is to run all the mac tests every time. There’s no #ifdef removing them, but each tests will check to see if the algorithm is available by calling t_cose_is_algorithm_supported(). Grep for t_cose_is_algorithm_supported() and see that a lot of tests do this now with some just returning success if the algorithm is not available. Longer term I want to do a little better perhaps by the test returning a special error code to indicate they didn’t actually run so the print out of tests run is more accurate.
The issue is a little different for COSE_Sign and COSE_Sign1. You can leave that one to me if you want. My thought there is to replace T_COSE_DISABLE_SIGN1 with T_COSE_DISABLE_SIGN because lots of people only want COSE_Sign1. A #ifdef for COSE_Sign will remove a lot of code. Dead-strip won’t remove much COSE_Sign code. Vice versa, disabling COSE_Sign1 won’t remove that much code if you are using COSE_Sign so there’s not that much need to disable COSE_SIGN1.
When support is added for COSE_Mac over and above COSE_Mac0, we’ll need T_COSE_DISABLE_MAC_WITH_RECIPIENTS for reasons similar to COSE_Sign, but that’s not now.
So the only thing to do here is remove T_COSE_DISABLE_MAC0 and make the MAC tests conditional on t_cose_is_algorithm_supported.
Thank you!
Another reason to reduce the number of T_COSE_DISABLE_XXX is that the build check in tdv scales by the square of the number of these because it checks all combinations. It's also just less stuff the user has to worry about.
Thank you very much Laurence for the clarification, now I have 2 separate PRs to cover these tasks.
Removing the _DISABLE_MAC0 option is covered in this PR: #211
Replacing the _DISABLE_SIGN1 option with _DISABLE_SIGN1 is covered here: #222
This one is still in progress, there are still open questions around it.
@laurencelundblade can we close this issue?
One last fix #230 and this is closed.