----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66621/#review201183 -----------------------------------------------------------
Looks great, thanks for adding this feature! Most issues are related to style, otherwise there isn't much that would need bigger changes. 3rdparty/libprocess/include/process/jwt.hpp Line 48 (original), 50 (patched) <https://reviews.apache.org/r/66621/#comment282241> Please add 'RS256' to the supported algorithms. 3rdparty/libprocess/src/jwt.cpp Line 30 (original), 28-31 (patched) <https://reviews.apache.org/r/66621/#comment282269> Please sort these usings. 3rdparty/libprocess/src/jwt.cpp Lines 262 (patched) <https://reviews.apache.org/r/66621/#comment282270> Break before `const string& token`, i.e. every function argument on a separate line. 3rdparty/libprocess/src/jwt.cpp Lines 265-269 (patched) <https://reviews.apache.org/r/66621/#comment282272> This shouldn't really happen, as we're making sure in `pemToRSA` that this isn't `nullptr`. Hence we should consider a violation of this fatal and do a ``` CHECK_NOTNULL(publicKey.get()); ``` instead. 3rdparty/libprocess/src/jwt.cpp Lines 312-314 (patched) <https://reviews.apache.org/r/66621/#comment282277> This should include a reason (e.g. the OpenSSL error) why the verification failed. See my comment on `verify_rsa_sha256` below on how this could be achieved by returning a `Try<Nothing>`. 3rdparty/libprocess/src/jwt.cpp Line 306 (original) <https://reviews.apache.org/r/66621/#comment282242> Keep this line. 3rdparty/libprocess/src/ssl/utilities.cpp Line 371 (original), 371 (patched) <https://reviews.apache.org/r/66621/#comment282244> Insert an empty line here, were using 2 lines between functions. Here and below. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 375-384 (patched) <https://reviews.apache.org/r/66621/#comment282283> Indent with 2 spaces. Here and in the functions below. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 386 (patched) <https://reviews.apache.org/r/66621/#comment282278> Insert a line. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 390 (patched) <https://reviews.apache.org/r/66621/#comment282279> Insert a line. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 394 (patched) <https://reviews.apache.org/r/66621/#comment282280> Insert a line. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 398-399 (patched) <https://reviews.apache.org/r/66621/#comment282291> Let's not use `malloc` here. A common pattern in Mesos would be ``` std::vector<unsigned char> signatureData; signatureData.reserve(RSA_size(privateKey.get()); ``` 3rdparty/libprocess/src/ssl/utilities.cpp Lines 416 (patched) <https://reviews.apache.org/r/66621/#comment282285> Let's include the OpenSSL error string here. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 421 (patched) <https://reviews.apache.org/r/66621/#comment282281> Insert a line. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 422 (patched) <https://reviews.apache.org/r/66621/#comment282276> Let's have this function return a `Try<Nothing>` and return an error if `RSA_verify != 1` and construct an error message including the actual OpenSSL error like it's done in `process/openssl.cpp` or above in `generate_hmac_sha256`. 3rdparty/libprocess/src/ssl/utilities.cpp Lines 442 (patched) <https://reviews.apache.org/r/66621/#comment282243> Remove this line. 3rdparty/libprocess/src/tests/jwt_keys.hpp Lines 21 (patched) <https://reviews.apache.org/r/66621/#comment282245> Please use `const char PRIVATE_KEY[]` instead of `const std::string PRIVATE_KEY` as this is the convention used in Mesos code. 3rdparty/libprocess/src/tests/jwt_keys.hpp Lines 51 (patched) <https://reviews.apache.org/r/66621/#comment282246> `const char PUBLIC_KEY[]` 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 26-27 (patched) <https://reviews.apache.org/r/66621/#comment282247> Because `jwt_tests.cpp` is the only unit using these keys, we could just define them here directly. Though it's nice to have this large block of text in a separate file, so this isn't an issue for me. 3rdparty/libprocess/src/tests/jwt_tests.cpp Line 29 (original), 31-35 (patched) <https://reviews.apache.org/r/66621/#comment282248> Please sort these usings. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 50 (patched) <https://reviews.apache.org/r/66621/#comment282249> Two lines between functions. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 52 (patched) <https://reviews.apache.org/r/66621/#comment282252> Indent with 4 spaces. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 61 (patched) <https://reviews.apache.org/r/66621/#comment282250> Dito. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 63 (patched) <https://reviews.apache.org/r/66621/#comment282253> Indent with 4 spaces. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 73 (patched) <https://reviews.apache.org/r/66621/#comment282251> Dito. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 78-79 (patched) <https://reviews.apache.org/r/66621/#comment282254> Let's remove the `Try`, because we don't expect these functions to fail here. ``` RSA_shared_ptr privateKey = CHECK_NOTNULL(pemToRSAPrivateKey(PRIVATE_KEY).get()); RSA_shared_ptr publicKey = CHECK_NOTNULL(pemToRSAPublicKey(PUBLIC_KEY).get()); ``` 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 81-82 (patched) <https://reviews.apache.org/r/66621/#comment282267> Let's just use two lambdas here like in `JWTTest.Create`. The generator functions plus the templated `create_token` function don't really reduce the overhead of having two almost similar lambdas. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 96 (patched) <https://reviews.apache.org/r/66621/#comment282255> Please insert a blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 120 (patched) <https://reviews.apache.org/r/66621/#comment282256> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 144 (patched) <https://reviews.apache.org/r/66621/#comment282257> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 168 (patched) <https://reviews.apache.org/r/66621/#comment282258> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 192 (patched) <https://reviews.apache.org/r/66621/#comment282259> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 218 (patched) <https://reviews.apache.org/r/66621/#comment282260> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 247 (patched) <https://reviews.apache.org/r/66621/#comment282261> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 254 (patched) <https://reviews.apache.org/r/66621/#comment282268> It would be great if you could use the signature of an actual private key (that doesn't match our public key) here. It's hard to distinguish from the `JWTError` but the idea of this test is to have a signature that can be parsed but doesn't match, not a signature that can't be parsed. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 297 (patched) <https://reviews.apache.org/r/66621/#comment282262> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 321 (patched) <https://reviews.apache.org/r/66621/#comment282263> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 347 (patched) <https://reviews.apache.org/r/66621/#comment282264> Blank line above. 3rdparty/libprocess/src/tests/jwt_tests.cpp Lines 505 (patched) <https://reviews.apache.org/r/66621/#comment282266> Please add a blank line above. - Jan Schlicht On April 15, 2018, 12:19 a.m., Clément Michaud wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66621/ > ----------------------------------------------------------- > > (Updated April 15, 2018, 12:19 a.m.) > > > Review request for mesos. > > > Bugs: https://issues.apache.org/jira/browse/MESOS-8788 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-8788 > > > Repository: mesos > > > Description > ------- > > Add alg RS256 support for JWT generator and validator. > > Currently, the JWT library only supports unsecured and HS256 tokens. I > implemented RS256 to use asymmetrical keys so that Mesos can use it at some > point. > > https://issues.apache.org/jira/browse/MESOS-8788 > > > 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/1/ > > > Testing > ------- > > I've 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). > > > Thanks, > > Clément Michaud > >
