You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Clément Michaud <cl...@gmail.com> on 2018/04/14 22:15:57 UTC

Review Request 66621: Add alg RS256 support for JWT generator and validator.

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

Review request for mesos.


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

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

Posted by Jan Schlicht <ja...@mesosphere.io>.

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

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621

Relevant logs:

- [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 447, in <module>
    main()
  File ".\support\apply-reviews.py", line 442, in main
    reviewboard(options)
  File ".\support\apply-reviews.py", line 432, in reviewboard
    apply_review(options)
  File ".\support\apply-reviews.py", line 163, in apply_review
    commit_patch(options)
  File ".\support\apply-reviews.py", line 292, in commit_patch
    shell(cmd, options['dry_run'])
  File ".\support\apply-reviews.py", line 147, in shell
    error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "C:\Program Files\Python27\lib\subprocess.py", line 610, in _execute_child
    args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 23: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On April 14, 2018, 10:19 p.m., Clément Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated April 14, 2018, 10:19 p.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
> 
>

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

Posted by Clément Michaud <cl...@gmail.com>.


> On avr. 16, 2018, 9:25 matin, Alexander Rojas wrote:
> > I haven't review it very thoroughly yet, but I found something that needs to be fixed. SSL support is an optional configurable feature (`--with-ssl` in the configure options). Since your patch depends on SSL, you need the guards to enable the feature just when SSL is enabled.

Hello Alexander, thanks for the review.
Actually, the code is compiled only when the flag is set to true. Check Makefile.am, `src/jwt.cpp` and `src/ssl/utilities.cpp` are in `if ENABLE_SSL` block.


- Clément


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


On avr. 14, 2018, 10:19 après-midi, Clément Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated avr. 14, 2018, 10:19 après-midi)
> 
> 
> 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
> 
>

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

Posted by Alexander Rojas <al...@mesosphere.io>.

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



I haven't review it very thoroughly yet, but I found something that needs to be fixed. SSL support is an optional configurable feature (`--with-ssl` in the configure options). Since your patch depends on SSL, you need the guards to enable the feature just when SSL is enabled.

- Alexander Rojas


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

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.

- Mesos Reviewbot


On April 15, 2018, 3:49 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, 3:49 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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [66621]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 16, 2018, 9 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, 9 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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Clément Michaud <cl...@gmail.com>.


> On avr. 16, 2018, 9:52 après-midi, Mesos Reviewbot Windows wrote:
> > FAIL: Failed to apply the current review.
> > 
> > Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`
> > 
> > All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621
> > 
> > Relevant logs:
> > 
> > - [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):
> > 
> > ```
> > Traceback (most recent call last):
> >   File ".\support\apply-reviews.py", line 447, in <module>
> >     main()
> >   File ".\support\apply-reviews.py", line 442, in main
> >     reviewboard(options)
> >   File ".\support\apply-reviews.py", line 432, in reviewboard
> >     apply_review(options)
> >   File ".\support\apply-reviews.py", line 163, in apply_review
> >     commit_patch(options)
> >   File ".\support\apply-reviews.py", line 292, in commit_patch
> >     shell(cmd, options['dry_run'])
> >   File ".\support\apply-reviews.py", line 147, in shell
> >     error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
> >     return Popen(*popenargs, **kwargs).wait()
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
> >     errread, errwrite)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 610, in _execute_child
> >     args = '{} /c "{}"'.format (comspec, args)
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 23: ordinal not in range(128)
> > ```
> 
> Clément Michaud wrote:
>     Can anyone help me find what is wrong with the bot?The script is running well on my laptop.

Can it be the accent in my first name?


- Clément


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


On avr. 16, 2018, 9 après-midi, Clément Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated avr. 16, 2018, 9 après-midi)
> 
> 
> 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 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'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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Clément Michaud <cl...@gmail.com>.


> On avr. 16, 2018, 9:52 après-midi, Mesos Reviewbot Windows wrote:
> > FAIL: Failed to apply the current review.
> > 
> > Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`
> > 
> > All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621
> > 
> > Relevant logs:
> > 
> > - [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):
> > 
> > ```
> > Traceback (most recent call last):
> >   File ".\support\apply-reviews.py", line 447, in <module>
> >     main()
> >   File ".\support\apply-reviews.py", line 442, in main
> >     reviewboard(options)
> >   File ".\support\apply-reviews.py", line 432, in reviewboard
> >     apply_review(options)
> >   File ".\support\apply-reviews.py", line 163, in apply_review
> >     commit_patch(options)
> >   File ".\support\apply-reviews.py", line 292, in commit_patch
> >     shell(cmd, options['dry_run'])
> >   File ".\support\apply-reviews.py", line 147, in shell
> >     error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
> >     return Popen(*popenargs, **kwargs).wait()
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
> >     errread, errwrite)
> >   File "C:\Program Files\Python27\lib\subprocess.py", line 610, in _execute_child
> >     args = '{} /c "{}"'.format (comspec, args)
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 23: ordinal not in range(128)
> > ```

Can anyone help me find what is wrong with the bot?The script is running well on my laptop.


- Clément


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


On avr. 16, 2018, 9 après-midi, Clément Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated avr. 16, 2018, 9 après-midi)
> 
> 
> 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 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
> -------
> 
> 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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621

