Kerberos implementation questions

[Emmanuel Lecharny-3]

Hi,

I have some questions regarding the kerberos implementation :

1) We have a TicketModifier class. Is it really usefull ?
2) We have a TicketFactory class, but we have to instanciate it in
order to create Tickets. Any rationnal for the factory's method not
being static ?
3) We are using the standard javax.security...KerberosPrincipal, I
just wonder if it wouldn't be a btter idea to have our own class, so
that the Ticket class will store a realm, instead of asking the SUN
class to return it for us (the idea is to stick as much as possible to
the RFC names and structure)

As i'm just dipping by finger toe in the cold kerberos water, those
questions may seems naive, but this will be the kind of questions a
maintainer may ask anyway ...

Thanks.


--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

[Enrique Rodriguez-2]

On 7/1/07, Emmanuel Lecharny <elecharny@...> wrote:
> Hi,
>
> I have some questions regarding the kerberos implementation :
>
> 1) We have a TicketModifier class. Is it really usefull ?

The Ticket has no attribute setters, so the intention is that you use
the modifier to create immutable Ticket's.  In earlier days of
ApacheDS we settled on the "modifier pattern" so we could have
immutable objects.  I still think we should have immutable objects,
but if you can think of a better way to create them, I would like to
hear your idea(s).  At a minimum some classes and methods should be
better hidden w.r.t. package private, etc.  Keep in mind these
modifiers are used *everywhere*, not just with Ticket, although the
practice is used inconsistently.

Also, the modifiers are really useful in the switch-statement-heavy
ASN.1 DER codecs currently in use.  In the move to your DER work, this
nicety would go away.

> 2) We have a TicketFactory class, but we have to instanciate it in
> order to create Tickets. Any rationnal for the factory's method not
> being static ?

No, they could be static.  When I started developing the Kerberos and
Change Password *clients* recently, I had some utility code for
bootstrapping the troubleshooting of the clients and I moved said
utility code into the new TicketFactory without much thought.  I
expect that pretty soon, once the clients stabilize, we can simply
delete TicketFactory.  It is not used server-side.

The problem I ran into that precipitated TicketFactory is that a
successful Change Password request requires a successful service
ticket request, which requires a successful TGT request.
TicketFactory contains largely boilerplate code for making one-off
tickets.  As such it is mostly a development aid.

> 3) We are using the standard javax.security...KerberosPrincipal, I
> just wonder if it wouldn't be a btter idea to have our own class, so
> that the Ticket class will store a realm, instead of asking the SUN
> class to return it for us (the idea is to stick as much as possible to
> the RFC names and structure)

I think that makes sense.  I think I mostly used the existing classes
in the interest of getting the server live and I haven't had the
chance to go back and clean it up.  I agree that the server-side
should be cleansed of Sun classes.  Right now it is a bit of a mix.

Keep in mind, however, that the Sun classes are more useful on the
client-side.  The issue I ran into is that on the client-side, users
of JAAS will need to work with javax.security...KerberosTicket.  For
example, if you want to execute a client-side SASL GSSAPI bind using
Sun's JNDI, your Subject must be populated with KerberosTicket's,
which are created with KerberosPrincipals.  This goes for user's of
JGSS, as well.  For this reason, I went with the javax.security
classes in the new client code.  The server-side code is a bit of a
mix, since I started using our own classes but later ran into some
issues requiring the JAAS classes.  One of the utility methods on
TicketFactory converts between the two.

Enrique

[Emmanuel Lecharny-3]

On 7/2/07, Enrique Rodriguez <enriquer9@...> wrote:
> On 7/1/07, Emmanuel Lecharny <elecharny@...> wrote:
> > Hi,
> >
> > I have some questions regarding the kerberos implementation :
> >
> > 1) We have a TicketModifier class. Is it really usefull ?
>
> The Ticket has no attribute setters, so the intention is that you use
> the modifier to create immutable Ticket's.

