> On Feb. 24, 2017, 2:07 a.m., Vinod Kone wrote: > > src/slave/http.cpp, line 831 > > <https://reviews.apache.org/r/56812/diff/2/?file=1641768#file1641768line831> > > > > so previously we were sending an empty subject (`Subject()`) but now we > > are sending `Option::None()` to `getObjectApprover`? what's the difference? > > i wish this change was done in a separate review instead of mixing it here > > with the use of auth context.
Yes, the existing code is buggy. If authorization is enabled, but authentication is disabled (so that `principal` is `NONE`), then we end up default-constructing the authorization `Subject` instead of using `None()`. This creates a `Subject` with an empty string for its value. This means that if authorization is enabled, authentication is disabled, and an ACL has been set allowing a user called "" to do things, then requests which do not specify any Authorization header will be granted access incorrectly. Clearly, this is an extreme corner case, but a bug nonetheless. I didn't notice the bug until the reviews were already in flight, so I applied the change in these patches. If you like, I can create a patch before this one which applies the changes to make use of `None()` correctly, and then rebase onto that. > On Feb. 24, 2017, 2:07 a.m., Vinod Kone wrote: > > src/slave/http.cpp, line 836 > > <https://reviews.apache.org/r/56812/diff/2/?file=1641768#file1641768line836> > > > > not related to your change, but I'm curious why this method uses the > > object approver whereas the `Slave::Http::flags()` doesn't? cc @arojas I'm guessing it's just for historical reasons; the old handler probably was not updated when the newer one was added? - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56812/#review166636 ----------------------------------------------------------- On Feb. 22, 2017, 1:18 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56812/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2017, 1:18 a.m.) > > > Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till > Toenshoff, and Vinod Kone. > > > Bugs: MESOS-7003 > https://issues.apache.org/jira/browse/MESOS-7003 > > > Repository: mesos > > > Description > ------- > > This patch updates the HTTP endpoint handlers in the > agent process to accept an `AuthenticationContext` > instead of an `Option<string>& principal`. > > > Diffs > ----- > > src/slave/http.cpp 8a9fabf861369d3ae659dce21fa3932f6f7b9161 > src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 > src/slave/slave.cpp 45905297836017e9031359894fc71e614c13cfcc > > Diff: https://reviews.apache.org/r/56812/diff/ > > > Testing > ------- > > Testing details can be found at the end of this review chain. > > > Thanks, > > Greg Mann > >
