User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021111 Phoenix/0.4 Build Identifier: Mozilla should implement an interface for writing plugins to add new forms of authentication to mozilla. This is a very popular feature in Apache and I believe the same ability would be popular in a browser. Plugins would probably have already been created to fix bugs like bug 23679 and bug 17578 if such an api were available. Features 1. Ability to install authentication plugins without recompling mozilla. Possible Plugins NTLM, SPNEGO, KerberosV4, KerberosV5, GENERIC_GSSAPI, PGP, PEM., etc. Reasons: 1. Not everyone needs Kerberos, NTLM, or other future authentication methods but the list of supported Auth types will just continue to grow. Plugins will allow users to just install what they need. 2. Cross platform solutions would not be such a necessity. As it has been pointed out in the NTLM bug- Windows users should use SSPI, UNIX users should probably use something related to Samba. 3. Some licensing problems would go away (Samba is GPL so could without some magic it can't be linked with moz.) 4. Moz + Apache would become the perfect test environment for new authentication types and moz auth plugin programmers wouldn't have to be expert moz programmers. Reproducible: Always Steps to Reproduce: 1. 2. 3.

the problem is coming up with a sufficient API. we already have nsIHttpAuthenticator, but that is only useful for HTTP authentication schemes (following the rules of RFC 2617), which NTLM, for example, most certainly is not. major changes are needed to the HTTP implementation to support NTLM. that is why NTLM has not been implemented yet. accepting, but futuring...

Apache takes a VERY general approach to auth handlers, basically they get access to information about the request, the IP or hostname of the machine on the otherside of the connection, and access to all HTTP headers and the ability to add or change them. (Apache's one remaining problem is how to keep state when there are multiple requests and replies for the same authentication, and that can be handled using the EAPI of mod_ssl.) I realize such an approach may seem messy when mozilla uses such a structured approach, but as you pointed out these edge case authentication types will never easily be described using a normal api. Do you consider this a feasable approach for creating a mozilla auth plugin api?

> Do you consider this a feasable approach for creating a mozilla auth > plugin api? you'd have to describe the approach in greater detail. how would you propose modifying mozilla's existing HTTP auth plugin API (i.e., nsIHttpAuthenticator)?

My reason for opening this bug was not to complain about the current nsIHttpAuthenticator api (which I am just learning) but to ask that new authentication types could be added dynamically from plugins without having to recompile the mozilla source (or even require me to maintain a copy of the mozilla source except for development purposes). I want to be able to just put a copy of an authentication plugin out on a webpage like someone would for a acrobat or flash plugin and tell users if you need Mozilla to support X (SPNEGO, NTLM, KerberosV5, etc.) go install the plugin. I apologize if this ability already exists in mozilla and I just was not aware of it, but I have found no examples or documentation that explains how to do it or even if it can be done. If I'm wrong please point me in the right direction. Thanks, Christopher

that is what XPCOM is for and .xpi's are for. you can easily create a XPCOM component that implements nsIHttpAuthenticator (or whatever mozilla interfaces) and distribute it using as a .xpi. however, nsIHttpAuthenticator is not a frozen API, which means that doing this for nsIHttpAuthenticator would not be guaranteed to work with future versions of mozilla. see http://mozilla.org/catalog/architecture/xpcom/ for more info. also, ask questions on netscape.public.mozilla.xpcom. i'm tempted to close this bug as INVALID/WONTFIX unless you can point out problems w/ nsIHttpAuthenticator that prevent you from implementing drop-in auth plugins (in addition to the problems already related to NTLM auth that is).

An authentication scheme needs the ability to decline to handle a request. For example an authentication type based off of Kerberos is not available if the user does not have any credentials so the browser should be able to fallback to the other auth handlers. In addition, an auth handler should be be given the option to decline if there is already a cache entry for that request (for that domain or for that exact request), otherwise if the authentication method fails the auth handler and the server will get into an infinite loop trying the same authentication scheme when there are other schemes that may be able to fulfill the authentication.

chris: good points.. thx! will definitely make sure those requirements are satisfied.

Attached is an early (but fully working) patch for updating the nsIHttpAuthenticator api and all the necessary changes so it is used by nsIHttpChannel, nsIHttpBasicAuth and nsIHttpDigestAuth. Here is a summary of the changes, 1. nsIHttpChannel now keeps a member variable named mScheme which keeps track of what authentication scheme was used for the last request. 2. SelectChallenge now accepts two new parameters pscheme, which consists of the previous scheme attempted and retry, a variable, which states whether that scheme should be tried again, or skipped. 3. Select Challenge is now called before getCredentials, and the selected Authenticator is passed into getCredentials. 4. GenerateCredentials now calls getCredentials in a loop. If the chosen authenticator fails at any point before it generates a credential to pass, the next available authenticator can be selected. 5. The nsIHttpAuthenticator Interface now includes a function named Retry, which getCredentials calls before it tries a tries the same authentication type again, after failing previously. Assumptions made in the code. 1. For a particular request the web servers will always return the same list of supported authentication types, in the same order. I'm not sure if I used the correct format for the patch, I can't access Mozilla's CVS from from where I am, so I created the patch off a nightly source tarball from 1/25/03.

Darin, could you give me some feedback on this patch? Thanks.

Comment on attachment 113010 [details] [diff] [review] authentication api update a bit busy at the moment.. adding this to my review queue.

please see my changes to the authenticator API in bug 159015. those changes generalize the API a bit more. looks like you've added some things here that generalize the behavior of nsHttpChannel even further. the fallback mechanisms in particular that you outline here and have coded up sound really great. i plan to revisit this patch after my patch for bug 159015 lands (scheduled for 1.4 alpha).

What about SASL? It's an authentication API defined in a RFC for text based protocols. The Cyrus implementation is by Cyrus IMAP, Sendmail and others. If Mozilla used this API it could either use it's own plug-ins or some 3rd party. Eg the mentioned Cyrus lib on Unix.

right, i intend to make this API flexible enough for that authentication scheme as well. the point is that we need to do the following: 1- abstract nsIHttpAuthenticator further 2- freeze nsIHttpAuthenticator per the mozilla.org guidelines finally, this will allow folks to write their own XPCOM component that can be dropped into any mozilla based application. authors will be able to provide .xpi files that users can just click-to-install. but, this probably won't happen during the 1.4 timeframe. i really appreciate your help in determining what kinds of modifications are needed to support the variety of authentication schemes that exist. anything else you want to add would be most appreciated. i'll be sure to roll all of it up in the final version of nsIHttpAuthenticator (and supporting framework) before the interface is frozen.

Mutual authentication isn't possible if the last auth token comes from the server. The client will receive a HTTP OK response and a WWW-Authenticate message simultaneously. If that happens the auth handler will not fire, so the client cannot process the last auth token.

interesting.. i'll keep that in mind.

This patch against 1.4.1 allows an authentication plugin to error out at any point during the creation of the credential at which point the next available auth plugin is tried.

The above patch is much smaller than the previous and updated to 1.4.1. (Thanks Darin for all othe updates you made to the nsHttpChannel that made this so much easier.) At this point the code is really only useful for client side errors. It will not automatically fail over to do a different auth type if the server keeps rejecting what is being sent, even though a particular auth method may only know 1 thing like a non-identity based authentication method.

yes, i like this design. it makes sense to give the authenticator a shot at producing credentials before assuming that we will only use that authenticator. patch looks good... i may have a few tweaks to add, but for the most part i like this. more review comments in a bit.

Comment on attachment 133724 [details] [diff] [review] updated patch ok, one problem straight off the bat... you must call ChallengeReceived before GenerateCredentials. that is a requirement of the nsIHttpAuthenticator interface.

i think trying to call GenerateCredentials up front is going to cause trouble. can you instead, call ChallengeReceived and failover to the next auth scheme from there? it seems like the authenticator should have enough information when ChallengeReceived is called to determine whether or not it can handle the server challenge.

I'm not sure what you are referring to, I created a new nsHttpChannel function called nsHttpChannel::GenerateCredential it always calls auth->ChallengeReceived before auth->GenerateCredential I agree that most auth schemes it would be sufficient to only call auth->ChallengeReceived to see if it would error out. In a lot of ways it is even a better design to only check that function. That being said, the main auth type I'm interested in is the GSSAPI and the gss_init_sec_context call will be called in generate credentials. gss_init_sec_context will error out if it cannot find a key for the server at which point I would not be able to fall back to other auth types. There are some workarounds though (I would try gss_init_sec_context in auth->ChallengeReceived just to check its error status, then though its output buffer away). Let me know what design you would prefer and I'll fix it up and resubmit it.

I just took a look at the patch and realized it was awful to read. Here it is again with more lines of context.

oh, doh! that does make sense now. sorry, i completely misread the previous patch.

Comment on attachment 133729 [details] [diff] [review] same patch more context > nsHttpChannel::GetCredentials(const char *challenges, > PRBool proxyAuth, > nsAFlatCString &creds) > { >+ * At this point the code is really only useful for client side >+ * errors (it will not automatically fail over to do a different >+ * auth type if the server keeps rejecting what is being sent, even >+ * if a particular auth method only knows 1 thing, like a non-identity >+ * based authentication method) yeah, this is a limitation, but maybe such simplistic auth schemes could keep a pointer to the last challenge in the "session state" (which is stored in the auth cache per entry). this information could perhaps be used in ChallengeReceived to determine that calling GenerateCredentials would serve no purpose. although, i'm not sure if that would work well in the case of multiple parallel requests all getting challenged in the same way. hmm.. >+ rv = GenerateCredential(challenge.get(), scheme.get(), proxyAuth, creds, auth); I would call this GetCredentialsForChallenge() instead since the credentials may not need to be generated if they are taken from the cache. >+nsresult >+nsHttpChannel::GenerateCredential(const char *challenge, >+ const char *scheme, >+ PRBool proxyAuth, >+ nsAFlatCString &creds, >+ nsIHttpAuthenticator *auth >+ ) nit: fix indentation and parameter alignment. closing ")" should be on the same line as the last parameter. keep style consistent with the rest of the file... "when in rome, stay in rome" ;) >+nsHttpChannel::ParseChallenge(const char *challenge, > nsCString &scheme, > nsIHttpAuthenticator **auth) > { >+ const char * p = NULL; >+ LOG(("nsHttpChannel::ParseChallenge [this=%x]\n", this)); > >+ // get the challenge type >+ if ((p = PL_strchr(challenge, ' ')) != nsnull) >+ scheme.Assign(challenge, p - challenge); >+ else >+ scheme.Assign(challenge); > >+ // normalize to lowercase >+ ToLowerCase(scheme); nit: fix indentation to be 4 blank spaces. r=darin with these changes.

Thanks Darin!

Comment on attachment 133760 [details] [diff] [review] Updated per comments one final nit: can you use strchr in place of PL_strchr. i realize that the old code was calling PL_strchr also... but while your touching this code! ;-)

i will add this to 1.6 beta.

I'll update the patch to use strchr tomorrow. What are the chances of also getting it into the 1.4.x branch?

Christopher: well, let's see ... i've set the blocking 1.4.2 flag. You should add a comment here explaining why you feel this should be incluced in 1.4.2. If you provide a convincing enough argument, I'd expect drivers to approve the request.

Requesting checking into 1.4 branch Risk Very Low: The code is mainly re-arranged from the original, not much new added. In addition, it makes no changes which would break included auth or 3rd party auth plugins. This patch makes Auth types like NTLM (or custom auth types like the Negotiate method) much more usable by allowing malfunctioning plugins or auth plugins which cannot currently be used (ie. kerberos when the user doesn't have a credential cache) be skipped. Currently, if an authtype like NTLM fails after loading because of a DLL problem, no other authentication types could be supported by Mozilla as a backup. In most situations using new auth types like Kerberos just isn't feasable without the failover support provided by this patch.

I just noticed this bug, I've been following 17578 for a while, hoping to see GSSAPI authentication integrated at some point. I have updated the fixes for 17578 to work with MS IIS Integrated Windows Authentication and tested it successfully. I'd like to coordinate with someone about getting these patches integrated into the official builds. Also, on the subject of authentication methods: * KerberosV4 is useless, it has lots of weaknesses and is generally considered way out of date. * KerberosV5 is far more interesting, but mostly only when used with GSSAPI. GSSAPI is a standard interface, the Kerberos V5 APIs are NOT. MIT KRB5 does things one way, Heimdal does them another, etc etc. Trying to write a portable application that uses the KRB5 APIs directly is a waste of time. Use GSS-API, it was created to solve this very problem, plus it's pluggable and generally easy to port across platforms. * GENERIC_GSSAPI is meaningless without the underlying mechanism support. However, all GSSAPI implementations that I know of at least have KRB5 as the basic mechanism, so supporting GSSAPI is a very good idea especially if secure single-sign on for HTTP is of value to you. * SASL is not terribly interesting. Microsoft has already standardized on a GSSAPI for their secure authentication, why re-invent the wheel again. Especially, if the end result looks like this: SASL -> GSSAPI -> KerberosV5. Just drop the SASL and use GSSAPI directly. * NTLM is interesting when interacting with MS, they accept NTLM directly and NTLM inside of GSSAPI tokens.

Comment on attachment 134265 [details] [diff] [review] Updated with PL_strchr->strchr >diff -ru mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp Argh. What happened to that promised context? :( "diff -p -u6" should be the minimal amount of context you ask for review on... >+ // figure out which challenge we can handle and which authenticator to use. >+ for (const char *eol = challenges - 1; eol; ) { This is mis-indented. It looks like you need to outdent the whole loop a space. Or are there tabs involved? In any case, fix this. Also, you lost the "LF separated" comment when you moved this code. Was that on purpose? If not, put it back, please. You never init rv before the loop, and you return rv after the loop. I guess we can assert that challenges != 1 or something? In any case, this will cause a compiler warning... You may want to init rv to some failure value (NS_ERROR_NOT_AVAILABLE?). >+nsHttpChannel::GetCredentialsForChallenge(const char *challenge, >+ const char *scheme, >+ PRBool proxyAuth, >+ nsAFlatCString &creds, >+ nsIHttpAuthenticator *auth) Typical style is to put the out param last..... or maybe first, but not somewhere in the middle. >+nsHttpChannel::ParseChallenge(const char *challenge, > nsCString &scheme, > nsIHttpAuthenticator **auth) Fix the indentation here? >+ if (NS_SUCCEEDED(GetAuthenticator(scheme.get(), auth))) >+ return NS_OK; I know you just moved this code, but wouldn't return GetAuthenticator(scheme.get(), auth); make more sense? That'd propagate the actual error code, instead of making up NS_ERROR_FAILURE.... darin, your call here. >+ rv = ParseChallenge(challenge,unused1, getter_AddRefs(auth)); Space after comma, please >+++ mozilla_altered/netwerk/protocol/http/src/nsHttpChannel.h Tue Oct 21 >+ nsresult GetCredentialsForChallenge(const char *challenge, const char * scheme, PRBool proxyAuth, nsAFlatCString &creds, nsIHttpAuthenticator * auth); >+ nsresult ParseChallenge(const char *challenge, nsCString &scheme, nsIHttpAuthenticator **auth); Could you document these methods in the header? Maybe even using the javadoc syntax? That would make it possible to figure out what they do without wading through the code.... > nsresult GetAuthenticator(const char *scheme, nsIHttpAuthenticator **); > void ParseRealm(const char *challenge, nsACString &realm); For that matter, as long as you understand this code, you get brownie points for documenting these two as well... ;) sr=bzbarsky with those changes.

chris: i will take care of revising the patch per bz's review comments and then i will check it in. it will be included in mozilla 1.6 beta. as for the 1.4 branch, it still depends on what drivers have to say.

fixed-on-trunk (for 1.6 beta) bz: i'm filing a bug re: adding comments to all of the methods in nsHttpChannel.

here's the patch i ended up landing on the trunk. we will need to back-port this to the 1.4 branch.

Comment on attachment 134454 [details] [diff] [review] final trunk patch requesting 1.4 branch approval. please see comment #31.

Comment on attachment 134454 [details] [diff] [review] final trunk patch a=mkaply for 1.4.2 darin, please don't break anything when you create a 1.4.2 patch :)

fixed1.4.2

This was fixed on the 1.4-branch