mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Clement Michaud <clement.michau...@gmail.com>
Subject Re: Review Request 66621: Add support for alg RS256 to JWT library.
Date Tue, 15 May 2018 20:52:10 GMT


> On mai 15, 2018, 2:30 après-midi, Till Toenshoff wrote:
> > 3rdparty/libprocess/src/ssl/utilities.cpp
> > Lines 433 (patched)
> > <https://reviews.apache.org/r/66621/diff/4/?file=2008446#file2008446line433>
> >
> >     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.
> 
> Clement Michaud wrote:
>     ERR_error_string gives more info about the error compared to ERR_reason_error_string
which is a subset, so I propose to keep ERR_error_string. Moreover both options already exist
in the code.
>     
>     I handled the null pointer with ternary as suggested though.

Eventually I used ERR_reason_error_string because there was an example using the ternary in
the same file so I just did the exact same thing.


- Clement


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66621/#review202608
-----------------------------------------------------------


On mai 15, 2018, 8:49 après-midi, Clement Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated mai 15, 2018, 8:49 après-midi)
> 
> 
> 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/5/
> 
> 
> 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