tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Costin Manolache <cos...@eng.sun.com>
Subject Re: cvs commit: jakarta-tomcat/src/share/org/apache/tomcat/servlets AuthServlet.java
Date Sat, 08 Apr 2000 01:37:18 GMT
> 
> >
> > > * It doesn't go to the <form-error-page> page on invalid
> > >   username/password; it just goes back to the login page again.
> > >   The interactions between the players on this didn't make a
> > >   quick fix easy to identify.
> >
> > That doesn't work for BASIC authentication either - and the
> >   reason is that
> > void errorPage() is not  implemented :-)
> >
> 
> >
> > The behavior is identical for BASIC and FORM - if user doesn't
> > match we call errorPage() to handle that.
> >
> > ( in BASIC, if you click CANCEL in the login dialog you should
> > be redirected to the error page - which doesn't happen )
> >
> 
> It doesn't seem to me that it would be the same error page in any case.
> <form-error-page> is only defined if you are processing a <form-login-config>;
> in other words, you are defining what page to use when form-based login detects
> an error.  For BASIC authentication it should go to the server's default page
> for error 401.  (Right now it doesn't -- it just displays a blank page).
Yes, the equivalent of <form-error-page> is a 401 - that is not 
displayed because errorPage() is not
implemented. The behavior is the same - if the authentication fails, we 
need to do something.

Implementing the "positive" branch seemed more important in given time, 
and that's the reason
errorPage()  is empty. But that doesn't make FORM authentication "worse" 
than BASIC, and
if we feel the lack of error page will confuse users - we should rename 
BASIC to
> EXPERIMENTAL_BASIC too.
> 
> > > * Once you successfully authenticate, getRemoteUser() is
> > >   set correctly but getUserPrincipal() is not.
> >
> > Again - the code is identical for FORM and BASIC -
> > and it seems to work the same way ( same output and
> > user principal for both of them ).
> >
> 
> It may have at one point but doesn't now -- possibly as a side effect of one
> bug I had to fix to get BASIC to work.  Previously, there was code that set the
> remote user value if there was an "Authorization" HTTP header in the message --
> which caused the check for a valid password to be bypassed, and it would thus
> accept *any* username/password combination.  I fixed this for BASIC, which
> might have broken it for FORM.
It's just a matter of fixing the FORM with the same code - or even 
better moving the common
code in a separate method that will be used by DIGEST too. Since we have 
the fix - I don't see
> reason to not apply it to FORM.
> 
> 
>  of the previous issue, isUserInRole()
> 
> > >   never returns true even though the user is registered in
> 
> > >   the role via conf/tomcat-users.conf (I had to fix a parsing
> 
> > >   issue to recognize a comma-delimited set of roles).
> 
> >
> 
> > Same is true - there is no difference between FORM and BASIC
> 
> > except the way they get the user and password.
> 
> >
> 
> 
> 
> I tried it with the current code base (validating the EXPERIMENTAL_FORM change)
> 
> and it currently behaves as I described.  I enhanced the index.jsp page that is
> 
> inside the protected area to help debug this.

That means if we use the same fix as in BASIC we'll have identical 
results for FORM.
How difficult is the fix ? Because if that's the only problem we can 
support both (or none).
> 
>  > For BASIC authentication it all seems to work correctly.  I tried all the
> 
> > > boundary conditions I could think of and they now work right, but I
> 
> > > certainly could have missed something.
> 
> >
> 
> > Except error page - that doesn't work in both cases, everything seems
> 
> > to work identical.
> 
> >
> 
> 
> 
> Yep ... but in this case they won't be quite identical.
> 
> 
> 
> However, I notice that I forgot a biggie on the list of things not working
> 
> right -- at the moment we're only saving the request URI when we challenge the
> 
> user with the form login page.  That means that any query parameters get lost
> 
> when the user successfully authenticates and you go back to the original
> 
> request.  However, there's a more serious problem as well -- all of the HTTP
> 
> headers, and the request data (for example, POST parameters) also get lost.
> 
> Per a clarification that Danny is going to issue, all of this needs to be saved
> 
> so you can completely emulate the original request when the user successfully
> 
> authenticates.  This one is going to be a lot more work to get right.  [Bug
> 
> #171 was the original report].

Saving parameters and headers is just few lines of code ( in addition to 
URI we save the headers
and parms ) - but if the user does a POST with an arbitrary body ( that 
is supposed to be 
read via input stream - like an upload ) - implementing such thing opens 
door to DOS attacks
and is very inefficient.  There are other situations ( like HEAD) that 
just can't be implemented.
My point is that it's hard to implement un-issued specification - and I 
suppose there are a
lot more details to be settled in the spec side (  having the original 
request is not going to
work in all cases, so how much is supposed to work is a long story, hard 
to fit on the
book border)

Anyway - what's done is done, and probably it's better that way - FORM 
login is
one of the strangest things, and not having it enabled might be good.


Costin


Mime
View raw message