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 09:35:21 UTC
Re: Review Request 56667: Added support for JSON Web Tokens.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/
-----------------------------------------------------------
(Updated Feb. 16, 2017, 10:35 a.m.)
Review request for mesos and Greg Mann.
Summary (updated)
-----------------
Added support for JSON Web Tokens.
Bugs: MESOS-7001
https://issues.apache.org/jira/browse/MESOS-7001
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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/
Testing (updated)
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/#review165823
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56665, 56666, 56667]
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. 16, 2017, 9:35 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2017, 9:35 a.m.)
>
>
> Review request for mesos 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/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > Regarding the description: I'm curious how exactly the current implementation isn't compliant with RFCs 7515/7519? The one thing I noticed was the lack of support for the 'crit' header parameter.
>
> Jan Schlicht wrote:
> There isn't support for `alg=none` and it is strongly recommended to also support `alg=RS256`. Standard claims aren't validated, though it's up to the specific applications to specify which of these claims are mandatory; it would make sense to validate them as part of a general-purpose JWT implementation. Decoded JSON isn't tested for line breaks, whitespaces, correct UTF-8 encoding.
Sorry, I've read the RFC wrong: We don't have to test the JSON for line breaks, but the base64. I'll add support for `alg=none` and the `crit` header.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/#review166057
-----------------------------------------------------------
On Feb. 22, 2017, 3:26 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2017, 3:26 p.m.)
>
>
> Review request for mesos 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/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > Regarding the description: I'm curious how exactly the current implementation isn't compliant with RFCs 7515/7519? The one thing I noticed was the lack of support for the 'crit' header parameter.
There isn't support for `alg=none` and it is strongly recommended to also support `alg=RS256`. Standard claims aren't validated, though it's up to the specific applications to specify which of these claims are mandatory; it would make sense to validate them as part of a general-purpose JWT implementation. Decoded JSON isn't tested for line breaks, whitespaces, correct UTF-8 encoding.
> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/jwt.hpp, lines 44-48
> > <https://reviews.apache.org/r/56667/diff/2/?file=1637101#file1637101line44>
> >
> > Do we want to include support for the 'crit' header parameter for spec compliance?
If JWT parsing is kept internal only, e.g. we only parse JWTs created by the secret generator, it's IMHO okay to leave out 'crit' parameter handling, because we wouldn't have it in any header.
> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/jwt.hpp, line 77
> > <https://reviews.apache.org/r/56667/diff/2/?file=1637101#file1637101line77>
> >
> > Do you think it's worth using a `JSON::Object` for the header? This would let the module accommodate arbitrary header keys (AKA 'Private Header Parameter Names' from RFC-7515), which could be useful for users who want to use the module for other purposes?
Same as above: It depends on what the scope of this implementation should be. An internal-only JWT implementation doesn't need a `JSON::Object` for the header, because we control what will be in the header. Of course, if this module should be more general-purpose, this would need to be changed, along with some other changes. But then we could also strive for full RFC compliance, which would mean (among others) support for `alg=none`, probably `alg=RS256` and other subtleties.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/#review166057
-----------------------------------------------------------
On Feb. 16, 2017, 10:35 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2017, 10:35 a.m.)
>
>
> Review request for mesos 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/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Feb. 20, 2017, 7:36 a.m., Greg Mann wrote:
> > Regarding the description: I'm curious how exactly the current implementation isn't compliant with RFCs 7515/7519? The one thing I noticed was the lack of support for the 'crit' header parameter.
>
> Jan Schlicht wrote:
> There isn't support for `alg=none` and it is strongly recommended to also support `alg=RS256`. Standard claims aren't validated, though it's up to the specific applications to specify which of these claims are mandatory; it would make sense to validate them as part of a general-purpose JWT implementation. Decoded JSON isn't tested for line breaks, whitespaces, correct UTF-8 encoding.
>
> Jan Schlicht wrote:
> Sorry, I've read the RFC wrong: We don't have to test the JSON for line breaks, but the base64. I'll add support for `alg=none` and the `crit` header.
Thx Jan!! If we run into any spec-compliance issues which will take a bunch of time to implement, we can always just make a note in comments/documentation of how we differ from the RFCs, and create tickets to update in the future.
> On Feb. 20, 2017, 7:36 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/jwt.hpp, line 77
> > <https://reviews.apache.org/r/56667/diff/2/?file=1637101#file1637101line77>
> >
> > Do you think it's worth using a `JSON::Object` for the header? This would let the module accommodate arbitrary header keys (AKA 'Private Header Parameter Names' from RFC-7515), which could be useful for users who want to use the module for other purposes?
>
> Jan Schlicht wrote:
> Same as above: It depends on what the scope of this implementation should be. An internal-only JWT implementation doesn't need a `JSON::Object` for the header, because we control what will be in the header. Of course, if this module should be more general-purpose, this would need to be changed, along with some other changes. But then we could also strive for full RFC compliance, which would mean (among others) support for `alg=none`, probably `alg=RS256` and other subtleties.
Looks like this isn't a strict requirement of the spec, so feel free to ignore for now :)
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/#review166057
-----------------------------------------------------------
On Feb. 22, 2017, 2:26 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2017, 2:26 p.m.)
>
>
> Review request for mesos 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/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/#review166057
-----------------------------------------------------------
Regarding the description: I'm curious how exactly the current implementation isn't compliant with RFCs 7515/7519? The one thing I noticed was the lack of support for the 'crit' header parameter.
3rdparty/libprocess/include/process/jwt.hpp (lines 44 - 48)
<https://reviews.apache.org/r/56667/#comment237961>
Do we want to include support for the 'crit' header parameter for spec compliance?
3rdparty/libprocess/include/process/jwt.hpp (line 77)
<https://reviews.apache.org/r/56667/#comment237959>
Do you think it's worth using a `JSON::Object` for the header? This would let the module accommodate arbitrary header keys (AKA 'Private Header Parameter Names' from RFC-7515), which could be useful for users who want to use the module for other purposes?
3rdparty/libprocess/src/jwt.cpp (lines 74 - 77)
<https://reviews.apache.org/r/56667/#comment237962>
Just one newline here?
- Greg Mann
On Feb. 16, 2017, 9:35 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2017, 9:35 a.m.)
>
>
> Review request for mesos 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/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/
-----------------------------------------------------------
(Updated Feb. 23, 2017, 3:22 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Made implementation RFC compliant (added 'crit' header parameter handling, 'none' algorithm, commented on standard claim validation)
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 (updated)
-----
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/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 56667: Added support for JSON Web Tokens.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56667/
-----------------------------------------------------------
(Updated Feb. 22, 2017, 3:26 p.m.)
Review request for mesos and Greg Mann.
Changes
-------
Addressed issues.
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 (updated)
-----
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/
Testing
-------
make check
Thanks,
Jan Schlicht