You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2018/05/15 14:30:29 UTC
Re: Review Request 66621: Add support for alg RS256 to JWT library.
-----------------------------------------------------------
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
>
>
Re: Review Request 66621: Add support for alg RS256 to JWT library.
Posted by Clement Michaud <cl...@gmail.com>.
> 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
>
>
Re: Review Request 66621: Add support for alg RS256 to JWT library.
Posted by Clement Michaud <cl...@gmail.com>.
> 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.
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.
- Clement
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66621/#review202608
-----------------------------------------------------------
On avr. 21, 2018, 11:22 matin, Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
>
> (Updated avr. 21, 2018, 11:22 matin)
>
>
> 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
>
>