You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by "Peter M. Goldstein" <pe...@yahoo.com> on 2002/10/01 08:15:00 UTC
[PATCH] NNTP Server fixes
All,
This is a patch which attempts to resolve some of the more obvious
problems with the current NNTP implementation. Some of the corrections
were:
i) Parsing of the LIST command and its extensions was badly incorrect.
The parsing is now correct, with all extensions and wildmat parameters
being set as appropriate.
ii) The GROUP command wiped the previous selected group, even if the
group name passed in didn't correspond to a valid newsgroup.
iii) The auth implementation was completely wrong. This fix needs
further refactoring, but the whole AuthService architecture was badly
designed. It does not allow per-connection authentication, which makes
it useless for our purposes. These changes leave the AuthService class
in place, but move the authRequired configuration to the NNTP server
handler configuration. The AuthService is unused, and should be removed
completely. If flexible, pluggable authentication services are desired
in the future, a new interface and implementation should be used.
iv) Added a number of comments.
v) Made the protocol debugging like the SMTP and POP3 debugging.
vi) Fixed an obvious typo in the TLS support for the NNTP server.
Problems that still need to be addressed:
i) Passwords are displayed in the debug log
ii) The code is very un-robust. Even slightly misformed commands will
lead to nasty errors in the handler. There is no consistent syntax
checking.
iii) The functionality needs a complete sweep to make sure that commands
do what is expected
iv) The code needs a great deal of refactoring to make it correct and
maintainable.
--Peter
RE: [PATCH] NNTP Server fixes
Posted by "Noel J. Bergman" <no...@devtech.com>.
> That is why I am agreeing with you to replace it.
OK, point of agreement. You both think that the authentication mechanism is
fundamentally flawed at the architecture level, and should be replaced. :-)
> It may not be the best mechanism, but AuthServiceFactory fixes the problem
> you found and it is better than coupling auth stuff inside handler.
I believe that Peter feels that what he's done is a quick hack to make it
work, and since both of you agree that the whole thing should be ripped out,
tossed, and replaced as soon as 2.1 is closed he'd rather go with something
that he's been testing rather than start again right now with an
AuthServiceFactory that he expects would ALSO be ripped, tossed and
replaced.
If I am reading both of you correctly, then his point of view makes some
sense. Rather than invest the time in AuthServiceFactory now, with the
intent of replacing it anyway, why not go out the door with his code (if it
works), and then start the discussion about proper design?
> I'll make sure AuthService issue is corrected before release. The change
> delta will be smaller with AuthServiceFactory from current codebase.
My personal view is that we're getting down to the short straws on the 2.1
release, and I'm getting very antsy about changes. You get a vote, I don't,
but there have been several bugs introduced recently that came from innocent
little fixes and code refactoring. Bugs happen. The only way to guarantee
that you don't introduce a new one is not to change code. So unless we have
a bug to fix, I'd just as soon not change code. Peter still has connection
handling changes that he wants to check in, which makes me very nervous at
this late date.
> We have so far talked about what should not be. Let us talk about what
> should be the right way. ideas...
The sense that I get from Peter's messages is that he wants to focus on
getting 2.1 out, and not lose his focus by getting into tangential
discussion on future architecture. This is similar to why Danny wants to
put off discussion about the Mailet API until later, and why I'd like him to
put off discussing the store interface, too. Because we all want to be able
to participate and discuss those, but right now we're trying to focus on the
current codebase.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Noel J. Bergman" <no...@devtech.com>.
> I want an AuthService that allows me to load different JAAS providers
> for the purpose of authentication ...
Sounds like a good Avalon Service, too. Any service within Avalon that had
an authentication notion could use it. So perhaps we can get the Avalon
guys into the discussion, although for first release I'd rather have it
under James control (CVS, scheduling, etc.) than Avalon.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
You're again missing the point.
> > ii) Correctly designing a pluggable, flexible, correct
> > authentication/authorization mechanism is highly non-trivial. It
>
> It may not be the best mechanism, but AuthServiceFactory fixes the
problem
> you found and it is better than coupling auth stuff inside handler.
>
> > We do not have the time to do this properly without pushing out the
> > release.
>
> I'll make sure AuthService issue is corrected before release. The
change
> delta will be smaller with AuthServiceFactory from current codebase.
No, a simple AuthServiceFactory hack is not better than the current
situation. A simple AuthServiceFactory would further expose the lousy
AuthService interface - which remains wrong for all the reasons I
discussed earlier.
There never was a pluggable AuthService. And AuthService had other,
substantial issues that I corrected by incorporating it into the handler
(Exposure of user/password as sole authentication mechanism, failure to
wipe username/password on bad auth, etc.). I'm not interested in
minimizing the delta from the past code base. I'm interested in
minimizing the delta from the current, correct, tested code base. And
no change like the one you're proposing is going to do that.
> We have so far talked about what should not be. Let us talk about what
> should be the right way. ideas...
And that's exactly what we shouldn't be doing so close to a release.
Planning new architectural changes for things that haven't worked for
nine months, after they've just been fixed, is ridiculous.
But since you asked, here goes:
I want an AuthService that allows me to load different JAAS providers
for the purpose of authentication, to allow us to take advantage of
numerous third-party authentication modules. It should have
per-protocol callbacks, to allow us to use the same root class for all
of the James services authentication needs. A limitation to
user/password is obviously not acceptable. It should support complex,
dynamic policy definition and should be able to link into external
policy engines for authorization (i.e. RSA ClearTrust, IBM Policy
Director) as well as into a simple home-grown policy class.
Of course everyone agrees with me on this one, right? We're going to
have this by tomorrow, right?
Of course not. As I've tried to explain, doing something like this
properly is difficult. It's not something you toss on the end of a
release. There are a number of issues that need to be discussed and
clarified. And it affects basically the whole server.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] NNTP Server fixes
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
> i) Current solution is broken, not only implementation-wise but also
> design/architecture
That is why I am agreeing with you to replace it.
>
> ii) Correctly designing a pluggable, flexible, correct
> authentication/authorization mechanism is highly non-trivial. It
It may not be the best mechanism, but AuthServiceFactory fixes the problem
you found and it is better than coupling auth stuff inside handler.
> We do not have the time to do this properly without pushing out the
> release.
I'll make sure AuthService issue is corrected before release. The change
delta will be smaller with AuthServiceFactory from current codebase.
We have so far talked about what should not be. Let us talk about what
should be the right way. ideas...
thanks,
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> You were pointing out a problem in the AuthService, I am suggesting
remove
> the AuthService with something that addresses what you laid out in
your
> email (replace with AuthServiceFactory - also suggested by you).
>
> We may be talking about the same thing.
No, we really aren't.
You're not reading my comments. My points are as follows:
i) Current solution is broken, not only implementation-wise but also
design/architecture
ii) Correctly designing a pluggable, flexible, correct
authentication/authorization mechanism is highly non-trivial. It
requires time, effort, and knowledge (and potentially use) of current
auth APIs. There are a number of subtleties, and I'm sure there will be
some strong divergent opinions among the developers. If you rush it,
you get things like the current AuthService - incorrect, inflexible, and
locked tightly to a particular code base.
We do not have the time to do this properly without pushing out the
release. There is very obviously not a user base aggressively demanding
this feature, as it's been broken for at least nine months. So it, like
a number of other things, can get pushed off to the next revision.
> > have an extensive background in authentication and authorization -
and
> > really feel that the AuthService should be retired.
>
> Agreed, why don't you fix the rest, I'll address the AuthSevice if you
> like.
> Don't have lot of Auth and Authorization software experience but my
> professional job is security. (not often bug free though). Please feel
> free
> to review it.
As per the above, no.
> Regarding comments etc.
> - How about use @see where possible and
Yep, as I agreed earlier.
> - Not do javadocs on private methods/variables. There does not really
seem
> to be a gain in javadoc for implementation details.
Absolutely not. All methods/variables should be documented. I don't
understand how you can say there is no gain in javadocing implementation
details. The primary goal of documentation is to convey an
understanding of the code to developers other than the author.
Implementation details are just as critical as anything else. So they
should receive the same level of documentation.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] NNTP Server fixes
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
> AuthService is not worth fixing, for all the reasons I laid out
> in my previous email. I have very strong feelings on this matter
You were pointing out a problem in the AuthService, I am suggesting remove
the AuthService with something that addresses what you laid out in your
email (replace with AuthServiceFactory - also suggested by you).
We may be talking about the same thing.
> have an extensive background in authentication and authorization - and
> really feel that the AuthService should be retired.
Agreed, why don't you fix the rest, I'll address the AuthSevice if you like.
Don't have lot of Auth and Authorization software experience but my
professional job is security. (not often bug free though). Please feel free
to review it.
Regarding comments etc.
- How about use @see where possible and
- Not do javadocs on private methods/variables. There does not really seem
to be a gain in javadoc for implementation details.
Does that sound right ?
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> bugs:
> Good stuff about the bugs. Group command was a miss and LIST seems to
have
> been a really bad one. It was half correct half wrong, but the net
effect
> was all wrong. The strange thing is that I have tested with Outlook,
> Netsacpe and KClient and still it did not show up. Others tested with
news
> group clients and it did not show either.
IMHO, there is very little use in testing protocol servers with generic
clients without doing lower level protocol testing first. Clients are
always designed to be robust in the face of misbehaving servers. You
start by getting the protocol exactly right (usually by automated tests
or by simple telnet sessions) and then you test with common clients to
find any gotchas - either errors in your implementation or client
quirks. I'm not surprised that the clients missed these issues.
> authservice:
> You are right about the AuthService. Should never have been a service,
> however I think it is a useful abstraction. Would it be possible to
have
> an
> AuthServiceFactory as a service instead of AuthService. UsersStore
etc.
> are
> ideally not a concern of the Handler, but a concern of AuthService. It
> should be easy fix AuthService this way and still have the advantages
of
> the
> abstraction.
IMHO the AuthService is not worth fixing, for all the reasons I laid out
in my previous email. I have very strong feelings on this matter - I
have an extensive background in authentication and authorization - and
really feel that the AuthService should be retired. Designing and
implementing a pluggable authentication/authorization framework is not
trivial. Moreover there are a number of generally accepted tools (JAAS)
and others under development (JSR 115) that could be relevant.
This needs to be designed properly, not patched together at the end of a
release cycle to fix something that didn't work. And considering that
this bug has been languishing for nine months or so, I don't think there
has been any extensive exercise of this code. It can wait until next
version.
> comments:
> prefer the older style more. We can define our own coding standard if
you
> prefer that. Copying a Jakarta coding style if that involves cut paste
> comments from base classes seems less than good. @see is the lesser of
two
> evils if there is no agreement. Let us reduce comments, unless they
are
> really helpful. A comment like '// see section <...>' is far more
helpful
> to
> me than say javadoc style comments on a private variable. Too much
chaff.
> I prefer 'C: ' and 'S: ' to indicate the client and server side, it
seems
> more standard and is less verbose.
Just as Danny feels strongly about some elements of the Jakarta project,
this is one I feel we must hold to. Javadoc is one of the real
strengths of the Java language, and I want to take full advantage of it.
As I mentioned before, I have no objection to the use of @see tags to
reduce the size of the comments - must they must be present. I also
believe that focusing on the comments for overridden/inherited methods
misses the big picture - that's not even close to the majority of
methods commented in this or in other documentation changes. I've
already expressed the reasons why I believe that extensive commenting is
essential to a healthy open source project, so I won't repeat them here.
Suffice to say, I'm very happy with the improvement in source code
comments over the last few months and I hope to see further improvement
before release.
> malformed messages:
> right error messages are being returned in most handlers. If they are
not
> then that should be fixed. Your patch does not seem to have changed
> malformed headers significantly. Did you have another patch that
address
> this.
As I said, this is an open issue. But basically, once you get past the
parseCommand method and into a doXXX method, there appears to be no real
syntax checking. There were random RuntimeExceptions thrown (see the
former check method) as well as potential NPEs, other exceptions at a
variety of locations (commands expecting arguments that don't get them).
I'm working on some of this now.
> Regd: throwing exception the idea was that server need not go crazy
> checking
> all possible bad arguments. It should at some point throw an exception
and
> terminate connection from an obviously bad client (or test program).
This is in violation of the spec. There is no such thing as an
"obviously bad client". The specification dictates the appropriate
behavior for cases of malformed values. So we can't do that.
> Excellent patches and bad bugs killed.
Thank you.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Commenting Issues
Posted by "Noel J. Bergman" <no...@devtech.com>.
[Coalescing the NNTP server discussion on commenting]
> IMHO I'd agree that javadocs should only be used on public and package
scope
> methods and fields. [Danny]
I can agree that javadocs MUST be used on public/package scope items. I
won't say that they MUST NOT be used elsewhere. I just don't have any
particular cares about the latter.
> Private fields and methods need only be commented, and
> privately, where their use is not clear from the code. [Danny]
Ok, this means that most of them should be commented except for the most
trivial. However, I am of the mind that comments should be a way of telling
another programmer (or myself, much later), what and why. Especially the
why. A comment like:
item.unlock(); // remove the lock
is worthless. I can see that we are calling the unlock method. But a
comment like:
// spool.remove() has a side-effect! It unlocks the
// message so that other threads can work on it! If
// we don't remove it, we must unlock it!
spool.unlock(key);
tells me something important about the operation of the code.
> I subscribe to the XP view that a requirement for extensive commenting of
> sources is in avoidance of writing clear code. [Danny]
[Personal aside: XP is nothing more than a marketing slick relabeling for
Gen-Xers of tried and true practices, many of which date back 30 years or
more.]
IAE, what is clear to one programmer who knows an RFC inside-out may not be
clear to another. I put that comment about spooling in for the next person
who wanted to work on that code. By the time I figured out how spooling
worked, the code would have been "clear" to me (at least for the duration of
that week ;-)).
> Obviously this has to be tempered, in a OS project particularly, with the
> need to make the code accessable to any would be developer [Danny]
I agree that we must establish the mindset that we at least have to comment
the code as if we were explaining it to another person so that they can make
changes. Which is, of course, what we're really doing.
This is basically what you and Peter are both agreeing. As Peter said,
"James developers don't have the benefit of any of these [face-to-face XP]
methodologies [that enhance communication between developers], so commenting
is correspondingly more important as a tool for inter-developer
communication."
> > - Not do javadocs on private methods/variables. There does not really
> > seem to be a gain in javadoc for implementation details. [Harmeet]
> All methods/variables should be documented. I don't understand
> how you can say there is no gain in javadocing implementation
> details. [Peter]
Javadocs !== documentation. Javdocs <= documentation. He isn't saying that
methods/variables should not be documented. He's just saying that he
doesn't see the gain of using javadoc tags for private implementation items.
In fact, I could argue that exposing that private members with javadoc tags
is dangerous, on the grounds that implementation details should not be
exposed outside of the source. We don't want someone to read the javadocs
and make assumptions about implementation. Only those things that make up
the contract should be javadoc'd. I'm willing to listen to a contrary view,
but please explain it.
> The primary goal of documentation is to convey an understanding of the
code
> to developers other than the author. [Peter]
Peter, the good news is that no one appears to be disagreeing about
commenting. The issue here appears to be only whether or not to use javadoc
tags within the comments for private items.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Mailet API logging and other changes
Posted by "Noel J. Bergman" <no...@devtech.com>.
> Lets leave the Mailet logging, and other mailet API issues 'till after the
> release.
I wasn't planning to make any changes, other than reduce some debugging
output, until after the release. Certainly not API changes! I'm not
looking to cause a problem. :-)
As things stand, I'm done. Other than checking in some new mailet/matcher
code, I've got nothing on my plate for 2.1 release. Which is good, since
Peter wants feature freeze tomorrow. :-)
The logs are very important to me. I maintain a monitor in the upper
righthand side of my workstation watching all of the James logs in
real-time. If anything, I have added some useful logging information, e.g.,
when a socket times out now, we now log with whom it timed out. OTOH, too
much tracing information, once the code works, can mask real problems.
One thing that I really want for the next release is the ability to
dynamically configure logging. For example, if I suspect a problem in
handshaking, I want to be able to turn on debug for a protocol handler, and
then turn it off afterwards. Right how I have to reboot to affect that
change.
> There are a number of potential changes we need to discuss to the API and
> James' implementation of it.
The storage interface is the other major area that comes to mind.
> I'm prepared to review my opposition to advanced logging in the API, but
not
> right now.
I agree. API changes should all be post-2.1.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: Mailet API logging and other changes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Danny et al,
> Lets leave the Mailet logging, and other mailet API issues 'till after
the
> release.
+1
I concur. Focusing on the release now (and getting it out) will help us
resolve these issues sooner.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Mailet API logging and other changes
Posted by Danny Angus <da...@apache.org>.
Noel, All,
Lets leave the Mailet logging, and other mailet API issues 'till after the
release.
There are a number of potential changes we need to discuss to the API and
James' implementation of it.
I'm prepared to review my opposition to advanced logging in the API, but not
right now.
d.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Danny,
> I subscribe to the XP view that a requirement for extensive commenting
of
> sources is in avoidance of writing clear code.
I don't necessarily disagree with this view - on XP projects. As we
discussed on the list the last time this came up, there are a number of
XP methodologies that are precisely designed to spread the knowledge in
lieu of code commenting (developer rotation, pair programming, design
reviews, etc.). James developers don't have the benefit of any of these
methodologies, so commenting is correspondingly more important as a tool
for inter-developer communication.
> Obviously this has to be tempered, in a OS project particularly, with
the
> need to make the code accessable to any would be developer, and
frankly I
> think James own code is not the inscrutable part of this, the steep
> learning
> curve for interested participants is James use of Avalon in all its
> guises.
Speaking as someone who recently came up to speed on the James and
Avalon code, I'd have to disagree with this assertion. The Avalon code
and lifecycle were very easy to pick up - precisely because Avalon is
very well documented. I found the James code far more confusing and
inscrutable.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by Danny Angus <da...@apache.org>.
> A comment like '// see section <...>' is far more
> helpful to
> me than say javadoc style comments on a private variable.
IMHO I'd agree that javadocs should only be used on public and package scope
methods and fields.
Private fields and methods need only be commented, and privately, where
their use is not clear from the code.
I subscribe to the XP view that a requirement for extensive commenting of
sources is in avoidance of writing clear code.
Obviously this has to be tempered, in a OS project particularly, with the
need to make the code accessable to any would be developer, and frankly I
think James own code is not the inscrutable part of this, the steep learning
curve for interested participants is James use of Avalon in all its guises.
d.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] NNTP Server fixes
Posted by Harmeet Bedi <ha...@kodemuse.com>.
bugs:
Good stuff about the bugs. Group command was a miss and LIST seems to have
been a really bad one. It was half correct half wrong, but the net effect
was all wrong. The strange thing is that I have tested with Outlook,
Netsacpe and KClient and still it did not show up. Others tested with news
group clients and it did not show either.
authservice:
You are right about the AuthService. Should never have been a service,
however I think it is a useful abstraction. Would it be possible to have an
AuthServiceFactory as a service instead of AuthService. UsersStore etc. are
ideally not a concern of the Handler, but a concern of AuthService. It
should be easy fix AuthService this way and still have the advantages of the
abstraction.
comments:
prefer the older style more. We can define our own coding standard if you
prefer that. Copying a Jakarta coding style if that involves cut paste
comments from base classes seems less than good. @see is the lesser of two
evils if there is no agreement. Let us reduce comments, unless they are
really helpful. A comment like '// see section <...>' is far more helpful to
me than say javadoc style comments on a private variable. Too much chaff.
I prefer 'C: ' and 'S: ' to indicate the client and server side, it seems
more standard and is less verbose.
My preference is that protocol level debugging should not be controlled by
logger debug level. I prefer a system variable for this level of debugging
but no big thing.
malformed messages:
right error messages are being returned in most handlers. If they are not
then that should be fixed. Your patch does not seem to have changed
malformed headers significantly. Did you have another patch that address
this.
Regd: throwing exception the idea was that server need not go crazy checking
all possible bad arguments. It should at some point throw an exception and
terminate connection from an obviously bad client (or test program).
Excellent patches and bad bugs killed.
cheers,
Harmeet
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
To: "'James Developers List'" <ja...@jakarta.apache.org>
Sent: Tuesday, October 01, 2002 11:04 AM
Subject: RE: [PATCH] NNTP Server fixes
>
> Harmeet,
>
> > > ii) The GROUP command wiped the previous selected group, even if the
> > > group name passed in didn't correspond to a valid newsgroup.
> >
> > Could you give a telnet comparison, or the real bug you found.
> >
> > wildmat was not 100% but 99% based on what is used by nntp clients and
> > supported by regex package.
>
> Consider a news server that hosts a single group, alt.james
>
> Examples of bugs found:
>
> Client has an active news session. Types GROUP alt.james . This sets
> the internal group pointer to alt.james . Now type GROUP moose.squirrel
> . This group doesn't exist, so the group pointer should remain at
> alt.james . It doesn't - it gets set to null.
>
> Client has an active news session. Types LIST NEWSGROUPS. This gets
> parsed by the handler as LIST, because when going through the "if else"
> chain the second token ("NEWSGROUPS") gets consumed during the check as
> to whether it matches LIST EXTENSIONS. The key problem with the LIST
> commands can be found here:
>
> else if ( command.equals("LIST") && tokens.hasMoreTokens() &&
> tokens.nextToken().toUpperCase(Locale.US).equals("EXTENSIONS") )
> doLISTEXTENSIONS();
> else if ( command.equals("LIST") && tokens.hasMoreTokens() &&
> tokens.nextToken().toUpperCase(Locale.US).equals("OVERVIEW.FMT") )
> doLISTOVERVIEWFMT();
> ...
> else if (command.equals("LIST"))
> doLIST(tokens);
>
> Note that each call to tokens.nextToken() consumes a token, advancing
> the StringTokenizer.
>
> It doesn't matter how good or correct the wildmat implementation is.
> The wildmat string isn't being seen by LIST NEWSGROUPS, because of the
> problem above. Honestly, I didn't even look at the wildmat
> implementation - I was focused on fixing problems in NNTPHandler.
>
> > > iii) The auth implementation was completely wrong. This fix needs
> > > further refactoring, but the whole AuthService architecture was
> badly
> > > designed. It does not allow per-connection authentication, which
> makes
> > > it useless for our purposes. These changes leave the AuthService
> class
> > > in place, but move the authRequired configuration to the NNTP server
> > > handler configuration. The AuthService is unused, and should be
> removed
> > > completely. If flexible, pluggable authentication services are
> desired
> > > in the future, a new interface and implementation should be used.
> >
> > AuthService alllowed
> > - validation on actual commands, via <isAuthorized>
> > - User Pasword state machine encapsulation.
> > - plugin authentication mechanism.
> > Shouldn't the bug that you found be fixed in AuthService ?
> >
> > Regarding perconnection check in authservice - AuthService is an
> > interface,
> > how can it prevent it ?
> > Another implemenation can be plugged in if need be used differently in
> the
> > handler. How does AuthService prevent you from doing what you need ?
>
> You're missing the point. AuthService doesn't allow any of these
> things. The bug can't be fixed in AuthService because AuthService is
> conceptually incorrect.
>
> Consider the following situation. Bob and Simon both connect to the
> NNTP server. Bob is a legitimate user, Simon is not. Bob's news client
> authenticates him and he gets access to services. That's all fine and
> dandy. Simon connects and attempts to access services for which he's
> not authorized. And he gets access to them. Why? Because Bob has
> already authenticated and Simon's NNTPHandler grabs the same AuthService
> that Bob grabbed. In fact, in the current code, once one person
> authenticates everyone will be authenticated. Disastrous.
>
> AuthService should never have been a Phoenix block. An
> AuthServiceFactory would be a potentially correct alternative, but
> considering the other problems with AuthService (non-standard code, not
> unified with other authentication services in James, has an obvious
> java.* replacement in JAAS) and our imminent deadlines, I chose to move
> the code into the NNTP Handler and make it correct.
>
> As far as the rest,
>
> i) validation on actual commands - that's a trivial feature as
> implemented. All the current code does is "return isAuthenticated() ||
> command.equals(A) || command.equals(B) || command.equals(C)". This is
> supported in my changes. A properly broken out
> authentication/authorization mechanism would also support this.
>
> ii) User/Password state machine encapsulation - For the moment, I
> sacrificed this. For all the reasons described above, AuthService is
> not fixable. Given time constraints and the time necessary for a proper
> debate as to how to architect the mechanism, I felt it was more
> important to get it working in a simple fashion.
>
> iii) plugin authentication mechanism - with all due respect, this
> doesn't exist. A plugin authentication mechanism would need to allow a
> variety of authentication credentials. As such it would need to have
> some control over the client/server dialogue. AuthService is lock
> stepped to user/password and has no control over the client/server
> dialogue. Thus it is not a plugin authentication mechanism.
>
> > >
> > > iv) Added a number of comments.
> >
> > Found some of the comments distracting. What is the point in cutting
> and
> > pasting comments that are in base class in the derived class as well ?
> > Wouldn't javadoc take care of this ?
> > It would be better to have protocol conformance or implementation
> comments
> > instead.
>
> For the most part the comments are specific to the particular class.
> The few cases where they aren't specific, they are there largely for
> completeness. It has also been my experience that developers are more
> likely to alter pre-existing Javadoc than add new Javadoc. Finally, the
> Jakarta code standards state that all methods should have Javadoc. As
> we are a Jakarta project, our code base to be in line with Jakarta
> standards. We could certainly use @see tags for some of these inherited
> methods, as is done in some of the FetchPOP code, but it happens not to
> be my default style.
>
> > Is there a way to avoid malformed commands in any protocol. What can
> be
> > done
> > ? One could throw an exception and stop the connection. Isn't that
> what
> > was/is happenning. An example would be better.
>
> That's not what's supposed to happen as per the RFC. NNTP, like SMTP,
> has full support for bad syntax notification. So the server should be
> notifying the client that the syntax of the command was erroneous.
> Instead, exceptions are thrown and caught at a higher level, where the
> doQUIT method is invoked. Not a well behaved server. See the SMTP
> server (which does syntax checking on all incoming commands) for an
> example of correct behavior.
>
> --Peter
>
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
> > I understand that @see is correctly used to document an overriden
> interface
> > or abstract method where that method doesn't warrant any additional
> comment
>
> Personally, I'd rather have the @see tag, unless there is
implementation
> specific content in the comment. Otherwise it is clutter.
No real objection. I'll mod the code appropriately.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Noel J. Bergman" <no...@devtech.com>.
> > We could certainly use @see tags for some of these inherited
> > methods, as is done in some of the FetchPOP code, but it
> > happens not to be my default style.
> I understand that @see is correctly used to document an overriden
interface
> or abstract method where that method doesn't warrant any additional
comment
Personally, I'd rather have the @see tag, unless there is implementation
specific content in the comment. Otherwise it is clutter.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by Danny Angus <da...@apache.org>.
> We could certainly use @see tags for some of these inherited
> methods, as is done in some of the FetchPOP code, but it happens not to
> be my default style.
I understand that @see is correctly used to document an overriden interface
or abstract method where that method doesn't warrant any additional comment,
it makes it clear that the method primarily exists to provide an
implemention of the interface/abstract method.
d.
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> > ii) The GROUP command wiped the previous selected group, even if the
> > group name passed in didn't correspond to a valid newsgroup.
>
> Could you give a telnet comparison, or the real bug you found.
>
> wildmat was not 100% but 99% based on what is used by nntp clients and
> supported by regex package.
Consider a news server that hosts a single group, alt.james
Examples of bugs found:
Client has an active news session. Types GROUP alt.james . This sets
the internal group pointer to alt.james . Now type GROUP moose.squirrel
. This group doesn't exist, so the group pointer should remain at
alt.james . It doesn't - it gets set to null.
Client has an active news session. Types LIST NEWSGROUPS. This gets
parsed by the handler as LIST, because when going through the "if else"
chain the second token ("NEWSGROUPS") gets consumed during the check as
to whether it matches LIST EXTENSIONS. The key problem with the LIST
commands can be found here:
else if ( command.equals("LIST") && tokens.hasMoreTokens() &&
tokens.nextToken().toUpperCase(Locale.US).equals("EXTENSIONS") )
doLISTEXTENSIONS();
else if ( command.equals("LIST") && tokens.hasMoreTokens() &&
tokens.nextToken().toUpperCase(Locale.US).equals("OVERVIEW.FMT") )
doLISTOVERVIEWFMT();
...
else if (command.equals("LIST"))
doLIST(tokens);
Note that each call to tokens.nextToken() consumes a token, advancing
the StringTokenizer.
It doesn't matter how good or correct the wildmat implementation is.
The wildmat string isn't being seen by LIST NEWSGROUPS, because of the
problem above. Honestly, I didn't even look at the wildmat
implementation - I was focused on fixing problems in NNTPHandler.
> > iii) The auth implementation was completely wrong. This fix needs
> > further refactoring, but the whole AuthService architecture was
badly
> > designed. It does not allow per-connection authentication, which
makes
> > it useless for our purposes. These changes leave the AuthService
class
> > in place, but move the authRequired configuration to the NNTP server
> > handler configuration. The AuthService is unused, and should be
removed
> > completely. If flexible, pluggable authentication services are
desired
> > in the future, a new interface and implementation should be used.
>
> AuthService alllowed
> - validation on actual commands, via <isAuthorized>
> - User Pasword state machine encapsulation.
> - plugin authentication mechanism.
> Shouldn't the bug that you found be fixed in AuthService ?
>
> Regarding perconnection check in authservice - AuthService is an
> interface,
> how can it prevent it ?
> Another implemenation can be plugged in if need be used differently in
the
> handler. How does AuthService prevent you from doing what you need ?
You're missing the point. AuthService doesn't allow any of these
things. The bug can't be fixed in AuthService because AuthService is
conceptually incorrect.
Consider the following situation. Bob and Simon both connect to the
NNTP server. Bob is a legitimate user, Simon is not. Bob's news client
authenticates him and he gets access to services. That's all fine and
dandy. Simon connects and attempts to access services for which he's
not authorized. And he gets access to them. Why? Because Bob has
already authenticated and Simon's NNTPHandler grabs the same AuthService
that Bob grabbed. In fact, in the current code, once one person
authenticates everyone will be authenticated. Disastrous.
AuthService should never have been a Phoenix block. An
AuthServiceFactory would be a potentially correct alternative, but
considering the other problems with AuthService (non-standard code, not
unified with other authentication services in James, has an obvious
java.* replacement in JAAS) and our imminent deadlines, I chose to move
the code into the NNTP Handler and make it correct.
As far as the rest,
i) validation on actual commands - that's a trivial feature as
implemented. All the current code does is "return isAuthenticated() ||
command.equals(A) || command.equals(B) || command.equals(C)". This is
supported in my changes. A properly broken out
authentication/authorization mechanism would also support this.
ii) User/Password state machine encapsulation - For the moment, I
sacrificed this. For all the reasons described above, AuthService is
not fixable. Given time constraints and the time necessary for a proper
debate as to how to architect the mechanism, I felt it was more
important to get it working in a simple fashion.
iii) plugin authentication mechanism - with all due respect, this
doesn't exist. A plugin authentication mechanism would need to allow a
variety of authentication credentials. As such it would need to have
some control over the client/server dialogue. AuthService is lock
stepped to user/password and has no control over the client/server
dialogue. Thus it is not a plugin authentication mechanism.
> >
> > iv) Added a number of comments.
>
> Found some of the comments distracting. What is the point in cutting
and
> pasting comments that are in base class in the derived class as well ?
> Wouldn't javadoc take care of this ?
> It would be better to have protocol conformance or implementation
comments
> instead.
For the most part the comments are specific to the particular class.
The few cases where they aren't specific, they are there largely for
completeness. It has also been my experience that developers are more
likely to alter pre-existing Javadoc than add new Javadoc. Finally, the
Jakarta code standards state that all methods should have Javadoc. As
we are a Jakarta project, our code base to be in line with Jakarta
standards. We could certainly use @see tags for some of these inherited
methods, as is done in some of the FetchPOP code, but it happens not to
be my default style.
> Is there a way to avoid malformed commands in any protocol. What can
be
> done
> ? One could throw an exception and stop the connection. Isn't that
what
> was/is happenning. An example would be better.
That's not what's supposed to happen as per the RFC. NNTP, like SMTP,
has full support for bad syntax notification. So the server should be
notifying the client that the syntax of the command was erroneous.
Instead, exceptions are thrown and caught at a higher level, where the
doQUIT method is invoked. Not a well behaved server. See the SMTP
server (which does syntax checking on all incoming commands) for an
example of correct behavior.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: [PATCH] NNTP Server fixes
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
>
> All,
>
> This is a patch which attempts to resolve some of the more obvious
> problems with the current NNTP implementation. Some of the corrections
> were:
>
> i) Parsing of the LIST command and its extensions was badly incorrect.
> The parsing is now correct, with all extensions and wildmat parameters
> being set as appropriate.
>
> ii) The GROUP command wiped the previous selected group, even if the
> group name passed in didn't correspond to a valid newsgroup.
Could you give a telnet comparison, or the real bug you found.
wildmat was not 100% but 99% based on what is used by nntp clients and
supported by regex package.
>
> iii) The auth implementation was completely wrong. This fix needs
> further refactoring, but the whole AuthService architecture was badly
> designed. It does not allow per-connection authentication, which makes
> it useless for our purposes. These changes leave the AuthService class
> in place, but move the authRequired configuration to the NNTP server
> handler configuration. The AuthService is unused, and should be removed
> completely. If flexible, pluggable authentication services are desired
> in the future, a new interface and implementation should be used.
AuthService alllowed
- validation on actual commands, via <isAuthorized>
- User Pasword state machine encapsulation.
- plugin authentication mechanism.
Shouldn't the bug that you found be fixed in AuthService ?
Regarding perconnection check in authservice - AuthService is an interface,
how can it prevent it ?
Another implemenation can be plugged in if need be used differently in the
handler. How does AuthService prevent you from doing what you need ?
>
> iv) Added a number of comments.
Found some of the comments distracting. What is the point in cutting and
pasting comments that are in base class in the derived class as well ?
Wouldn't javadoc take care of this ?
It would be better to have protocol conformance or implementation comments
instead.
>
> v) Made the protocol debugging like the SMTP and POP3 debugging.
>
> vi) Fixed an obvious typo in the TLS support for the NNTP server.
>
> Problems that still need to be addressed:
>
> i) Passwords are displayed in the debug log
>
> ii) The code is very un-robust. Even slightly misformed commands will
> lead to nasty errors in the handler. There is no consistent syntax
> checking.
Is there a way to avoid malformed commands in any protocol. What can be done
? One could throw an exception and stop the connection. Isn't that what
was/is happenning. An example would be better.
>
> iii) The functionality needs a complete sweep to make sure that commands
> do what is expected
>
> iv) The code needs a great deal of refactoring to make it correct and
> maintainable.
>
Excellent points but a bit general. :-)
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: [PATCH] NNTP Server fixes
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Forgot to put on the last email:
Comments, questions welcome. If there are no objections I'm going to
submit this code in a day or so.
--Peter
> -----Original Message-----
> From: Peter M. Goldstein [mailto:peter_m_goldstein@yahoo.com]
> Sent: Monday, September 30, 2002 11:15 PM
> To: 'James Developers List'
> Subject: [PATCH] NNTP Server fixes
>
>
> All,
>
> This is a patch which attempts to resolve some of the more obvious
> problems with the current NNTP implementation. Some of the
corrections
> were:
>
> i) Parsing of the LIST command and its extensions was badly incorrect.
> The parsing is now correct, with all extensions and wildmat parameters
> being set as appropriate.
>
> ii) The GROUP command wiped the previous selected group, even if the
> group name passed in didn't correspond to a valid newsgroup.
>
> iii) The auth implementation was completely wrong. This fix needs
> further refactoring, but the whole AuthService architecture was badly
> designed. It does not allow per-connection authentication, which
makes
> it useless for our purposes. These changes leave the AuthService
class
> in place, but move the authRequired configuration to the NNTP server
> handler configuration. The AuthService is unused, and should be
removed
> completely. If flexible, pluggable authentication services are
desired
> in the future, a new interface and implementation should be used.
>
> iv) Added a number of comments.
>
> v) Made the protocol debugging like the SMTP and POP3 debugging.
>
> vi) Fixed an obvious typo in the TLS support for the NNTP server.
>
> Problems that still need to be addressed:
>
> i) Passwords are displayed in the debug log
>
> ii) The code is very un-robust. Even slightly misformed commands will
> lead to nasty errors in the handler. There is no consistent syntax
> checking.
>
> iii) The functionality needs a complete sweep to make sure that
commands
> do what is expected
>
> iv) The code needs a great deal of refactoring to make it correct and
> maintainable.
>
> --Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>