Do we need to create immutable Tickets ? We just produce Tickets in
the server, then send them to the client. What's the point to have
Immutable Tickets ? I may miss something ...

In earlier days of
> ApacheDS we settled on the "modifier pattern" so we could have
> immutable objects.  I still think we should have immutable objects,
> but if you can think of a better way to create them, I would like to
> hear your idea(s).  At a minimum some classes and methods should be
> better hidden w.r.t. package private, etc.  Keep in mind these
> modifiers are used *everywhere*, not just with Ticket, although the
> practice is used inconsistently.

The fact that this Modifier thing is used everywhere don't give me a
clue why we should use it. I agree that some classes might be hidden,
but we have to give a clear reason to do so.

There is obviously something we should take into account : we will
have a server side and a client side. It may be very important to
protect some elements on the client side, and not on the server side,
but we also have to explain the reasons behind those choices.


>
> Also, the modifiers are really useful in the switch-statement-heavy
> ASN.1 DER codecs currently in use.  In the move to your DER work, this
> nicety would go away.

May be... I don't really understand why it's important to have them in
the current codec (encoder or decoder ?), but my lack of knowledge of
the current code is mainly the reason I have those questions.

>
> > 2) We have a TicketFactory class, but we have to instanciate it in
> > order to create Tickets. Any rationnal for the factory's method not
> > being static ?
>
> No, they could be static.  When I started developing the Kerberos and
> Change Password *clients* recently, I had some utility code for
> bootstrapping the troubleshooting of the clients and I moved said
> utility code into the new TicketFactory without much thought.  I
> expect that pretty soon, once the clients stabilize, we can simply
> delete TicketFactory.  It is not used server-side.

Ok, just fine. May be we can discuss the way we should do it. I like
the Factory pattern, but if we only have one kind of object we can
generate, it's a little but to heavy...

>
> The problem I ran into that precipitated TicketFactory is that a
> successful Change Password request requires a successful service
> ticket request, which requires a successful TGT request.
> TicketFactory contains largely boilerplate code for making one-off
> tickets.  As such it is mostly a development aid.

Yes, I get your point. Again, I'm not at all against a TicketFactory,
and it can make perfect sense to keep it. I just don't have enough
element to make a point right now.

>
> > 3) We are using the standard javax.security...KerberosPrincipal, I
> > just wonder if it wouldn't be a btter idea to have our own class, so
> > that the Ticket class will store a realm, instead of asking the SUN
> > class to return it for us (the idea is to stick as much as possible to
> > the RFC names and structure)
>
> I think that makes sense.  I think I mostly used the existing classes
> in the interest of getting the server live and I haven't had the
> chance to go back and clean it up.  I agree that the server-side
> should be cleansed of Sun classes.  Right now it is a bit of a mix.

Ok, I gonna have a look at it. From the client side, we obviously must
work with Sun classes, but from server side, having our own classes
will help a lot (debug, logs, etc.). It can be done step by step, but
first we need to build integration tests to be sure that moving from
Sun to our own classes don't break everything.

This is what I find difficult atm : changing the code is risky,
because of the lack of tests.

<SNIP/>
--
Regards,
Cordialement,
Emmanuel Lécharny
www.iktek.com

[Enrique Rodriguez-2]

On 7/2/07, Emmanuel Lecharny <elecharny@...> wrote:

> On 7/2/07, Enrique Rodriguez <enriquer9@...> wrote:
> > On 7/1/07, Emmanuel Lecharny <elecharny@...> wrote:
> > > Hi,
> > >
> > > I have some questions regarding the kerberos implementation :
> > >
> > > 1) We have a TicketModifier class. Is it really usefull ?
> >
> > The Ticket has no attribute setters, so the intention is that you use
> > the modifier to create immutable Ticket's.
>
> Do we need to create immutable Tickets ? We just produce Tickets in
> the server, then send them to the client. What's the point to have
> Immutable Tickets ? I may miss something ...

