mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Schlicht <...@mesosphere.io>
Subject Re: Review Request 56753: Implemented the JWT authenticator.
Date Tue, 28 Feb 2017 14:01:07 GMT


> On Feb. 22, 2017, 8:58 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, line 97
> > <https://reviews.apache.org/r/56753/diff/1/?file=1637194#file1637194line97>
> >
> >     Is it possible to use `JSON::String` here directly?

Not possible. Also needed to support multiple types, see the next issue. Hope it's okay that
I'm dropping this.


> On Feb. 22, 2017, 8:58 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, lines 99-101
> > <https://reviews.apache.org/r/56753/diff/1/?file=1637194#file1637194line99>
> >
> >     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?

Agreed, we shouldn't ignore any field. I've changed it to call `stringify` an any value. All
JSON types have implementations for this and `jsonify` themselves. Handling of `JSON::String`
is a bit different though, because `stringify` would return a quoted string. RFC 7519 states
that a claim can be any JSON object, IMO we're good by return either a string, a stringified
number or a JSON string of more complicated values.


> On Feb. 22, 2017, 8:58 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, lines 77-89
> > <https://reviews.apache.org/r/56753/diff/1/?file=1637194#file1637194line77>
> >
> >     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?

Thanks for the hint, wasn't aware of that. Added error information where it was appropriate.


- Jan


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


On Feb. 28, 2017, 2:36 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2017, 2:36 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 '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
> 
>


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