-----------------------------------------------------------
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
>
>
|