I think it is good programming practice, both for security
implications and for the resulting API, even if it is internal to
ApacheDS on the server-side.  You can web search on "security
immutable" or here is a direct reference from Sun:

http://java.sun.com/security/seccodeguide.html#gcg6

> ...
> Ok, I gonna have a look at it. From the client side, we obviously must
> work with Sun classes, but from server side, having our own classes
> will help a lot (debug, logs, etc.). It can be done step by step, but
> first we need to build integration tests to be sure that moving from
> Sun to our own classes don't break everything.
>
> This is what I find difficult atm : changing the code is risky,
> because of the lack of tests.
> ...

I have integration tests I would like to add to server-unit.  But it
requires adding a dep for kerberos-clients to server-unit, since the
tests use the new client.  If this is acceptable, I will add the dep
and commit a new test class.

This dep will also set us up for a new SASL GSSAPI bind integration
test and some tests for Change Password, as well.

Enrique

[Alex Karasulu]

On 7/9/07, Enrique Rodriguez <enriquer9@...> wrote:

I have integration tests I would like to add to server-unit.  But it
requires adding a dep for kerberos-clients to server-unit, since the
tests use the new client.  If this is acceptable, I will add the dep
and commit a new test class.

This dep will also set us up for a new SASL GSSAPI bind integration
test and some tests for Change Password, as well.

The dep is just fine especially if you make the dep have test scope. 

Alex

[Enrique Rodriguez-2]

On 7/9/07, Alex Karasulu <akarasulu@...> wrote:
> ...
> The dep is just fine especially if you make the dep have test scope.

OK, one last question, can you review the following group/artifact
id's and make changes/suggestions before I deploy and add this to
server-unit:

    <dependency>
      <groupId>org.apache.directory.client.kerberos</groupId>
      <artifactId>apacheds-kerberos-client</artifactId>
      <version>${pom.version}</version>
    </dependency>

    <dependency>
      <groupId>org.apache.directory.client.kerberos</groupId>
      <artifactId>apacheds-password-client</artifactId>
      <version>${pom.version}</version>
    </dependency>

Enrique

[Alex Karasulu]

First of all this presumes the clients mirror the version of the server. I don't
think you want that right?  Also the client is an external (to apacheds) project
to ApacheDS so I would put the correct version of the client into the
dependencyManagement section.  Then you don't need to specify the version
of the artifact in the dependencies section for these clients.

Secondly you want <scope>test</scope> in there for these dependencies if in
fact you intend their scope to be test scope.

Alex

On 7/9/07, Enrique Rodriguez <enriquer9@...> wrote:

On 7/9/07, Alex Karasulu <akarasulu@...> wrote:
> ...
> The dep is just fine especially if you make the dep have test scope.

OK, one last question, can you review the following group/artifact
id's and make changes/suggestions before I deploy and add this to
server-unit:

    <dependency>
      <groupId>org.apache.directory.client.kerberos</groupId>
      <artifactId>apacheds-kerberos-client</artifactId>
      <version>${ pom.version}</version>
    </dependency>

    <dependency>
      <groupId>org.apache.directory.client.kerberos</groupId>
      <artifactId>apacheds-password-client</artifactId>
      <version>${pom.version}</version>
    </dependency>

Enrique

[Enrique Rodriguez-2]

On 7/9/07, Alex Karasulu <akarasulu@...> wrote:
> First of all this presumes the clients mirror the version of the server. I
> don't
> think you want that right?

Nope.  I just now looked at how shared-ldap is set up so I'll follow
the way that's done.  Unfortunately, the ldap clients are on Maven 1
so I couldn't use them as an example.

> Also the client is an external (to apacheds)
> project
> to ApacheDS so I would put the correct version of the client into the
> dependencyManagement section.  Then you don't need to specify the version
> of the artifact in the dependencies section for these clients.

OK, I take it you mean the dependencyManagement section of the
apacheds trunk's POM, not server-unit.

