Skip to content

Conversation

@aahajj
Copy link

@aahajj aahajj commented Jun 21, 2025

I've added some improvements to pycose:

  • Updated the README file.
  • Added a check for the content-type parameter — RFC 9052 says it can't have leading or trailing spaces and must follow the <type-name>/<subtype-name> format.
  • Added checks to make sure the user can't put the alg and crit header parameters in the unprotected header, as per RFC 9052.
  • Added checks to ensure correct usage of nonces and key lengths, following RFC 9053.

@BrianSipos
Copy link
Contributor

Is it correct to actually prohibit the alg header from the unprotected map? Even some of the examples from RFC 9052 contain unprotected alg headers, such as for the A128KW algorithm which does not support AAD and so would not be able to protect a header map.

@aahajj
Copy link
Author

aahajj commented Jun 23, 2025

@BrianSipos Thank you! You're absolutely right. I overlooked AES-KW. What I had in mind was this part of RFC 9052:

This header parameter MUST be authenticated where the ability to do so exists. This support is provided by AEAD algorithms or construction (e.g., COSE_Sign and COSE_Mac0). This authentication can be done either by placing the header parameter in the protected-header-parameters bucket or as part of the externally supplied data.

From this, I understood that placing alg in the unprotected header is inappropriate if authentication is possible.

Since Keywrap doesn’t support AAD, it might make sense to prohibit alg in unprotected headers for all other message types with an exception for algorithms like AES128KW. Would you agree?

@BrianSipos
Copy link
Contributor

BrianSipos commented Jun 24, 2025

It is true that there are two categories of encrypt algorithms: those which support AAD and those which do not. For those that do support it your logic applies to require protection of alg and crit, for those that do not support it the pycose encoder and decoder should probably require that the protected header be empty. That would avoid user expectation that whatever is in that header is in fact protected.

The algorithms I know to not support AAD are:

@BrianSipos
Copy link
Contributor

The algorithm class _RsaOaep (or its children) should be marked as not-authenticating as well.

I think the current constraint is good (cannot be in uhdr when algorithm supports AD) but the opposite can also be enforced: the phdr must be empty if the algorithm does not support AD. The COSE spec does not explicitly prohibit this I think, which might be an errata.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 3, 2025

@aahajj
Copy link
Author

aahajj commented Jul 3, 2025

Yup, that makes sense. Thank you.

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