You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2017/02/16 14:07:03 UTC
Review Request 56753: Implemented the JWT authenticator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56753/
-----------------------------------------------------------
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
Re: Review Request 56753: Implemented the JWT authenticator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56753/#review166370
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56623, 56617, 56618, 56901, 56619, 56812, 56813, 56624, 56621, 56665, 56666, 56667, 56753]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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
>
>
Re: Review Request 56753: Implemented the JWT authenticator.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> 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
>
>
Re: Review Request 56753: Implemented the JWT authenticator.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 56753: Implemented the JWT authenticator.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Feb. 25, 2017, 3:26 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, lines 81-102
> > <https://reviews.apache.org/r/56753/diff/2/?file=1642463#file1642463line81>
> >
> > I was just discussing this claim with Vinod; let's leave it out of our implementation. We'd like to move toward a world where the new `AuthenticationContext` is first-class throughout the authN/authZ code, so that, for example, a scheduler could subscribe using a credential that resolves to an `AuthenticationContext` which contains claims, but no principal. In such a world, we would simply use the "sub" field as-is within the claims, with no need to translate it into the `principal` member.
Agreed. Though this has some consequences for the `SecretGenerator`, because it shouldn't map a `Principal::value` to a claim. Not sure yet, but if called with a `Principal` that has its `value` set, it should fail. But probably even better would be to change `SecretGenerator::generate` to accept a `map` of claims instead of `Principal`.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56753/#review166801
-----------------------------------------------------------
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
>
>
Re: Review Request 56753: Implemented the JWT authenticator.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56753/#review166801
-----------------------------------------------------------
3rdparty/libprocess/src/jwt_authenticator.cpp (lines 81 - 102)
<https://reviews.apache.org/r/56753/#comment238878>
I was just discussing this claim with Vinod; let's leave it out of our implementation. We'd like to move toward a world where the new `AuthenticationContext` is first-class throughout the authN/authZ code, so that, for example, a scheduler could subscribe using a credential that resolves to an `AuthenticationContext` which contains claims, but no principal. In such a world, we would simply use the "sub" field as-is within the claims, with no need to translate it into the `principal` member.
- 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
>
>
Re: Review Request 56753: Implemented the JWT authenticator.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56753/
-----------------------------------------------------------
(Updated Feb. 22, 2017, 3:28 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Rebased.
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 (updated)
-----
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