----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56753/#review166195 -----------------------------------------------------------
3rdparty/libprocess/include/process/authenticator.hpp (lines 110 - 112) <https://reviews.apache.org/r/56753/#comment238071> Could you elaborate a bit here on the implementation? i.e., the type of signature supported, RFC compliance or lack thereof, etc. Also, it might be more accurate to say something like "Implements the "Bearer" authentication scheme using JSON web tokens. Token signatures are validated using the specified secret key." 3rdparty/libprocess/src/jwt_authenticator.cpp (line 58) <https://reviews.apache.org/r/56753/#comment238133> Do we need the `http::` namespace here? It isn't used for `JWTAuthenticator::authenticate` below. 3rdparty/libprocess/src/jwt_authenticator.cpp (line 61) <https://reviews.apache.org/r/56753/#comment238076> AFAIK we should return a WWW-Authenticate header containing 'Bearer realm="REALM"' here, since we're expecting the 'Bearer' scheme in client requests. 3rdparty/libprocess/src/jwt_authenticator.cpp (lines 69 - 73) <https://reviews.apache.org/r/56753/#comment238077> Since you explicitly limit the number of tokens to 2 here, I don't think we need the `token.size() != 2` check. 3rdparty/libprocess/src/jwt_authenticator.cpp (lines 77 - 89) <https://reviews.apache.org/r/56753/#comment238081> In these error cases, we could provide additional error information in the WWW-Authenticate header. See https://tools.ietf.org/html/rfc6750#section-3 We can use "error=invalid_token, error_description=ERROR_MESSAGE", with appropriate messages for each case. Perhaps it makes sense to construct and return the Unauthorized response within each conditional so that we can set the header appropriately at construction? 3rdparty/libprocess/src/jwt_authenticator.cpp (lines 81 - 89) <https://reviews.apache.org/r/56753/#comment238084> Is there a reason to do this in two stages, as opposed to doing `jwt->payload.find<JSON::String>("sub")`? 3rdparty/libprocess/src/jwt_authenticator.cpp (line 97) <https://reviews.apache.org/r/56753/#comment238085> Is it possible to use `JSON::String` here directly? 3rdparty/libprocess/src/jwt_authenticator.cpp (lines 99 - 101) <https://reviews.apache.org/r/56753/#comment238089> Are you sure this won't end up silently ignoring fields which have numerical values? If the field's value is a JSON number, we should probably convert it to a string and place it in the map. Similarly with other JSON types; could we stringify a JSON object and place it into the map as well? 3rdparty/libprocess/src/jwt_authenticator.cpp (lines 104 - 107) <https://reviews.apache.org/r/56753/#comment238176> FYI I updated the `AuthenticationContext` patches to use a `map` instead of `Option<map>`, and I added a constructor `AuthenticationContext(Option<string>, map<string, string>)`, so you could make use of it here. In that case we would duplicate the principal in the "sub" claim, but perhaps that's OK? 3rdparty/libprocess/src/jwt_authenticator.cpp (line 138) <https://reviews.apache.org/r/56753/#comment238131> I think this should return "Bearer" - Greg Mann On Feb. 22, 2017, 2:28 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56753/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2017, 2:28 p.m.) > > > Review request for mesos and Greg Mann. > > > Bugs: MESOS-7001 > https://issues.apache.org/jira/browse/MESOS-7001 > > > Repository: mesos > > > Description > ------- > > This HTTP authenticator extracts a JWT from the requests' authorization > header using the 'Bearer' schema and validates it against a secret using > HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all > other claims will be additional labels of the 'AuthenticationContext'. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c > 3rdparty/libprocess/include/process/authenticator.hpp > e5489c6cb4adc8a822e7dd4515542618c36136f9 > 3rdparty/libprocess/src/authenticator.cpp > cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 > 3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/http_tests.cpp > fb4da9aecff0370d97a15269c5d8fffb30e0478f > > Diff: https://reviews.apache.org/r/56753/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