> Secondly you want <scope>test</scope> in there for these dependencies if in
> fact you intend their scope to be test scope.

OK, I see how the server-unit POM does this for ldapsdk.

Enrique

[Emmanuel Lecharny]

Guys,

I did a simple test on my laptop writing a kerberos-unity project,
mimicing the server-unit project. I think that this should be a good
approach, as you don't have anymore dependency to deal with.

wdyt ?

Alex Karasulu a écrit :

> First of all this presumes the clients mirror the version of the
> server. I
> don't
> think you want that right?  Also the client is an external (to apacheds)
> project
> to ApacheDS so I would put the correct version of the client into the
> dependencyManagement section.  Then you don't need to specify the version
> of the artifact in the dependencies section for these clients.
>
> Secondly you want <scope>test</scope> in there for these dependencies
> if in
> fact you intend their scope to be test scope.
>
> Alex
>
> On 7/9/07, Enrique Rodriguez <enriquer9@...> wrote:
>
>>
>> On 7/9/07, Alex Karasulu <akarasulu@...> wrote:
>> > ...
>> > The dep is just fine especially if you make the dep have test scope.
>>
>> OK, one last question, can you review the following group/artifact
>> id's and make changes/suggestions before I deploy and add this to
>> server-unit:
>>
>>     <dependency>
>>       <groupId>org.apache.directory.client.kerberos</groupId>
>>       <artifactId>apacheds-kerberos-client</artifactId>
>>       <version>${pom.version}</version>
>>     </dependency>
>>
>>     <dependency>
>>       <groupId>org.apache.directory.client.kerberos</groupId>
>>       <artifactId>apacheds-password-client</artifactId>
>>       <version>${pom.version}</version>
>>     </dependency>
>>
>> Enrique
>>
>

[Enrique Rodriguez-2]

On 7/9/07, Emmanuel Lecharny <elecharny@...> wrote:
> Guys,
>
> I did a simple test on my laptop writing a kerberos-unity project,
> mimicing the server-unit project. I think that this should be a good
> approach, as you don't have anymore dependency to deal with.
>
> wdyt ?

What would you do in the case of SASL GSSAPI bind tests, which use
both Kerberos and LDAP?  I am in favor of a single integration test
module.

Similarly, integration tests for Change Password would use Change
Password and Kerberos.

kerberos-unit would make sense if you were only testing the Kerberos
protocol, but then why not just put the tests in protocol-kerberos.

Enrique

[Emmanuel Lecharny]

Enrique Rodriguez a écrit :

> On 7/9/07, Emmanuel Lecharny <elecharny@...> wrote:
>
>> Guys,
>>
>> I did a simple test on my laptop writing a kerberos-unity project,
>> mimicing the server-unit project. I think that this should be a good
>> approach, as you don't have anymore dependency to deal with.
>>
>> wdyt ?
>
>
> What would you do in the case of SASL GSSAPI bind tests, which use
> both Kerberos and LDAP?  I am in favor of a single integration test
> module.
>
> Similarly, integration tests for Change Password would use Change
> Password and Kerberos.
>
> kerberos-unit would make sense if you were only testing the Kerberos
> protocol, but then why not just put the tests in protocol-kerberos.
>
> Enrique
>

In my mind, you should always write separate tests for separate
elements. If you want to test SASL GSSAPI, nothing forbid you to create
a GSSAPI-unit project, where you launch a kerberos + ldap server. The
very same for changepw.

At least, it will help people like me who are trying to cope with
Kerberos and all the other related subprojects : it will provide a clear
base for each project's tests, instead of mixing everything in a giant
server-uni catch all test.

As we already have around 50 subprojects, 3 more won't harm a lot ...

Emmanuel

[Alex Karasulu]

Right now the organization is setup to have two kinds of integration tests
in two separate modules:

    core-unit => which IMO should be called core-integration :)
    server-unit => which IMO should be called server-integration :)

