Skip to content
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

Add SHA256 utility implementation #408

Merged
merged 10 commits into from
Jan 27, 2023

Conversation

emersonknapp
Copy link
Contributor

To be used for REP-2011 type version hashing.
Simple sha256 implementation to generate a 256-byte message digest for any data. Implementation originally copied from Brad Conte https://github.com/B-Con/crypto-algorithms, with modifications to fit code style and fix compiler warnings.

Prereq for ros2/rcl#1027
Part of ros2/ros2#1159

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Besides the things I've pointed out inline, we should also add tests for this implementation.

All of that said, the idea here is good.

src/sha256.c Outdated Show resolved Hide resolved
src/sha256.c Outdated Show resolved Hide resolved
src/sha256.c Outdated Show resolved Hide resolved
src/sha256.c Outdated Show resolved Hide resolved
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

emersonknapp commented Jan 24, 2023

Thanks for the comments. I hadn't finished doing code style last night, just wanted to make sure this was open as draft for the rep2011 discussion today.

Agreed, tests necessary.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@emersonknapp @clalancette quick question, why we are not using openssl to avoid re-implementing the function in rcutils? i was thinking that would be more cost effective for maintenance? Do we want to avoid the dependency on that intentionally? just a question.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp changed the title [WIP] Add sha256 utility implementation Add SHA256 utility implementation Jan 24, 2023
@emersonknapp
Copy link
Contributor Author

Yes, the motivation is that we want to use this function in RCL, which does not have a direct dependency on OpenSSL. ros2-security does have the OpenSSL dependency, but not all installations will enable that feature, but type version hashing will be built into rcl.

@wjwwood may also have more input on the reason

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp marked this pull request as ready for review January 25, 2023 00:01
@emersonknapp emersonknapp requested review from clalancette and removed request for wjwwood and ahcorde January 25, 2023 00:02
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left a few comments, but they are all pretty minor. Overall this looks pretty good to me.

src/sha256.c Outdated Show resolved Hide resolved
src/sha256.c Show resolved Hide resolved
src/sha256.c Outdated
Comment on lines 126 to 134
for (i = 0; i < len; ++i) {
ctx->data[ctx->datalen] = data[i];
ctx->datalen++;
if (ctx->datalen == 64) {
sha256_transform(ctx, ctx->data);
ctx->bitlen += 512;
ctx->datalen = 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it is worth it, but we could probably optimize this. In particular, we could memcpy 64 bytes at a time into ctx->data (dealing with the special case of the first block and the last block), calling sha256_transform on each 64-byte block we complete.

I'll leave it up to you whether you think that improvement is worth it and you want to try it. This is not a blocking comment from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave block-copy implementation a go. The tests still pass so it seems all right. I don't have a performance comparison but maybe it looks better this way.

src/sha256.c Outdated Show resolved Hide resolved
src/sha256.c Outdated Show resolved Hide resolved
include/rcutils/sha256.h Outdated Show resolved Hide resolved
test/test_sha256.cpp Outdated Show resolved Hide resolved
test/test_sha256.cpp Outdated Show resolved Hide resolved
test/test_sha256.cpp Outdated Show resolved Hide resolved
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. This looks good to me with green CI.

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/emersonknapp/e2b18a4fab8fcd32f47ceb057768b8e8/raw/fddd335b1bc03a0c5381e82c3b3989fae80e2d5f/ros2.repos
BUILD args: --packages-above-and-dependencies rcutils
TEST args: --packages-above rcutils
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11402

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

unrelated rclcpp test failed, rerunning Linux:

  • Linux Build Status

@clalancette
Copy link
Contributor

You can ignore that failure; it is fixed in ros2/rclcpp#2092 .

@emersonknapp
Copy link
Contributor Author

OK then 👍, I don't have merge access on this repo so that's up to you now! Thanks for the reviews

@clalancette clalancette merged commit 9570700 into ros2:rolling Jan 27, 2023
@emersonknapp emersonknapp deleted the emersonknapp/type-version-hash branch January 27, 2023 21:37
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.

3 participants