Add SHA256 utility implementation#408
Conversation
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
|
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. |
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
@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
changed the title
|
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
requested review from
clalancette
and removed request for
ahcorde and
wjwwood
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
I've left a few comments, but they are all pretty minor. Overall this looks pretty good to me.
src/sha256.c
Outdated
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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>
clalancette
left a comment
clalancette
left a comment
There was a problem hiding this comment.
Thanks for iterating. This looks good to me with green CI.
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
|
Gist: https://gist.githubusercontent.com/emersonknapp/e2b18a4fab8fcd32f47ceb057768b8e8/raw/fddd335b1bc03a0c5381e82c3b3989fae80e2d5f/ros2.repos
|
|
unrelated rclcpp test failed, rerunning Linux:
|
|
You can ignore that failure; it is fixed in ros2/rclcpp#2092 . |
|
OK then 👍, I don't have merge access on this repo so that's up to you now! Thanks for the reviews |
3 participants
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