The first module (core-unit) contains junit extensions for integration tests as sources
and in the test area contains integration tests which use these junit extensions. These
junit extensions start up the core of ADS without any MINA based network services. 

The second, server-unit, contains integration tests which test the network services of
ADS.  Up until this point we have just had LDAP network tests.  However we can add
any kind of network protocol test to these tests.

If we add a kerberos-unit then to follow the same scheme we would have to rename
server-unit to ldap-unit.  Then we would create a changepw-unit and so on.  I don't know
how many tests we're adding but if not that many keeping them in the same project
may be less overhead both in terms of maven build time and in terms of managing the
project.

I'd recommend (for now) just putting the kerberos tests and other protocol tests into a
special package to differentiate them from the LDAP integration tests in server-unit.  We
eventually need to better organize them but a package level separation should give
new comers enough of a cue as to the nature of the test.

For now I say because I'd like to keep the tests together as much as possible since we
may soon need to refactor these integration tests to use one or more test suites that starts
a single server which is cleaned out rather than deleted and restarted for each test method
or test class.  Clumping the integration tests into packages may make this job easier down
the road for us.

Thoughts?

Alex

On 7/9/07, Emmanuel Lecharny <elecharny@...> wrote:

Enrique Rodriguez a écrit :

> On 7/9/07, Emmanuel Lecharny <elecharny@...> wrote:
>
>> Guys,
>>
>> I did a simple test on my laptop writing a kerberos-unity project,
>> mimicing the server-unit project. I think that this should be a good
>> approach, as you don't have anymore dependency to deal with.
>>
>> wdyt ?
>
>
> What would you do in the case of SASL GSSAPI bind tests, which use
> both Kerberos and LDAP?  I am in favor of a single integration test
> module.
>
> Similarly, integration tests for Change Password would use Change
> Password and Kerberos.
>
> kerberos-unit would make sense if you were only testing the Kerberos
> protocol, but then why not just put the tests in protocol-kerberos.
>
> Enrique
>

In my mind, you should always write separate tests for separate
elements. If you want to test SASL GSSAPI, nothing forbid you to create
a GSSAPI-unit project, where you launch a kerberos + ldap server. The
very same for changepw.

At least, it will help people like me who are trying to cope with
Kerberos and all the other related subprojects : it will provide a clear
base for each project's tests, instead of mixing everything in a giant
server-uni catch all test.

As we already have around 50 subprojects, 3 more won't harm a lot ...

Emmanuel

[Enrique Rodriguez-2]

On 7/9/07, Alex Karasulu <akarasulu@...> wrote:
> Right now the organization is setup to have two kinds of integration tests
> in two separate modules:
>
>     core-unit => which IMO should be called core-integration :)
>     server-unit => which IMO should be called server-integration :)

I agree with these module names.

> ...
> If we add a kerberos-unit then to follow the same scheme we would have to
> rename
> server-unit to ldap-unit.  Then we would create a changepw-unit and so on.
> I don't know
> how many tests we're adding but if not that many keeping them in the same
> project
> may be less overhead both in terms of maven build time and in terms of
> managing the
> project.

I can only picture 1, maybe 2, test classes for each SASL GSSAPI,
Kerberos, and CP.  I don't see the point of separate modules with 1-2
classes.  Emmanuel makes the point that "you should always write
separate tests for separate elements" but then goes on to equate this
with separate modules (!?).  IMO, we do have separate tests already
and there is nothing confusing about having them in the same module.

> ...
> I'd recommend (for now) just putting the kerberos tests and other protocol
> tests into a
> special package to differentiate them from the LDAP integration tests in
> server-unit.  We
> eventually need to better organize them but a package level separation
> should give
> new comers enough of a cue as to the nature of the test.

I was fine with separating by test class, but package works better if
we do get above just a single test class for each protocol.  We could
even name the packages so they correspond to the module being tested,
such as:

o.a.d.server.core
o.a.d.server.protocol.kerberos
o.a.d.server.protocol.ldap
o.a.d.server.protocol.changepw

