You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/03/30 11:53:57 UTC
[GitHub] [couchdb] rnewson opened a new pull request #2732: Enhance JWT
controls (again)
rnewson opened a new pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732
## Overview
This PR;
* fixes the broken required_claims property (due to my misunderstanding of proplists:get_keys).
* Removes the enhancement to the `alg` check as it is superceded by the binding of algorithm type and key value in jwtf_keystore.
* Changes the default required_claims to include nbf and exp. Administrators can remove either or both restriction if they choose.
## Testing recommendations
All changes covered by unit and/or integration tests in this PR.
## Related Issues or Pull Requests
N/A
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [x] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r401103820
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
An administrator that wishes to use JWT and replication would have to disable it, though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jaydoane commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400391815
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
I found this [answer on SO](https://stackoverflow.com/questions/43291659/usage-of-nbf-in-json-web-tokens) which talks about some possible use of `nbf`. How are we justifying this requirement?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jaydoane commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400386957
##########
File path: src/jwtf/src/jwtf.erl
##########
@@ -123,15 +123,33 @@ validate(Header0, Payload0, Signature, Checks, KS) ->
Key = key(Header1, Checks, KS),
verify(Alg, Header0, Payload0, Signature, Key).
+
validate_checks(Checks) when is_list(Checks) ->
- UnknownChecks = proplists:get_keys(Checks) -- ?CHECKS,
+ case {lists:usort(Checks), lists:sort(Checks)} of
+ {L, L} ->
+ ok;
+ {L1, L2} ->
+ error({duplicate_checks, L2 -- L1})
+ end,
+ {_, UnknownChecks} = lists:partition(fun valid_check/1, Checks),
case UnknownChecks of
[] ->
ok;
UnknownChecks ->
error({unknown_checks, UnknownChecks})
end.
+
+valid_check(Check) when is_atom(Check) ->
+ lists:member(Check, ?CHECKS);
+
+valid_check({Check, _}) when is_atom(Check) ->
Review comment:
How does a check get the tuple form? Would it be worth having typespecs here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] atrauzzi commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
atrauzzi commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r401097307
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
Secure by default would be to require an `exp` claim in my opinion. :smile:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jaydoane commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r401143585
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
> ah, you know, I think I will advocate for not including either -- because of replication. To use JWT with replication, you can set the "headers" directly and include your token, but this is no use if it expires. So, the token issuer can issue short-lived tokens for users (setting exp and/or nbf as they see fit) and we'll enforce them just because they're present. That same token issuer can issue a token without exp for use in replication tasks, though the only way to then revoke them is to change the secrets the server trusts...
I suspect someone using token based replication is also going to know how to edit default.ini, so would still vote +1 to include `exp`. FWIW, I think the replication token should still expire, even if it's set years in advance. An eternal token seems no better than a password.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400888908
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
we're discussing the defaults, @atrauzzi. Admins can change these settings. "Easy to get started" is my goal as long as it comports with "secure by default". What is your concrete suggestion for the required_claims default?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] atrauzzi commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
atrauzzi commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r401107600
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
Ah, right!
Well, let's make sure the docs include something about changing the secret as a way to mitigate compromised tokens then. Just so that people have some evidence that this scenario was considered.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r401110520
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
right. and of course you can use different secrets for the different key uses, so you could rotate your "replication" secret independently.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson merged pull request #2732: Enhance JWT controls
(again)
Posted by GitBox <gi...@apache.org>.
rnewson merged pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r401103820
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
An administrator that wishes to use JWT and replication would have to remove it, though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400405582
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
it's akin to the "Not Valid Before" and "Not Valid After" fields that all TLS certificates comes with, and recognises that tracking the "true" time is difficult, and that being able to influence clocks could lead to a security issue.
We check for expiration under the reasonable expectation that clocks typically move forward. If we don't check the nbf then there's an infinite window of time where the token is valid. If we check bnf then it's a specific timeframe, which is harder to target.
An alternative approach here that I considered, and would like your views on;
1) We validate every field present in the token, regardless of server setting. So, if the token includes `nbf`, `exp` or both, we'll validate them, rejecting them if they don't hold with respect to the servers clock.
2) The required_claims is then _only_ about whether we require those claims or not.
3) The default list would then be empty. Not insisting on exp or nbf but also not accepting a token with an nbf in the future or an exp in the past as valid.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] jaydoane commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
jaydoane commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400527009
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
I would prefer that we require a minimum of `exp` via default.ini, so you'd have to explicitly remove it if you wanted an arguably less secure system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400560514
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
that was my original feeling for `exp,nbf` so I will add another commit tomorrow. Noting that before this PR the default of `exp` was ignored anyway.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400864117
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
ah, you know, I think I will advocate for not including either -- because of replication. To use JWT with replication, you can set the "headers" directly and include your token, but this is no use if it expires. So, the token issuer can issue short-lived tokens for users (setting exp and/or nbf as they see fit) and we'll enforce them just because they're present. That same token issuer can issue a token without exp for use in replication tasks, though the only way to then revoke them is to change the secrets the server trusts...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] atrauzzi commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
atrauzzi commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400878996
##########
File path: rel/overlay/etc/default.ini
##########
@@ -142,7 +142,7 @@ max_db_number_for_dbs_info_req = 100
;[jwt_auth]
; List of claims to validate
-; required_claims = exp
+; required_claims = exp,nbf
Review comment:
I think enforcing anything outside of the spec could surprise or get in the way of some legitimate design that somebody intends for.
My thought is to have the bare minimum out of a zero-configuration setup. As in, if I run CouchDB and only enable JWTs, they should work. This is more in the spirit of being easy to get started with and being able to dial in desired configuration incrementally.
That all said, I am open to some degree of caution, without forcing people to implement things that aren't applicable to their system (`nbf` being one potential example of such).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] rnewson commented on a change in pull request #2732:
Enhance JWT controls (again)
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2732: Enhance JWT controls (again)
URL: https://github.com/apache/couchdb/pull/2732#discussion_r400401402
##########
File path: src/jwtf/src/jwtf.erl
##########
@@ -123,15 +123,33 @@ validate(Header0, Payload0, Signature, Checks, KS) ->
Key = key(Header1, Checks, KS),
verify(Alg, Header0, Payload0, Signature, Key).
+
validate_checks(Checks) when is_list(Checks) ->
- UnknownChecks = proplists:get_keys(Checks) -- ?CHECKS,
+ case {lists:usort(Checks), lists:sort(Checks)} of
+ {L, L} ->
+ ok;
+ {L1, L2} ->
+ error({duplicate_checks, L2 -- L1})
+ end,
+ {_, UnknownChecks} = lists:partition(fun valid_check/1, Checks),
case UnknownChecks of
[] ->
ok;
UnknownChecks ->
error({unknown_checks, UnknownChecks})
end.
+
+valid_check(Check) when is_atom(Check) ->
+ lists:member(Check, ?CHECKS);
+
+valid_check({Check, _}) when is_atom(Check) ->
Review comment:
for example, the `iss` check has a value, you have to say `{iss, <<"name of issuer">>}` in the `Check` param, and then jwtf will reject tokens with no `iss` property or with one where the value doesn't match.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services