I'm having some problems with AccumuloToken.  Christopher has already
brought it up in IRC, and I agree.  I'm putting it out on the list for a
more inclusive discussion.

I don't like exposing thrift as the serialization mechanism.  In
particular, this just hurts my eyes:

public interface AccumuloToken
<T extends TBase<?,?>, F extends TFieldIdEnum> extends TBase<T, F>,
Destroyable {
...
}

Is there some reason this is not just:

public interface AccumuloToken extends Writable, Destroyable {
...
}

I've switched this in my local development environment and it seems to work
just fine.

I don't like the class name.  Is there some reason why Accumulo isn't just
assumed and we call this Token or Credential, or even SecurityToken?

I don't like the rest of the code being littered with deprecation warnings.
 If we're not willing to switch the code over to The New Way, why should we
expect our users?

Are there not some security implications of dynamic class loading for
authorization when the class name is specified *by the remote caller*?

And I know a punched the Proxy in at the last second, but is there
something we should do with security to avoid changes to this new API?

-Eric

inline-
On Mon, Jan 28, 2013 at 10:18 AM, Eric Newton <[EMAIL PROTECTED]> wrote:

> I'm having some problems with AccumuloToken.  Christopher has already
> brought it up in IRC, and I agree.  I'm putting it out on the list for a
> more inclusive discussion.
>
> I don't like exposing thrift as the serialization mechanism.  In
> particular, this just hurts my eyes:
>
> public interface AccumuloToken
> <T extends TBase<?,?>, F extends TFieldIdEnum> extends TBase<T, F>,
> Destroyable {
> ...
> }
>
> Is there some reason this is not just:
>
> public interface AccumuloToken extends Writable, Destroyable {
> ...
> }
>
> I've switched this in my local development environment and it seems to work
> just fine.
>
This works because the tokens we provide still implement TBase, and the
Thrift serdes still work. I forgot, but agree that we should try to keep
thrift out of the client api. I'm refactoring it now to keep cruft of the
client end.
>
> I don't like the class name.  Is there some reason why Accumulo isn't just
> assumed and we call this Token or Credential, or even SecurityToken?
>
>
I can rename it to SecurityToken as part of the refactoring.
> I don't like the rest of the code being littered with deprecation warnings.
>  If we're not willing to switch the code over to The New Way, why should we
> expect our users?
>
>
We need to keep the AuthInfo floating around for API compatibility. As much
as I would love to kill them with fire, this kills api compatibility
between versions.
> Are there not some security implications of dynamic class loading for
> authorization when the class name is specified *by the remote caller*?
>
>
The security class is not specified by the remote caller, it's instantiated
from the configuration xml files. All the client does is provide a token
and it's up to the implementation to accept/reject it. I simply provided a
mechanism for the client to get a hint as to what type of token is expected.
> And I know a punched the Proxy in at the last second, but is there
> something we should do with security to avoid changes to this new API?
>
>
Like with the CLI and shell, the proxy only supports UserPassTokens. This
should cover a large majority of use cases, but you never know. However,
the great part about extensibility is it allows individuals to do what they
need to do if it reaches outside the common cases. In the worse case, they
can wrap whatever token they need into UserPass to fit their needs, but in
an ideal world we should be able to work with these. However, with the last
minute of the proxy, I'm afraid I do not have the time or knowledge of the
proxy to get that support in for 1.5.
> -Eric
>

On Mon, Jan 28, 2013 at 12:13 PM, John Vines <[EMAIL PROTECTED]> wrote:
> inline-
>
>
> On Mon, Jan 28, 2013 at 10:18 AM, Eric Newton <[EMAIL PROTECTED]> wrote:
>
>> I'm having some problems with AccumuloToken.  Christopher has already
>> brought it up in IRC, and I agree.  I'm putting it out on the list for a
>> more inclusive discussion.
>>
>> I don't like exposing thrift as the serialization mechanism.  In
>> particular, this just hurts my eyes:
>>
>> public interface AccumuloToken
>> <T extends TBase<?,?>, F extends TFieldIdEnum> extends TBase<T, F>,
>> Destroyable {
>> ...
>> }
>>
>> Is there some reason this is not just:
>>
>> public interface AccumuloToken extends Writable, Destroyable {
>> ...
>> }
>>
>> I've switched this in my local development environment and it seems to work
>> just fine.
>>
>
>
> This works because the tokens we provide still implement TBase, and the
> Thrift serdes still work. I forgot, but agree that we should try to keep
> thrift out of the client api. I'm refactoring it now to keep cruft of the
> client end.
>
>
>>
>> I don't like the class name.  Is there some reason why Accumulo isn't just
>> assumed and we call this Token or Credential, or even SecurityToken?
>>
>>
> I can rename it to SecurityToken as part of the refactoring.
>
>
>> I don't like the rest of the code being littered with deprecation warnings.
>>  If we're not willing to switch the code over to The New Way, why should we
>> expect our users?
>>
>>
> We need to keep the AuthInfo floating around for API compatibility. As much
> as I would love to kill them with fire, this kills api compatibility
> between versions.
>
>
>> Are there not some security implications of dynamic class loading for
>> authorization when the class name is specified *by the remote caller*?
>>
>>
> The security class is not specified by the remote caller, it's instantiated
> from the configuration xml files. All the client does is provide a token
> and it's up to the implementation to accept/reject it. I simply provided a
> mechanism for the client to get a hint as to what type of token is expected.
>
>
>> And I know a punched the Proxy in at the last second, but is there
>> something we should do with security to avoid changes to this new API?
>>
>>
> Like with the CLI and shell, the proxy only supports UserPassTokens. This
> should cover a large majority of use cases, but you never know. However,
> the great part about extensibility is it allows individuals to do what they
> need to do if it reaches outside the common cases. In the worse case, they
> can wrap whatever token they need into UserPass to fit their needs, but in
> an ideal world we should be able to work with these. However, with the last
> minute of the proxy, I'm afraid I do not have the time or knowledge of the
> proxy to get that support in for 1.5.

