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 2021/06/22 20:28:18 UTC
[GitHub] [couchdb] noahshaw11 opened a new pull request #3637: Remove case sensitivity for basic auth
noahshaw11 opened a new pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637
## Overview
Basic authentication in CouchDB currently expects `auth-scheme` in `Authorization` to be equal to "Basic" (case sensitive). However, according to [HTTP Authentication RFC2617](https://datatracker.ietf.org/doc/html/rfc2617#section-1.2), it should be case-insensitive.
## Testing recommendations
Tests have been modified to use different cases of "Basic" including "basic", "BAsIc", "Basic", and "BASIC". Furthermore, you can execute the following curl requests and see that you will receive an appropriate response each time.
```
/Users/ncshaw % curl http://localhost:15984/db1 -X PUT -H'Authorization: basic YWRtOnBhc3M='
{"ok":true}
/Users/ncshaw % curl http://localhost:15984/db2 -X PUT -H'Authorization: Basic YWRtOnBhc3M='
{"ok":true}
/Users/ncshaw % curl http://localhost:15984/db -X GET -H'Authorization: BASIC YWRtOnBhc3M='
{"cluster":{"n":0,"q":0,"r":0,"w":0},"compact_running":false,"data_size":0,"db_name":"db","disk_format_version":0,"disk_size":0,"instance_start_time":"0","purge_seq":0,"encryption":{"enabled":false,"key_manager":{}},"update_seq":"0000000a1f123cb400000000","uuid":"00974c2f618fc85a3baede507945551b","doc_del_count":0,"doc_count":102,"sizes":{"external":98233562,"views":0}}
/Users/ncshaw % curl http://localhost:15984/db -X GET -H'Authorization: bAsIc YWRtOnBhc3M='
{"cluster":{"n":0,"q":0,"r":0,"w":0},"compact_running":false,"data_size":0,"db_name":"db","disk_format_version":0,"disk_size":0,"instance_start_time":"0","purge_seq":0,"encryption":{"enabled":false,"key_manager":{}},"update_seq":"0000000a1f123cb400000000","uuid":"00974c2f618fc85a3baede507945551b","doc_del_count":0,"doc_count":102,"sizes":{"external":98233562,"views":0}}
```
## Related Issues or Pull Requests
The authorization scheme for HTTP Basic Authentication should not be case sensitive: https://github.com/apache/couchdb/issues/3367
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] 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
[GitHub] [couchdb] iilyak commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r659710282
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -99,27 +99,32 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try
- re:split(
- base64:decode(Base64Value),
- ":",
- [{return, list}, {parts, 2}]
- )
- of
- ["_", "_"] ->
- % special name and pass to be logged out
- nil;
- [User, Pass] ->
- {User, Pass};
- _ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
+ % undefined is sent during startup process
+ undefined ->
Review comment:
This `undefined` appear to me as a magic value. I think we can flip the order of clauses to make it more readable and be closer to functionality of original version (In original version we ignore any unexpected values).
```
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
Header when is_list(Header) ->
[Basic, Base64Value] = string:split(AuthorizationHeader, " "),
...
_ ->
nil
end.
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r656720816
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
Interesting term haha. I will make this change.
--
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
[GitHub] [couchdb] bessbd commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r656719490
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
This seems like a [piggyback](https://stackoverflow.com/a/333883) to me. Can you please add a test case that would fail without your production code change, but passes with it? That way when we're changing code parts related to this fix, we have one specific test case that shows us what exactly is about to break and it is clearly listed in the test report.
--
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
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r657191921
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -71,22 +71,39 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try re:split(base64:decode(Base64Value), ":",
- [{return, list}, {parts, 2}]) of
- ["_", "_"] ->
- % special name and pass to be logged out
+ % undefined is sent during startup process
+ undefined ->
nil;
- [User, Pass] ->
- {User, Pass};
_ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
- _ ->
- nil
+ try string:split(AuthorizationHeader, " ") of
+ [Basic, Base64Value] ->
+ BasicLower = string:casefold(Basic),
+ AuthorizationHeader1 = BasicLower ++ " " ++ Base64Value,
+ case AuthorizationHeader1 of
+ "basic " ++ Base64Value ->
Review comment:
Agreed, comparing on `"basic " ++ Base64Value` was not needed. See https://github.com/apache/couchdb/pull/3637/commits/a6c2028adf8f717b3a9ec97835388a456fb1ce03.
--
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
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r659992110
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -99,27 +99,32 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try
- re:split(
- base64:decode(Base64Value),
- ":",
- [{return, list}, {parts, 2}]
- )
- of
- ["_", "_"] ->
- % special name and pass to be logged out
- nil;
- [User, Pass] ->
- {User, Pass};
- _ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
+ % undefined is sent during startup process
+ undefined ->
Review comment:
Your approach makes sense. See https://github.com/apache/couchdb/pull/3637/commits/dd0b666e743f6466910b278000a865bb087da451.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r657188723
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
Added three new test cases to test lowercase, uppercase, and mixed case "Basic". See https://github.com/apache/couchdb/pull/3637/commits/a6c2028adf8f717b3a9ec97835388a456fb1ce03 and https://github.com/apache/couchdb/pull/3637/commits/33c38d2ae8f5dd77f38d29170217f0ea794d87e9.
--
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
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r657189410
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -71,22 +71,39 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try re:split(base64:decode(Base64Value), ":",
- [{return, list}, {parts, 2}]) of
- ["_", "_"] ->
- % special name and pass to be logged out
+ % undefined is sent during startup process
+ undefined ->
nil;
- [User, Pass] ->
- {User, Pass};
_ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
- _ ->
- nil
+ try string:split(AuthorizationHeader, " ") of
Review comment:
Understandable. Fixed in https://github.com/apache/couchdb/pull/3637/commits/a6c2028adf8f717b3a9ec97835388a456fb1ce03.
--
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
[GitHub] [couchdb] bessbd commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r658653322
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
Perfect, thank you!
--
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
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r659992110
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -99,27 +99,32 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try
- re:split(
- base64:decode(Base64Value),
- ":",
- [{return, list}, {parts, 2}]
- )
- of
- ["_", "_"] ->
- % special name and pass to be logged out
- nil;
- [User, Pass] ->
- {User, Pass};
- _ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
+ % undefined is sent during startup process
+ undefined ->
Review comment:
Your approach makes sense. See https://github.com/apache/couchdb/pull/3637/commits/dd0b666e743f6466910b278000a865bb087da451.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r656982528
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -71,22 +71,39 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try re:split(base64:decode(Base64Value), ":",
- [{return, list}, {parts, 2}]) of
- ["_", "_"] ->
- % special name and pass to be logged out
+ % undefined is sent during startup process
+ undefined ->
nil;
- [User, Pass] ->
- {User, Pass};
_ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
- _ ->
- nil
+ try string:split(AuthorizationHeader, " ") of
+ [Basic, Base64Value] ->
+ BasicLower = string:casefold(Basic),
+ AuthorizationHeader1 = BasicLower ++ " " ++ Base64Value,
+ case AuthorizationHeader1 of
+ "basic " ++ Base64Value ->
Review comment:
I think comparing `BasicLower` is simpler. Something like the following would do:
```
case string:casefold(Basic) of
"basic" ->
try re:split(base64:decode(Base64Value), ":",
[{return, list}, {parts, 2}]) of
...
```
--
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
[GitHub] [couchdb] iilyak commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r656980512
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -71,22 +71,39 @@ special_test_authentication_handler(Req) ->
basic_name_pw(Req) ->
AuthorizationHeader = header_value(Req, "Authorization"),
case AuthorizationHeader of
- "Basic " ++ Base64Value ->
- try re:split(base64:decode(Base64Value), ":",
- [{return, list}, {parts, 2}]) of
- ["_", "_"] ->
- % special name and pass to be logged out
+ % undefined is sent during startup process
+ undefined ->
nil;
- [User, Pass] ->
- {User, Pass};
_ ->
- nil
- catch
- error:function_clause ->
- throw({bad_request, "Authorization header has invalid base64 value"})
- end;
- _ ->
- nil
+ try string:split(AuthorizationHeader, " ") of
Review comment:
I think `string:split/2` can fail only if non string is passed into it. Therefore `try` is not needed 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
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r657188723
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
Added three new test cases to test lowercase and mixed case "Basic". See https://github.com/apache/couchdb/pull/3637/commits/a6c2028adf8f717b3a9ec97835388a456fb1ce03 and https://github.com/apache/couchdb/pull/3637/commits/33c38d2ae8f5dd77f38d29170217f0ea794d87e9.
--
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
[GitHub] [couchdb] iilyak merged pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
iilyak merged pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] noahshaw11 commented on a change in pull request #3637: Remove case sensitivity for basic auth
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #3637:
URL: https://github.com/apache/couchdb/pull/3637#discussion_r657188723
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
Added two new tests cases to test lowercase and mixed case "Basic". See https://github.com/apache/couchdb/pull/3637/commits/a6c2028adf8f717b3a9ec97835388a456fb1ce03.
##########
File path: test/elixir/test/security_validation_test.exs
##########
@@ -12,19 +12,19 @@ defmodule SecurityValidationTest do
@auth_headers %{
jerry: [
# jerry:mouse
- authorization: "Basic amVycnk6bW91c2U="
+ authorization: "basic amVycnk6bW91c2U="
],
tom: [
# tom:cat
- authorization: "Basic dG9tOmNhdA=="
+ authorization: "BAsIc dG9tOmNhdA=="
],
spike_cat: [
# spike:cat - which is wrong
authorization: "Basic c3Bpa2U6Y2F0"
],
spike: [
# spike:dog
- authorization: "Basic c3Bpa2U6ZG9n"
+ authorization: "BASIC c3Bpa2U6ZG9n"
Review comment:
Added two new test cases to test lowercase and mixed case "Basic". See https://github.com/apache/couchdb/pull/3637/commits/a6c2028adf8f717b3a9ec97835388a456fb1ce03.
--
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