mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rojas <alexan...@mesosphere.io>
Subject Re: Review Request 66621: Add support for alg RS256 to JWT library.
Date Thu, 19 Apr 2018 12:42:57 GMT

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


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