I can take a look at this.  We are not going to be adding any new
features for 1.5.   But I think we should certainly intergrate the
exiting new features to form a cohesive whole.  I have one idea for
this. I will post it on the ticket.

>
>
>> -Eric
>>

Inline
On Mon, Jan 28, 2013 at 12:13 PM, John Vines <[EMAIL PROTECTED]> wrote:

> I forgot, but agree that we should try to keep
> thrift out of the client api. I'm refactoring it now to keep cruft of the
> client end.
>

Thanks.
> I can rename it to SecurityToken as part of the refactoring.
>

Great.
> We need to keep the AuthInfo floating around for API compatibility. As much
> as I would love to kill them with fire, this kills api compatibility
> between versions.
>

Feel free to sprinkle warning suppression annotations
>
> The security class is not specified by the remote caller, it's instantiated
> from the configuration xml files. All the client does is provide a token
> and it's up to the implementation to accept/reject it. I simply provided a
> mechanism for the client to get a hint as to what type of token is
> expected.
>

I beg to differ.  Please see TokenHelper.fromBytes appears to be using
deserialized information directly to load a class.
> However, with the last
> minute of the proxy, I'm afraid I do not have the time or knowledge of the
> proxy to get that support in for 1.5.
>

Keith had an idea.  I'll add a authenticate method that will return a
token.  The proxy methods will all take tokens.

-Eric

That line in the TokenHelper determines what class the Token should be
deserialized as. It has nothing to do with what Authenticator it gets
thrown against.
On Mon, Jan 28, 2013 at 2:17 PM, Eric Newton <[EMAIL PROTECTED]> wrote:

> Inline
>
>
> On Mon, Jan 28, 2013 at 12:13 PM, John Vines <[EMAIL PROTECTED]> wrote:
>
>> I forgot, but agree that we should try to keep
>> thrift out of the client api. I'm refactoring it now to keep cruft of the
>> client end.
>>
>
> Thanks.
>
>
>> I can rename it to SecurityToken as part of the refactoring.
>>
>
> Great.
>
>
>> We need to keep the AuthInfo floating around for API compatibility. As
>> much
>> as I would love to kill them with fire, this kills api compatibility
>> between versions.
>>
>
> Feel free to sprinkle warning suppression annotations
>
>
>>
>> The security class is not specified by the remote caller, it's
>> instantiated
>> from the configuration xml files. All the client does is provide a token
>> and it's up to the implementation to accept/reject it. I simply provided a
>> mechanism for the client to get a hint as to what type of token is
>> expected.
>>
>
> I beg to differ.  Please see TokenHelper.fromBytes appears to be using
> deserialized information directly to load a class.
>
>
>> However, with the last
>> minute of the proxy, I'm afraid I do not have the time or knowledge of the
>> proxy to get that support in for 1.5.
>>
>
> Keith had an idea.  I'll add a authenticate method that will return a
> token.  The proxy methods will all take tokens.
>
> -Eric
>

Valid point, did not consider that. Another option, which I feel like an
idiot for not thinking of previously (and would also remedy the proxy
issue), is to switch to JSON (preferred), XML, protobuff, etc. blobs. Then
it switches from the user knowing what token class to invoke to what fields
need to be provided.
On Mon, Jan 28, 2013 at 2:48 PM, Eric Newton <[EMAIL PROTECTED]> wrote:

