You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Clement Michaud <cl...@gmail.com> on 2018/06/03 09:55:23 UTC
Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/
-----------------------------------------------------------
Review request for mesos.
Repository: mesos
Description
-------
JWT implementation can handle multiple type of keys for signing and
validating JSON web tokens. IETF defined a JSON representation of those
keys in
https://tools.ietf.org/html/rfc7517. This review implements this RFC.
This commit adds a parser to convert a JSON into a JWK set containing
RSA keys compatible with the implementation of RS256 in the JWT library.
The parser only supports parsing of RSA keys for now but it can support
multiple types of keys such as elliptic curve keys and symmetric keys
as documented in the JWA RFC.
Diffs
-----
3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/67425/diff/1/
Testing
-------
make check
Thanks,
Clement Michaud
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/#review204229
-----------------------------------------------------------
PASS: Mesos patch 67425 was successfully built and tested.
Reviews applied: `['67425']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67425
- Mesos Reviewbot Windows
On June 3, 2018, 5:55 p.m., Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> -----------------------------------------------------------
>
> (Updated June 3, 2018, 5:55 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
>
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
>
> The parser only supports parsing of RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67425/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Clement Michaud
>
>
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On June 4, 2018, 4:04 p.m., Alexander Rojas wrote:
> > Before going into detail with this, last patchset we forgot to add certain files to the install target wich ended up breaking tarball generation. Mostly a failure of your reviewers (myself included). Since you are adding new files, could you please make sure it works with `make distcheck` and also make sure of updating the `cmake` build (that should be a rule of thumb whenever one modifies the `Makefile.am`).
>
> Clement Michaud wrote:
> Yes, sure. I did not know I had to run those commands and did not get any failure when not doing so. I will make sure to run them from now on. Sorry for that.
> It would be good to add it in the test workflow though, don't you think?
>
> Thanks.
> Clément.
>
> Alexander Rojas wrote:
> No worries, I myself have messed up with it… so no need to be sorry. All your reviewers and shepherd forgot about it. I would try to get that into the contributing.
Done, you can help reviewing [r/67476/](https://reviews.apache.org/r/67476/)
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/#review204254
-----------------------------------------------------------
On June 4, 2018, 3:27 p.m., Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:27 p.m.)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
>
>
> Repository: mesos
>
>
> Description
> -------
>
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
>
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
>
> The parser only supports parsing of RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67425/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Clement Michaud
>
>
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Clement Michaud <cl...@gmail.com>.
> On juin 4, 2018, 2:04 après-midi, Alexander Rojas wrote:
> > Before going into detail with this, last patchset we forgot to add certain files to the install target wich ended up breaking tarball generation. Mostly a failure of your reviewers (myself included). Since you are adding new files, could you please make sure it works with `make distcheck` and also make sure of updating the `cmake` build (that should be a rule of thumb whenever one modifies the `Makefile.am`).
Yes, sure. I did not know I had to run those commands and did not get any failure when not doing so. I will make sure to run them from now on. Sorry for that.
It would be good to add it in the test workflow though, don't you think?
Thanks.
Clément.
- Clement
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/#review204254
-----------------------------------------------------------
On juin 4, 2018, 1:27 après-midi, Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> -----------------------------------------------------------
>
> (Updated juin 4, 2018, 1:27 après-midi)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
>
>
> Repository: mesos
>
>
> Description
> -------
>
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
>
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
>
> The parser only supports parsing of RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67425/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Clement Michaud
>
>
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/#review204254
-----------------------------------------------------------
Before going into detail with this, last patchset we forgot to add certain files to the install target wich ended up breaking tarball generation. Mostly a failure of your reviewers (myself included). Since you are adding new files, could you please make sure it works with `make distcheck` and also make sure of updating the `cmake` build (that should be a rule of thumb whenever one modifies the `Makefile.am`).
- Alexander Rojas
On June 4, 2018, 3:27 p.m., Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:27 p.m.)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
>
>
> Repository: mesos
>
>
> Description
> -------
>
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
>
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
>
> The parser only supports parsing of RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67425/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Clement Michaud
>
>
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/#review205633
-----------------------------------------------------------
3rdparty/libprocess/include/process/jwk_rsa.hpp
Lines 33 (patched)
<https://reviews.apache.org/r/67425/#comment288496>
A while ago I was playing with a similar concept as this, and since both a `Signer` and a `Verifier` share many things, I came up with the following interface:
```c++
class Hasher
{
public:
/// Verifies that the signature matches the message based using
/// the secrets known to this hasher instance. If there is an
/// error during the process, an instance of Error should be
/// returned.
virtual Try<bool> verify(
const std::string& message, const std::string& signature) const = 0;
/// Computes the signature of the given message.
virtual Try<std::string> sign(const std::string& message) const = 0;
/// Returns the algorithm implemented by the hasher instance.
virtual JWT::Alg algorithm() const = 0;
};
```
It works pretty well for symmetric algorithms, and for public/private key I returned error on the equivalent side if it was initialized with only one key.
3rdparty/libprocess/include/process/jwk_rsa.hpp
Lines 50 (patched)
<https://reviews.apache.org/r/67425/#comment288495>
We don't use hungarian notation. If needed, rather follow google codestye (i.e. `privateKey_` instead of `m_privateKey`).
3rdparty/libprocess/include/process/jwk_rsa.hpp
Lines 75 (patched)
<https://reviews.apache.org/r/67425/#comment288497>
Any reason why this is `shared_ptr` and not `unique_ptr`?
3rdparty/libprocess/include/process/jwk_set.hpp
Lines 46 (patched)
<https://reviews.apache.org/r/67425/#comment288499>
if the constructor is only used through `parse()`, perhaps we should make it at least protected.
3rdparty/libprocess/include/process/jwk_set.hpp
Lines 72 (patched)
<https://reviews.apache.org/r/67425/#comment288500>
I think mesos prefers to use `getSigners()` for accessors.
3rdparty/libprocess/src/jwk_set.cpp
Lines 40 (patched)
<https://reviews.apache.org/r/67425/#comment288502>
I would break this as follows:
```c++
using BIGNUMDeleter = void(*)(BIGNUM*);
using BIGNUMPtr = std::unique_ptr<BIGNUM, BIGNUMDeleter>;
```
However, I didn't find any single instance of this kind of alias anywhere in the code base.
3rdparty/libprocess/src/jwk_set.cpp
Lines 48 (patched)
<https://reviews.apache.org/r/67425/#comment288498>
mixed `snake_style` and `camelStyle`. I think the project prefers `camelStyle` (except in stout)
3rdparty/libprocess/src/jwk_set.cpp
Lines 203 (patched)
<https://reviews.apache.org/r/67425/#comment288505>
prefer to use `dp != nullptr && dq != nullptr…`
3rdparty/libprocess/src/jwk_set.cpp
Lines 262 (patched)
<https://reviews.apache.org/r/67425/#comment288504>
we don't break on `else`
3rdparty/libprocess/src/jwk_set.cpp
Lines 272 (patched)
<https://reviews.apache.org/r/67425/#comment288503>
add a `// namespace {` at the end.
- Alexander Rojas
On June 4, 2018, 3:27 p.m., Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> -----------------------------------------------------------
>
> (Updated June 4, 2018, 3:27 p.m.)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Bugs: MESOS-8974
> https://issues.apache.org/jira/browse/MESOS-8974
>
>
> Repository: mesos
>
>
> Description
> -------
>
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
>
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
>
> The parser only supports parsing of RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67425/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Clement Michaud
>
>
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Clement Michaud <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/
-----------------------------------------------------------
(Updated juin 4, 2018, 1:27 après-midi)
Review request for mesos.
Bugs: MESOS-8974
https://issues.apache.org/jira/browse/MESOS-8974
Repository: mesos
Description
-------
JWT implementation can handle multiple type of keys for signing and
validating JSON web tokens. IETF defined a JSON representation of those
keys in
https://tools.ietf.org/html/rfc7517. This review implements this RFC.
This commit adds a parser to convert a JSON into a JWK set containing
RSA keys compatible with the implementation of RS256 in the JWT library.
The parser only supports parsing of RSA keys for now but it can support
multiple types of keys such as elliptic curve keys and symmetric keys
as documented in the JWA RFC.
Diffs
-----
3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/67425/diff/1/
Testing
-------
make check
Thanks,
Clement Michaud
Re: Review Request 67425: [WIP] Implement a JWK Set parser converting
JWKs into RSA keys.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67425/#review204228
-----------------------------------------------------------
Bad review!
Reviews applied: []
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot
On June 3, 2018, 5:55 p.m., Clement Michaud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67425/
> -----------------------------------------------------------
>
> (Updated June 3, 2018, 5:55 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> JWT implementation can handle multiple type of keys for signing and
> validating JSON web tokens. IETF defined a JSON representation of those
> keys in
> https://tools.ietf.org/html/rfc7517. This review implements this RFC.
>
> This commit adds a parser to convert a JSON into a JWK set containing
> RSA keys compatible with the implementation of RS256 in the JWT library.
>
> The parser only supports parsing of RSA keys for now but it can support
> multiple types of keys such as elliptic curve keys and symmetric keys
> as documented in the JWA RFC.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am d434001fbc49d337b6e29f6ac8c9c7475922a819
> 3rdparty/libprocess/include/process/jwk.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_rsa.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/jwk_set.hpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_rsa.cpp PRE-CREATION
> 3rdparty/libprocess/src/jwk_set.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/jwk_set_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67425/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Clement Michaud
>
>