----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66621/#review201514 -----------------------------------------------------------
3rdparty/libprocess/include/process/jwt.hpp Lines 19 (patched) <https://reviews.apache.org/r/66621/#comment282787> I really would prefer, if possible, to forward declare `RSA_shared_ptr` here and leave in include in the cpp. 3rdparty/libprocess/include/process/ssl/utilities.hpp Lines 140 (patched) <https://reviews.apache.org/r/66621/#comment282788> I'm not so sure we gain too much by declaring this typedef. Moreover, we use typedefs rather sparecely in the code base, so I would get away with it. 3rdparty/libprocess/include/process/ssl/utilities.hpp Lines 189-190 (patched) <https://reviews.apache.org/r/66621/#comment282786> This comment needs an update. 3rdparty/libprocess/src/jwt.cpp Lines 303 (patched) <https://reviews.apache.org/r/66621/#comment282789> We don't use `auto` unless the type is obvious from the lhs (like a call to `new Type` or `make_shared<Type>`) but in this case it is not obvious. 3rdparty/libprocess/src/jwt.cpp Lines 364-365 (patched) <https://reviews.apache.org/r/66621/#comment282794> This could be better formatted: ```c++ return JWT( header, payload, base64::encode_url_safe(signarure.ger(), false)); ``` 3rdparty/libprocess/src/ssl/utilities.cpp Lines 374 (patched) <https://reviews.apache.org/r/66621/#comment282790> **Here and below** This files apparently uses snake case everywhere instead of snake case. Could you rectify that here in your function names and the variable names withing these functions? 3rdparty/libprocess/src/ssl/utilities.cpp Lines 382 (patched) <https://reviews.apache.org/r/66621/#comment282795> missed an space between `if` and `(`. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 401-406 (patched) <https://reviews.apache.org/r/66621/#comment282797> Note that we tend to avoid using `std::string` or `std::vector` in cpp files. Instead, at the top of the file add: ```c++ using std::string; using std::vector; ``` 3rdparty/libprocess/src/ssl/utilities.cpp Lines 405 (patched) <https://reviews.apache.org/r/66621/#comment282796> missed `#include <vector>` 3rdparty/libprocess/src/ssl/utilities.cpp Lines 415 (patched) <https://reviews.apache.org/r/66621/#comment282793> let's avoid one letter variable names. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 423 (patched) <https://reviews.apache.org/r/66621/#comment282798> missed an space between `if` and `(`. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 427 (patched) <https://reviews.apache.org/r/66621/#comment282799> I'd add an empty line here. - Alexander Rojas On April 16, 2018, 11 p.m., Clément Michaud wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66621/ > ----------------------------------------------------------- > > (Updated April 16, 2018, 11 p.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/2/ > > > 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, > > Clément Michaud > >
