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