> Right, but it would allow a malicious user to expand into a class with a
> malicious readFields, or deserialize method.
>
> -Eric
>
>
>
> On Mon, Jan 28, 2013 at 2:35 PM, John Vines <[EMAIL PROTECTED]> wrote:
>
>> That line in the TokenHelper determines what class the Token should be
>> deserialized as. It has nothing to do with what Authenticator it gets
>> thrown against.
>>
>>
>> On Mon, Jan 28, 2013 at 2:17 PM, Eric Newton <[EMAIL PROTECTED]>wrote:
>>
>>> Inline
>>>
>>>
>>> On Mon, Jan 28, 2013 at 12:13 PM, John Vines <[EMAIL PROTECTED]> wrote:
>>>
>>>> I forgot, but agree that we should try to keep
>>>> thrift out of the client api. I'm refactoring it now to keep cruft of
>>>> the
>>>> client end.
>>>>
>>>
>>> Thanks.
>>>
>>>
>>>> I can rename it to SecurityToken as part of the refactoring.
>>>>
>>>
>>> Great.
>>>
>>>
>>>> We need to keep the AuthInfo floating around for API compatibility. As
>>>> much
>>>> as I would love to kill them with fire, this kills api compatibility
>>>> between versions.
>>>>
>>>
>>> Feel free to sprinkle warning suppression annotations
>>>
>>>
>>>>
>>>> The security class is not specified by the remote caller, it's
>>>> instantiated
>>>> from the configuration xml files. All the client does is provide a token
>>>> and it's up to the implementation to accept/reject it. I simply
>>>> provided a
>>>> mechanism for the client to get a hint as to what type of token is
>>>> expected.
>>>>
>>>
>>> I beg to differ.  Please see TokenHelper.fromBytes appears to be using
>>> deserialized information directly to load a class.
>>>
>>>
>>>> However, with the last
>>>> minute of the proxy, I'm afraid I do not have the time or knowledge of
>>>> the
>>>> proxy to get that support in for 1.5.
>>>>
>>>
>>> Keith had an idea.  I'll add a authenticate method that will return a
>>> token.  The proxy methods will all take tokens.
>>>
>>> -Eric
>>>
>>
>>
>

I'm holding off, waiting for John's initial refactoring, before I provide a
lot of additional feedback, but I think Eric summed up much of the
conversation John and I had on IRC pretty well. I think the only place we
need to be concerned about AuthInfo is where it shows up in the public API.
But actually, internally, this should probably not be deprecated at all. It
should just be changed to be the holder of the binary serialization of
authentication tokens. I think there should only be one... we can add a
type parameter to AuthInfo to distinguish between different types if
necessary, as that is the most extensible. The token type does have to be
checked on the server side, though, to ensure that the server is configured
to accept that type of token.

Additionally, there's also the fact that making authentication pluggable,
means that some of our securityOperations no longer make sense. For
instance, the concept of a managed user within our API no longer makes
sense, because 1) not all authentication systems have a "create user" and
"drop user" concept, 2) not all those that do have "create user" and "drop
user" can do so with an authentication token, as an authentication token is
*not* the same thing as a user's credentials and should not be represented
as such in the API. So, I think these operations should be moved from the
current location (with deprecation), to authentication provider
implementations (login services?) that use them.

Below is how I would kind of like to see it work from an API perspective:

AuthToken rootUser passwordAuthenticationService.getToken(rootUserPrincipal, rootUserPassword);
passwordAuthenticationService.createUser(rootUser, "myUser", "myPassword");

AuthToken token = passwordAuthenticationService.getToken(myUserPrincipal,
password); // aka "log in"
//OR
AuthToken token = kerberosAuthenticationService.getToken(userPrincipal,
???);
//OR
AuthToken token = mockAuthenticationService.getToken(); // returns a token
for a mock user principal
... internal serialization magic to convert AuthToken to thrift form
AuthInfo ...
... rpc call, then deserialize back to AuthToken on server side ...
serverConfiguration.getAuthenticationService().authenticate(token); //
throws exception if token's type doesn't match the authentication service
or if user's token is expired or invalid
String userName = token.getPrincipal().getName();
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii
On Mon, Jan 28, 2013 at 2:17 PM, Eric Newton <[EMAIL PROTECTED]> wrote:

> Inline
>
>
> On Mon, Jan 28, 2013 at 12:13 PM, John Vines <[EMAIL PROTECTED]> wrote:
>
> > I forgot, but agree that we should try to keep
> > thrift out of the client api. I'm refactoring it now to keep cruft of the
> > client end.
> >
>
> Thanks.
>
>
> > I can rename it to SecurityToken as part of the refactoring.
> >
>
> Great.
>
>
> > We need to keep the AuthInfo floating around for API compatibility. As
> much
> > as I would love to kill them with fire, this kills api compatibility
> > between versions.
> >
>
> Feel free to sprinkle warning suppression annotations
>
>
> >
> > The security class is not specified by the remote caller, it's
> instantiated
> > from the configuration xml files. All the client does is provide a token
> > and it's up to the implementation to accept/reject it. I simply provided
> a
> > mechanism for the client to get a hint as to what type of token is
> > expected.
> >
>
> I beg to differ.  Please see TokenHelper.fromBytes appears to be using
> deserialized information directly to load a class.
>
>
> > However, with the last
> > minute of the proxy, I'm afraid I do not have the time or knowledge of
> the
> > proxy to get that support in for 1.5.
> >
>
> Keith had an idea.  I'll add a authenticate method that will return a
> token.  The proxy methods will all take tokens.
>
> -Eric
>