mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.
Date Thu, 17 Mar 2016 19:40:17 GMT


> On March 14, 2016, 9:38 a.m., Adam B wrote:
> > src/authentication/http/basic_authenticator_factory.cpp, lines 63-64
> > <https://reviews.apache.org/r/44678/diff/3/?file=1296983#file1296983line63>
> >
> >     Seems like you're changing the meaning of the parameters in `BasicAuthenticatorFactory::create(Parameters)`.
Before, `Parameters` was just a convenient protobuf for username/password key-value pairs.
Now, you're interpreting the keys as module parameter names. Are you actually introducing
a new "authentication_realm" module parameter to the Authenticator module interface?
> >     Why not just use `BasicAuthenticatorFactory::create(const string realm, const
Parameters& parameters)`?
> 
> Greg Mann wrote:
>     One could certainly use the other overloads of `create` to specify a realm, but if
we don't provide a way to include the realm in the `Parameters`, then we still need a default
realm for this version of `create`, and I was trying to eliminate the default realm from this
file. AFAICT, there isn't a well-defined interface for the HTTP authenticator modules, with
respect to their input parameters. I didn't find mention of it in the [design doc](https://docs.google.com/document/d/1kM3_f7DSqXcE2MuERrLTGp_XMC6ss2wmpkNYDCY5rOM/edit#heading=h.bbxx6wwg3tpe),
or in related JIRAs. I was working under the assumption that the structure of the input parameters
for any given HTTP authN module is defined arbitrarily by the module's implementation, but
perhaps that's not the case?

Per our recent discussion, I'm going to drop this one. Since we need to provide a constructor
that accepts `Parameters` to comply with the authorizer module interface, this change seems
necessary.


- Greg


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


On March 17, 2016, 7:39 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44678/
> -----------------------------------------------------------
> 
> (Updated March 17, 2016, 7:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4850
>     https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Modified basic HTTP authenticator creator to accept realm.
> 
> To accommodate different authentication realms for the master and agent, the default
basic HTTP authenticator needs to accept its authentication realm as a parameter. This patch
adds this parameter and modifies the HTTP authentication tests to validate it appropriately.
A new test was also added: `HttpAuthenticationTest.BasicWithoutRealm`.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp c11bb47c8e02f2e8645cf387d18eb64d1c8cb604

>   src/authentication/http/basic_authenticator_factory.cpp 62f851685db3b42c52bbcb7cff3e4f4703004ed7

>   src/examples/test_http_authenticator_module.cpp 459b7046bd76d3043d2484a2dd30c10d7deaedd4

>   src/master/master.cpp e6290ea686ccf17813d6faeaf2f2012f79cf3b7f 
>   src/tests/http_authentication_tests.cpp cf2bb762272fa38e04e5c26aef2858300bbd0459 
> 
> Diff: https://reviews.apache.org/r/44678/diff/
> 
> 
> Testing
> -------
> 
> HTTP authentication tests were updated to pass the authentication realm to the basic
HTTP authenticator, and to adhere to the new credentials format in the module parameters.
A new test was also added: `HttpAuthenticationTest.BasicWithoutRealm`
> 
> `make check` was used to test on both OSX and CentOS 7.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message