You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2015/12/07 15:18:39 UTC

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

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

(Updated Dec. 7, 2015, 3:18 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.


Bugs: MESOS-3232
    https://issues.apache.org/jira/browse/MESOS-3232


Repository: mesos


Description
-------

See summary.


Diffs (updated)
-----

  3rdparty/libprocess/Makefile.am 6ec6d7989df647f466ac6079738835ffcb2ea8ee 
  3rdparty/libprocess/include/process/authenticator.hpp 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
  3rdparty/libprocess/src/CMakeLists.txt 681f0cfec57e152568da41698c8bdd52c05f65a6 
  3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb 

Diff: https://reviews.apache.org/r/38094/diff/


Testing
-------

make check


Thanks,

Alexander Rojas

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

Posted by Alexander Rukletsov <ru...@gmail.com>.

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



3rdparty/libprocess/src/tests/http_tests.cpp (line 1408)
<https://reviews.apache.org/r/38094/#comment168699>

    Why do you use `http::Connection.send()` in the previous RR and `http::get()` here?


- Alexander Rukletsov


On Dec. 7, 2015, 3:11 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38094/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2015, 3:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3232
>     https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 6ec6d7989df647f466ac6079738835ffcb2ea8ee 
>   3rdparty/libprocess/include/process/authenticator.hpp 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
>   3rdparty/libprocess/src/CMakeLists.txt 681f0cfec57e152568da41698c8bdd52c05f65a6 
>   3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38094/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

Posted by Ben Mahler <be...@gmail.com>.

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


Thanks for the test, main question is why BasicAuthenticator is not a Process? Was that an oversight, or were you intending for it to be pure-const after construction and therefore thread-safe?


3rdparty/libprocess/include/process/authenticator.hpp (lines 82 - 97)
<https://reviews.apache.org/r/38094/#comment169840>

    This is going to be accessed concurrently, so shouldn't this be a Process? Especially since you've marked 'authenticate' as non-const? (I think it could have been const)



3rdparty/libprocess/src/authenticator.cpp (lines 47 - 49)
<https://reviews.apache.org/r/38094/#comment169842>

    Renaming 'result' to 'unauthorized' will look a bit clearer in the returns below. What about Forbidden here? If the credentials are bad, why do we re-issue the same challenge?



3rdparty/libprocess/src/authenticator.cpp (lines 57 - 59)
<https://reviews.apache.org/r/38094/#comment169843>

    Isn't the empty case caught below when you're parsing with the split?



3rdparty/libprocess/src/authenticator.cpp (lines 79 - 82)
<https://reviews.apache.org/r/38094/#comment169841>

    Rather than re-using the object by setting unauthorized to none here, let's just use a different object here to keep it simple.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1387 - 1389)
<https://reviews.apache.org/r/38094/#comment169849>

    How about:
    
    // Tests the "Basic" authenticator.
    
    Not sure we need to say the rest?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1390)
<https://reviews.apache.org/r/38094/#comment169848>

    Why repeat Authentication here? s/AuthenticationBasic/Basic/ will do



3rdparty/libprocess/src/tests/http_tests.cpp (line 1394)
<https://reviews.apache.org/r/38094/#comment169847>

    How about: s/testuser/user/
    
    Why did you need the 'principal' variable here? Looks like you don't use it, also we can just hard code "user" and "password" to keep the test really simple.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1395)
<https://reviews.apache.org/r/38094/#comment169844>

    Why the temporary Owned variable here?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1397)
<https://reviews.apache.org/r/38094/#comment169845>

    Can you rebase? I did a s/TEST_AUTHENTICATION_REALM/"realm"/ in your previous patch



3rdparty/libprocess/src/tests/http_tests.cpp (line 1398)
<https://reviews.apache.org/r/38094/#comment169846>

    How about: s/testpassword/password/



3rdparty/libprocess/src/tests/http_tests.cpp (line 1416)
<https://reviews.apache.org/r/38094/#comment169850>

    What's a "pass"? ;)


- Ben Mahler


On Dec. 11, 2015, 2:01 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38094/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2015, 2:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3232
>     https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 82778bbd34cfcf54b4268e019d634eb64e506d55 
>   3rdparty/libprocess/include/process/authenticator.hpp 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
>   3rdparty/libprocess/src/CMakeLists.txt 681f0cfec57e152568da41698c8bdd52c05f65a6 
>   3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38094/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

Posted by Ben Mahler <be...@gmail.com>.

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


The tests look great! Just a minor comment about Forbidden (sorry I sent you down that path). Should be good to commit after another iteration.


3rdparty/libprocess/src/authenticator.cpp (lines 53 - 54)
<https://reviews.apache.org/r/38094/#comment172593>

    Are these const?



3rdparty/libprocess/src/authenticator.cpp (lines 58 - 84)
<https://reviews.apache.org/r/38094/#comment172592>

    These should go at the bottom, under the implementation of BasicAuthenticatorProcess::authenticate.



3rdparty/libprocess/src/authenticator.cpp (line 104)
<https://reviews.apache.org/r/38094/#comment172599>

    Could we avoid the .get() here and just store the option?
    
    ```
    Option<string> authorization = request.headers.get("Authorization");
    
    if (authorization.isNone()) {
      return unauthorized;
    }
    
    vector<string> components = strings::split(authorization.get(), " ");
    ```



3rdparty/libprocess/src/authenticator.cpp (lines 121 - 123)
<https://reviews.apache.org/r/38094/#comment172596>

    Sorry, looking at this again, looks like this should also be 401 with a challenge?
    
    https://tools.ietf.org/html/rfc7235#section-3.1
    
    ```
       If the request included authentication credentials, then the 401
       response indicates that authorization has been refused for those
       credentials.
    ```
    
    Looks like 403 is the real "unauthorized", in the sense that if the request was authenticated but the user is not authorized to access to the resource, then 403 should be returned: https://tools.ietf.org/html/rfc7231#section-6.5.3
    
    So, `AuthenticationResult.forbidden` should be set only when no-one is allowed and therefore we'll never issue a challenge? Perhaps we should clarify this in the documentation of AuthenticationResult?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1389)
<https://reviews.apache.org/r/38094/#comment172601>

    One more newline here



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1390 - 1446)
<https://reviews.apache.org/r/38094/#comment172602>

    Very nice!


- Ben Mahler


On Dec. 14, 2015, 12:53 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38094/
> -----------------------------------------------------------
> 
> (Updated Dec. 14, 2015, 12:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3232
>     https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 82778bbd34cfcf54b4268e019d634eb64e506d55 
>   3rdparty/libprocess/include/process/authenticator.hpp 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
>   3rdparty/libprocess/src/CMakeLists.txt 681f0cfec57e152568da41698c8bdd52c05f65a6 
>   3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 9fe27039416290296e43c6327c85721342d02cb9 
> 
> Diff: https://reviews.apache.org/r/38094/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>