You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I would like to raise an issue regarding the handling of HMAC secret keys in the library. Currently, the library requires placement of the HMAC secret key inside the keyInfoProvider during HMAC signature verification. However, the KeyInfoProvider is meant to contain public keys, not secret keys. In the context of HMAC, getKeyInfo() (or getKeyInfoContent()) is not needed.
Also, putting a HMAC key in the keyInfoProvider during the signing process poses a potential security risk as it could lead to the leakage of the HMAC secret key inside the KeyInfo element.
I propose that the library should adopt a more appropriate approach for HMAC verification. Instead of using the KeyInfoProvider, it would be advisable to use the signingKey property for both HMAC signing and verification, as it ensures consistency in symmetric key usage.
This change would have the additional benefit of KeyInfoProvider no longer having to handle HMAC keys, and parsing solely PEM keys.
Suggested Solution:
Update the documentation to emphasize the use of the signingKey property for HMAC verification
Do not use keyInfoProvider during HMAC signing and verifying
Fix the HMAC tests
Please let me know if any further information is needed.
I'm not sure that leaking a private HMAC key is actually a problem if we turn the entire KeyInfo class into having only static members because the only place an HMAC key would come into play would be if it were set as a fallback because we can't encode a binary HMAC key into an <X509Certificate /> element (and we can check before embedding too).
Your points are well taken, @cjbarth. I do agree that a well-implemented system might not leak an HMAC secret key even if the KeyInfo class were made entirely static. However, my concern stems more from the perspective of a strict separation of concerns and the potential misuse of the system by developers not entirely familiar with its intricacies.
By including the HMAC secret key in the KeyInfoProvider, the library appears to be conflating the roles of public and secret keys.
Using the signingKey property for HMAC verification enforces the symmetric nature of HMAC keys and ensures consistency in key usage. Additionally, it would separate the handling of public keys (in KeyInfoProvider) from the handling of secret keys (in the signingKey property), thereby enhancing clarity in key management.
These are not criticisms of the current library but suggestions to further refine and improve it.
Activity
cjbarth commentedon Jun 10, 2023
I'm not sure that leaking a private HMAC key is actually a problem if we turn the entire
KeyInfoclass into having only static members because the only place an HMAC key would come into play would be if it were set as a fallback because we can't encode a binary HMAC key into an<X509Certificate />element (and we can check before embedding too).Am I missing something?
shunkica commentedon Jun 10, 2023
Your points are well taken, @cjbarth. I do agree that a well-implemented system might not leak an HMAC secret key even if the KeyInfo class were made entirely static. However, my concern stems more from the perspective of a strict separation of concerns and the potential misuse of the system by developers not entirely familiar with its intricacies.
By including the HMAC secret key in the KeyInfoProvider, the library appears to be conflating the roles of public and secret keys.
Using the signingKey property for HMAC verification enforces the symmetric nature of HMAC keys and ensures consistency in key usage. Additionally, it would separate the handling of public keys (in KeyInfoProvider) from the handling of secret keys (in the signingKey property), thereby enhancing clarity in key management.
These are not criticisms of the current library but suggestions to further refine and improve it.