mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 56667: Added support for JSON Web Tokens.
Date Mon, 06 Mar 2017 23:33:38 GMT

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




3rdparty/libprocess/include/process/jwt.hpp
Lines 13-14 (patched)
<https://reviews.apache.org/r/56667/#comment240063>

    s/JWT_HPP/PROCESS_JWT_HPP/



3rdparty/libprocess/include/process/jwt.hpp
Lines 26-27 (patched)
<https://reviews.apache.org/r/56667/#comment240067>

    Could you provide a comment here explaining the motivation for adding the new error type?



3rdparty/libprocess/include/process/jwt.hpp
Lines 38-40 (patched)
<https://reviews.apache.org/r/56667/#comment240066>

    Two newlines here?



3rdparty/libprocess/include/process/jwt.hpp
Lines 42 (patched)
<https://reviews.apache.org/r/56667/#comment240077>

    Should we also provide a link to RFC-7515 here?



3rdparty/libprocess/include/process/jwt.hpp
Lines 124-125 (patched)
<https://reviews.apache.org/r/56667/#comment240095>

    Fits on one line



3rdparty/libprocess/include/process/jwt.hpp
Lines 134 (patched)
<https://reviews.apache.org/r/56667/#comment240064>

    s/JWT_HPP/PROCESS_JWT_HPP/



3rdparty/libprocess/src/jwt.cpp
Lines 16 (patched)
<https://reviews.apache.org/r/56667/#comment240085>

    I think this include should occur first?



3rdparty/libprocess/src/jwt.cpp
Lines 42-43 (patched)
<https://reviews.apache.org/r/56667/#comment240089>

    To make this text more consistent, perhaps should do "Expected 3 components in token"?



3rdparty/libprocess/src/jwt.cpp
Lines 169 (patched)
<https://reviews.apache.org/r/56667/#comment240116>

    Do you think `parse_component` would be a better name for this?



3rdparty/libprocess/src/jwt.cpp
Lines 201-203 (patched)
<https://reviews.apache.org/r/56667/#comment240124>

    It looks like this will silently ignore 'typ' headers which are not strings - should we
return an error in that case?



3rdparty/libprocess/src/jwt.cpp
Lines 221 (patched)
<https://reviews.apache.org/r/56667/#comment240125>

    To be consistent with the naming above, you could use the variable name `alg_string` here.



3rdparty/libprocess/src/jwt.cpp
Lines 288 (patched)
<https://reviews.apache.org/r/56667/#comment240133>

    Should have a `break;` after this line.



3rdparty/libprocess/src/jwt.cpp
Lines 303 (patched)
<https://reviews.apache.org/r/56667/#comment240137>

    Shouldn't we use `header.typ` here?



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 38 (patched)
<https://reviews.apache.org/r/56667/#comment240144>

    It might be more obvious what's going on, and more durable, if we use the `generate_hmac_sha256`
helper here and below to make the key:
    ```
    const string header = "NOT A VALID HEADER";
    const string payload =
      base64::encode_url_safe("{\"exp\":9999999999,\"sub\":\"foo\"}");
    const string signature =
      base64::encode_url_safe(generate_hmac_sha256(strings::join(".", header, payload)));
    
    const string token = strings::join(".", header, payload, signature);
    ```



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 249 (patched)
<https://reviews.apache.org/r/56667/#comment240150>

    Use `generate_hmac_hs256` here?


- Greg Mann


On March 6, 2017, 2:53 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> -----------------------------------------------------------
> 
> (Updated March 6, 2017, 2:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
>     https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56667/diff/6/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message