----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66621/#review202608 -----------------------------------------------------------
Thanks for your contribution Clement. I appreciate how you put large efforts in matching the style already -- some tiny nits left. 3rdparty/libprocess/include/process/jwt.hpp Lines 23 (patched) <https://reviews.apache.org/r/66621/#comment284494> Why do we need this? There is a `typedef struct rsa_st RSA;` in "openssl/ossl_typ.h" - commonly dragged in by "openssl/rsa.h", no? 3rdparty/libprocess/include/process/jwt.hpp Line 108 (original), 124 (patched) <https://reviews.apache.org/r/66621/#comment285207> Thanks for this one! :) 3rdparty/libprocess/src/jwt.cpp Line 18 (original) <https://reviews.apache.org/r/66621/#comment284495> Please leave this blank as it was. 3rdparty/libprocess/src/jwt.cpp Lines 23 (patched) <https://reviews.apache.org/r/66621/#comment284493> This needs to go below the other system header, "memory". See https://github.com/apache/mesos/blob/e6298aef83039dacc80b8e2a8778efacbaa63efc/docs/c%2B%2B-style-guide.md#order-of-includes 3rdparty/libprocess/src/jwt.cpp Lines 304 (patched) <https://reviews.apache.org/r/66621/#comment285201> Comments should generally terminate with punctuation to make it easy for us readers to parse them in whole. Add the missing trailing periods here and everywhere else in your new comments please. 3rdparty/libprocess/src/jwt.cpp Lines 356-359 (patched) <https://reviews.apache.org/r/66621/#comment284504> Let's lighten this up for the reader by having a temporary that holds the concatenated message; ``` const string message = base64::encode_url_safe(stringify(header), false) + "." + base64::encode_url_safe(stringify(payload), false); onst Try<string> signature = sign_rsa_sha256(message, privateKey); ``` 3rdparty/libprocess/src/ssl/utilities.cpp Line 22 (original), 24 (patched) <https://reviews.apache.org/r/66621/#comment284500> Not yours, but could you please move this include up, below "memory"? 3rdparty/libprocess/src/ssl/utilities.cpp Lines 30 (patched) <https://reviews.apache.org/r/66621/#comment284501> Please move this up below "string" (which is below "memory" then) :). 3rdparty/libprocess/src/ssl/utilities.cpp Lines 40 (patched) <https://reviews.apache.org/r/66621/#comment284503> So far we did not use `using std::string;` but referenced `std::string` a bunch of times (see e.g. line 103). Lets make sure things remain consistent by adapting towards one of those directions, but entirely. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 385 (patched) <https://reviews.apache.org/r/66621/#comment284496> Let's compare towards a `nullptr` here please. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 390 (patched) <https://reviews.apache.org/r/66621/#comment284497> `if (rsa == nullptr)` would be more common within the Mesos codebase. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 433 (patched) <https://reviews.apache.org/r/66621/#comment284499> Would it make sense to use `ERR_reason_error_string(ERR_get_error())` instead? Also note that this would print an error like this for reasons that are unknown (returning a nullptr): "Failed to sign the message: " -- that looks broken. Can we please have it show no colon at all for such case? The ternary operator is what we prefer for those purposes. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 436-437 (patched) <https://reviews.apache.org/r/66621/#comment284498> Reformat please: ``` return string( reinterpret_cast<char*>(signatureData.data()), signatureLength); ``` Argument continuations have a single argument per line. 3rdparty/libprocess/src/tests/jwt_keys.hpp Lines 17 (patched) <https://reviews.apache.org/r/66621/#comment285205> Lets rephrase a little here please; ``` Private and public keys used for JWT tests. ``` 3rdparty/libprocess/src/tests/jwt_keys.hpp Lines 74 (patched) <https://reviews.apache.org/r/66621/#comment285206> We commonly trail such `#endif` by a comment referencing the opening of the condition, making navigation for the reader a bit less painful, hopefully :) ``` #endif // __JWT_KEYS_HPP__ ``` 3rdparty/libprocess/src/tests/jwt_tests.cpp Line 31 (original), 36-37 (patched) <https://reviews.apache.org/r/66621/#comment285204> Switch these two to alphabetize. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 324 (patched) <https://reviews.apache.org/r/66621/#comment285200> Period. 3rdparty/libprocess/src/tests/jwt_tests.cpp Line 179 (original), 338 (patched) <https://reviews.apache.org/r/66621/#comment285198> Terminate the comment with punctuation, please - add a period here. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 351 (patched) <https://reviews.apache.org/r/66621/#comment285199> Period. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 365 (patched) <https://reviews.apache.org/r/66621/#comment285197> Period. - Till Toenshoff On April 21, 2018, 11:22 a.m., Clement Michaud wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66621/ > ----------------------------------------------------------- > > (Updated April 21, 2018, 11:22 a.m.) > > > Review request for mesos, Alexander Rojas and Till Toenshoff. > > > Bugs: MESOS-8788 > https://issues.apache.org/jira/browse/MESOS-8788 > > > Repository: mesos > > > Description > ------- > > Add support for alg RS256 to JWT library. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/jwt.hpp > 768cbf6fa91537ff9f45f236f4033097c5cea959 > 3rdparty/libprocess/include/process/ssl/utilities.hpp > b7cc31c33fd35c93754407f8b350eeb993177f1d > 3rdparty/libprocess/src/jwt.cpp 921031e6fe3ced5a6be6bc96190fae6d8282ae26 > 3rdparty/libprocess/src/ssl/utilities.cpp > 4d3727daf53ec62a19255da5a9804d342e770ec2 > 3rdparty/libprocess/src/tests/jwt_keys.hpp PRE-CREATION > 3rdparty/libprocess/src/tests/jwt_tests.cpp > eb36a9aed3b11208c7cdc6f20b5347f46821a207 > > > Diff: https://reviews.apache.org/r/66621/diff/4/ > > > Testing > ------- > > make check > > I added the same tests than the ones for HS256 (i.e., validation in following > cases: bad header, bad payload, unknown alg, unsupported alg, valid token > etc.. and creation of a valid token). I also added a test to verify that the > validation of a RS256 token fails when using the wrong public key. > > > Thanks, > > Clement Michaud > >
