accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher <ctubb...@apache.org>
Subject Re: BatchWriter Auth/Audit Question
Date Fri, 19 Aug 2016 19:42:11 GMT
If you note the comment in the parent class, the implementation of
canAskAboutUser is relying on the authentication being done in the call to
canPerformSystemActions. If you reverse the order here, you lose that
authentication check, and the action will be allowed simply if the user is
equal to itself. That's no good. We need an authentication check, and we
can't rely on the client code to check it for us.

We could reverse the order, but with an explicit authentication check when
the user is asking about itself, but then there's likely going to be a
duplicate authentication check if that fails, because
canPerformSystemActions will also need to do an authentication check
(because it can also be called directly).

We can probably refactor this a bit, so we have an unauthenticated
"canPerformSystemActions" check which is shared by the authenticated
version and the reverse-ordered canAskAboutUser implementation, but we need
to be careful about reversing the order here and forgetting to do an
authentication check, or unnecessarily doubling-up on the authentication
check (though, maybe the double-check isn't that bad in the less common
case... depends on impl performance).

On Fri, Aug 19, 2016 at 2:33 PM Marc P. <marc.parisi@gmail.com> wrote:

> Sorry....by out of band, It could be done once per writer or even in the
> connector once...but I think we're in agreement Drob, Mike.
>
> On Fri, Aug 19, 2016 at 2:30 PM, Marc P. <marc.parisi@gmail.com> wrote:
>
> > Funny, exactly the same thing I mentioned to Josh last night. Are you
> > watching us?
> >
> >  In something I"m doing now I'm making the thrift calls and modeled it
> > after what the client code does; however,
> > once I removed the authenticateUser my throughput increased by 25 per
> > cent. In my trace table it was by and large the greatest audited call.
> >
> > I think we could change how that works and potentially keep that out of
> > band. The fail fast is great if the user doesn't have permissions to that
> > table and may save the server some work...but as it is now, I'm always
> > authenticated and I'm causing more work for the server. Perhaps we let
> the
> > natural exception that's thrown stop the client ( if it's being a good
> > client ). I think the 25 per cent improvement was well worth the
> > alternative risk.
> >
> > On Fri, Aug 19, 2016 at 2:16 PM, Mike Drob <mdrob@apache.org> wrote:
> >
> >> Devs,
> >>
> >> I was recently running a 1.7.2 cluster under a heavy write workload and
> >> noticed a _lot_ of audit messages showing up in the logs. After some
> >> digging, I found that one of the causes is the following call chain:
> >>
> >> TabletServerBatchWriter::sendMutationsToTabletServer
> >> calls TabletServer.ThriftClientHandler::startUpdate
> >> calls SecurityOperation::authenticateUser <-- always return true
> >> calls SecurityOperation::canAskAboutUser <-- always return true
> >> calls AuditedSecurityOperation::canPerformSystemActions <-- fails; logs
> >> "action: performSystemAction"
> >> calls AuditedSecurityAction::authenticate <-- succeeds; logs "action:
> >> authenticate;"
> >>
> >> There are two separate but related problems here, I think, that I want
> to
> >> confirm.
> >>
> >> When startUpdate calls security.authenticateUser(credentials,
> >> credentials);
> >> there is the comment that this is done to "// Make sure user is real"
> >> presumably to fail fast. This makes sense, but does it actually work? In
> >> authenticateUser and canAskAboutUser we end up comparing the two
> >> credentials to each other and this will always succeed because they're
> the
> >> same object. I expect the write fails later in the TabletServer code,
> >> possibly in the UpdateSession, but I haven't been able to untangle this
> >> part yet.
> >>
> >> Second, I think it makes sense to switch the order of the OR in
> >> 'canAskAboutUser.' If this is in the write path, then it seems like the
> >> most common case would be users asking about themselves, so we should
> >> check
> >> that before checking if a user is an admin. This also has the advantage
> of
> >> removing two uninformative audit log lines. We won't have an audit
> message
> >> that somebody wrote data, but we don't have that currently anyway.
> >>
> >> I did some visual spot checking and this looks like an issue in master
> >> also, but I haven't run that code to find out.
> >>
> >> Thoughts?
> >>
> >> Mike
> >>
> >
> >
>

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