mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 66621: Add support for alg RS256 to JWT library.
Date Tue, 15 May 2018 14:30:29 GMT

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


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