The difference between 'core' and 'protocol.ldap' would be the intent
of the integration test, ie even though you are firing up the server
and using the LDAP protocol in both cases, the target of the test may
be the core or something in the protocol workflow.

Enrique

[Emmanuel Lecharny]

Alex Karasulu a écrit :

> Right now the organization is setup to have two kinds of integration
> tests
> in two separate modules:
>
>    core-unit => which IMO should be called core-integration :)
>    server-unit => which IMO should be called server-integration :)

Well, there is a little difference between those names. I think that
core-unit and server-unit are good names, because they tell exactly what
they are good for : unit-testing the core and the server (MINA
included). IMHO, the proposed name (ie xxx-integration) is not good
because it suggest that it contains only integration tests, which is not
the case. Remember that to run in-test, you have to set -Dintegration
when running mvn.

That's another pb we have : integration tests are somhow mixed with unit
tests... Anyway, not a big deal.

> <SNIP>


> However we
> can add
> any kind of network protocol test to these tests.

Yes, we can, but I don't really think this would be a good idea. Ok, let
me explain what I have in mind : the base of all protocol is LDAP. Those
tests have been written to validate the core server (LDAP). Any other
protocols will use LDAP, so if we include other protocols tests into
server-unit, it will become a big mess. Suppose someone want to fix
something in protocol-NTP, he will have to change the server-unit to add
its new integration tests, and if the fix is not ok, then all the build
will be broken. With a clear separation, it will be possible to remove
the procotol-NTP-unit from the main pom.xml in order to build the server.

Two more points :
- I know that we are not supposed to commit a broken piece of code, but,
face the facts, it happens
- if someone is just trying to work on a specific protocol, I'm not sure
he has to run all the 15 minutes unit tests just to be sure his fix is ok.

>
> If we add a kerberos-unit then to follow the same scheme we would have to
> rename
> server-unit to ldap-unit.  

Well, may be. At least, it makes sense.

> Then we would create a changepw-unit and so on.
> I don't know
> how many tests we're adding but if not that many keeping them in the same
> project
> may be less overhead both in terms of maven build time and in terms of
> managing the
> project.

I'm not sure that having 5 more sub-projects will cost a lot compared to
the time spent in each server-unit and core-unit tests (around half a
second per test). When you run eclipse:eclipse, the full build is very
fast (less than 30seconds), compared to running mvn -Dintegration test
(15 minutes on my laptop).

Clarity should be favored, IMHO. Maybe we should add a subdivision
called protocol, and have sub-sub-projects in it :

apacheds
  |
  +--> core
  +--> ...
  +--> protocols
           |
          +--> LDAP
          +--> Kerberos
          +--> ...

>
> I'd recommend (for now) just putting the kerberos tests and other
> protocol
> tests into a
> special package to differentiate them from the LDAP integration tests in
> server-unit.  We
> eventually need to better organize them but a package level separation
> should give
> new comers enough of a cue as to the nature of the test.

Really, I don't think so. We have hundred of unit tests in server-unit,
and the package separation does not give an immediat clue about them. If
we have to rename server-unit to ldap-unit, then it won't be good. I
don't really like the idea of having all the protocol tests into
server-unit... But may be it's just me ;)

>
> For now I say because I'd like to keep the tests together as much as
> possible since we
> may soon need to refactor these integration tests to use one or more test
> suites that starts
> a single server which is cleaned out rather than deleted and restarted
> for
> each test method
> or test class.  Clumping the integration tests into packages may make
> this
> job easier down
> the road for us.

We definitively need to find a better way to organize tests. Starting
and shutting down the sevrer for each tests is just killing us. Now, as
I said, I would prefer to have a separate sub-project for kerberos tests
for the reasons I mentionned before. Of course, this is just my opinion,
and I won't roll down the floor crying and yelling if you think it's
better to gather all of them with a package separation.

I will be off for 2 weeks, so it's up to you, guys !

Emmanuel