mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Schlicht <...@mesosphere.io>
Subject Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.
Date Mon, 16 Apr 2018 10:02:44 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message