Relevant logs:

- [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 447, in <module>
    main()
  File ".\support\apply-reviews.py", line 442, in main
    reviewboard(options)
  File ".\support\apply-reviews.py", line 432, in reviewboard
    apply_review(options)
  File ".\support\apply-reviews.py", line 163, in apply_review
    commit_patch(options)
  File ".\support\apply-reviews.py", line 292, in commit_patch
    shell(cmd, options['dry_run'])
  File ".\support\apply-reviews.py", line 147, in shell
    error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "C:\Program Files\Python27\lib\subprocess.py", line 610, in _execute_child
    args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 23: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On April 16, 2018, 9 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, 9 p.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 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
> -------
> 
> 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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Alexander Rojas <al...@mesosphere.io>.

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

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



FAIL: Failed to apply the current review.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66621`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621

Relevant logs:

- [apply-review-66621-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621/logs/apply-review-66621-stderr.log):

```
Traceback (most recent call last):
  File ".\support\apply-reviews.py", line 447, in <module>
    main()
  File ".\support\apply-reviews.py", line 442, in main
    reviewboard(options)
  File ".\support\apply-reviews.py", line 432, in reviewboard
    apply_review(options)
  File ".\support\apply-reviews.py", line 163, in apply_review
    commit_patch(options)
  File ".\support\apply-reviews.py", line 292, in commit_patch
    shell(cmd, options['dry_run'])
  File ".\support\apply-reviews.py", line 147, in shell
    error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
  File "C:\Program Files\Python27\lib\subprocess.py", line 168, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files\Python27\lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "C:\Program Files\Python27\lib\subprocess.py", line 610, in _execute_child
    args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 23: ordinal not in range(128)
```

- Mesos Reviewbot Windows


On April 19, 2018, 4:10 p.m., Clément Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated April 19, 2018, 4:10 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/3/
> 
> 
> 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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [66621]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 19, 2018, 11:10 p.m., Clément Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated April 19, 2018, 11:10 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/3/
> 
> 
> 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
> 
>

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



PASS: Mesos patch 66621 was successfully built and tested.

Reviews applied: `['66621']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621

- Mesos Reviewbot Windows


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 Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [66621]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


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 Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



PASS: Mesos patch 66621 was successfully built and tested.

Reviews applied: `['66621']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66621

- Mesos Reviewbot Windows


On May 15, 2018, 9:05 p.m., Clement Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated May 15, 2018, 9:05 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/6/
> 
> 
> 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 Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [66621]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 15, 2018, 9:05 p.m., Clement Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated May 15, 2018, 9:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, 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/6/
> 
> 
> 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 Till Toenshoff <to...@me.com>.

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


Ship it!




Ship It!

- Till Toenshoff


On May 15, 2018, 9:05 p.m., Clement Michaud wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66621/
> -----------------------------------------------------------
> 
> (Updated May 15, 2018, 9:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, 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/6/
> 
> 
> 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>.

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

(Updated mai 15, 2018, 9:05 après-midi)


Review request for mesos, Alexander Rojas and Till Toenshoff.


Changes
-------

Remove useless empty line


Bugs: MESOS-8788
    https://issues.apache.org/jira/browse/MESOS-8788


Repository: mesos


Description
-------

Add support for alg RS256 to JWT library.


Diffs (updated)
-----

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

Changes: https://reviews.apache.org/r/66621/diff/5-6/


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

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


Changes
-------

Add period to comments, sort includes, fixes error message in openssl utilities


Bugs: MESOS-8788
    https://issues.apache.org/jira/browse/MESOS-8788


Repository: mesos


Description
-------

Add support for alg RS256 to JWT library.


Diffs (updated)
-----

  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/

Changes: https://reviews.apache.org/r/66621/diff/4-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
> 
>

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 Till Toenshoff <to...@me.com>.

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

-----------------------------------------------------------
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 (updated)
-----

  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/

Changes: https://reviews.apache.org/r/66621/diff/3-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 Clément Michaud <cl...@gmail.com>.

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

(Updated avr. 19, 2018, 11:10 après-midi)


Review request for mesos, Alexander Rojas and Till Toenshoff.


Changes
-------

Fixed all issues reported by Alexander


Bugs: MESOS-8788
    https://issues.apache.org/jira/browse/MESOS-8788


Repository: mesos


Description
-------

Add support for alg RS256 to JWT library.


Diffs (updated)
-----

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

Changes: https://reviews.apache.org/r/66621/diff/2-3/


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

Re: Review Request 66621: Add support for alg RS256 to JWT library.

Posted by Clément Michaud <cl...@gmail.com>.

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

(Updated avr. 16, 2018, 9 après-midi)


Review request for mesos.


Changes
-------

Fix style issues and add one test validating RS256 token with wrong public key


Summary (updated)
-----------------

Add support for alg RS256 to JWT library.


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 (updated)
-------

Add support for alg RS256 to JWT library.


Diffs (updated)
-----

  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/

Changes: https://reviews.apache.org/r/66621/diff/1-2/


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

Re: Review Request 66621: Add alg RS256 support for JWT generator and validator.

Posted by Clément Michaud <cl...@gmail.com>.

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

(Updated avr. 14, 2018, 10:19 après-midi)


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