Skip to content

crypto/ciphers: remove unneeded max_key_size in cipher_interface_st#16253

Merged
PeterKietzmann merged 1 commit into
RIOT-OS:masterfrom
Ollrogge:ciphers_PR
Apr 6, 2021
Merged

crypto/ciphers: remove unneeded max_key_size in cipher_interface_st#16253
PeterKietzmann merged 1 commit into
RIOT-OS:masterfrom
Ollrogge:ciphers_PR

Conversation

@Ollrogge

@Ollrogge Ollrogge commented Mar 30, 2021

Copy link
Copy Markdown
Member

Contribution description

This PR removes the max_key_size member of the cipher_interface_st. The only place I found where this struct member is used is this check. To me this check seems unnecessary. In my opinion it should be the job of the specific cipher modules, e.g. aes.c, to have checks related to key size.

Testing procedure

This change does not alter the behavior of any cipher. All tests under /tests/sys_crypto should still pass.

Issues/PRs references

#16183

@PeterKietzmann PeterKietzmann added Area: security Area: Security-related libraries and subsystems Area: sys Area: System Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 30, 2021
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 30, 2021
@PeterKietzmann

Copy link
Copy Markdown
Member

Overall, it seems to me that the ciphers module has a couple of parameters and implementation details to rethink and cleanup. This is one of them. ciphers aims to provide transparent access to varying block ciphers. Block chaining modes (cbc, ccm, ctr, ...) take advantage of this and can be re-used. Thereby, the blocksize needs to be accessed. The keysize, however, can and should be handled by the specific cipher implementation. Additionally, I cannot even think of the "max" keysize being helpful in the context of the meta module. ACK from my side.

@cholin was the original author. Do you have further insights?

@leandrolanzieri leandrolanzieri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree that the check of the key_size in cipher_init is limited and should be done properly in the cipher-specific init function. In fact, the only implementer of the interface has to do a second check of the key_size already.

LGTM, just a small potential improvement.

Comment thread sys/include/crypto/ciphers.h Outdated

@PeterKietzmann PeterKietzmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tests/sys_crypto and tests/unittests/tests-hashes still pass on native and nrf51dk (the latter of which includes CMAC that utilizes ciphers internally)

@PeterKietzmann PeterKietzmann added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Apr 6, 2021
@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 6, 2021
@leandrolanzieri

Copy link
Copy Markdown
Contributor

Looks good to me, running a fresh CI build just in case.

@PeterKietzmann

Copy link
Copy Markdown
Member

Well then, go!

@PeterKietzmann
PeterKietzmann merged commit c8cb79c into RIOT-OS:master Apr 6, 2021
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: security Area: Security-related libraries and subsystems Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants