----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56753/#review168065 -----------------------------------------------------------
Almost there! Thanks Jan, code is looking great. 3rdparty/libprocess/include/process/authenticator.hpp Lines 165-166 (original), 165-168 (patched) <https://reviews.apache.org/r/56753/#comment240165> Should there be two newlines here? 3rdparty/libprocess/include/process/authenticator.hpp Lines 175 (patched) <https://reviews.apache.org/r/56753/#comment240154> s/RS256/HS256/ 3rdparty/libprocess/include/process/authenticator.hpp Lines 176 (patched) <https://reviews.apache.org/r/56753/#comment240155> s/successfull/successful/ 3rdparty/libprocess/include/process/authenticator.hpp Lines 177 (patched) <https://reviews.apache.org/r/56753/#comment240156> Instead of "the claims of 'principal' will be set to the claims represented by the tokens", what do you think about: "the claims of the returned principal will be set to the claims within the token's payload" 3rdparty/libprocess/include/process/authenticator.hpp Lines 184 (patched) <https://reviews.apache.org/r/56753/#comment240153> Can use `override` instead of `virtual` here. 3rdparty/libprocess/src/jwt_authenticator.cpp Lines 43 (patched) <https://reviews.apache.org/r/56753/#comment240163> Eliminate `http` namespace here? 3rdparty/libprocess/src/jwt_authenticator.cpp Lines 72 (patched) <https://reviews.apache.org/r/56753/#comment240161> Ah, my apologies for asking you to remove the `token.size() != 2` check; it's still possible that the size is _less_ than 2, so we should indeed check that to avoid a possible error when doing `token[1]`. 3rdparty/libprocess/src/tests/http_tests.cpp Lines 2033 (patched) <https://reviews.apache.org/r/56753/#comment240166> Should we do something similar here to what you're doing below, and stringify an invalid JWT (invalid 'exp' or something else) to generate this? I quite liked the "forged token" test we encountered recently (where a correct token was altered to have a slightly different payload, i.e. a different 'sub'), perhaps you could use that case here, or add another case for it? - Greg Mann On March 6, 2017, 2:55 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56753/ > ----------------------------------------------------------- > > (Updated March 6, 2017, 2:55 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 > ------- > > 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 'Principal'. > > > 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/4/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
