Re: Review Request 67425: [WIP] Implement a JWK Set parser converting JWKs into RSA keys.

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