Re: NA - token = fatalf
On 02/12/2013 08:48 PM, Amos Jeffries wrote: On 13/02/2013 3:00 p.m., Alex Rousskov wrote: On 02/12/2013 06:41 PM, Amos Jeffries wrote: On 13/02/2013 11:33 a.m., Henrik Nordström wrote: Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. NTLM is slightly different and do not expect a blob. Simply NASPACEmessageNEWLINE where message may be blank. In the NA case, the helper returns something like NA NT_STATUS_NO_SUCH_USER *\n Is that a valid NA response? Looks like it should be correct. Yes ERR token=NT_STATUS_NO_SUCH_USER message=* The admin does not want to emit the token AFAICT. The admin wants NT_STATUS_NO_SUCH_USER to be the message. My understanding is that if they change the response to new ERR message=NT_STATUS_NO_SUCH_USER then a patched Squid will work, but I do not yet know whether they can change the helper. A bigger question, I think, is whether the quoted tokenless NA response used to work. If it used to work, we may have to preserve the old behavior even if this particular admin can change the helper script. I spy the HelperReply.cc parse() NA case is not converting the responses. AF is doing so, but not NA. Neither is the Negotiate switch case. This would seem to be the main bug. You may be right, but I cannot propose a fix because I do not know what the correct/supported format(s) are. Thus, I am focusing on preventing crashes and hoping that somebody who knows what old helpers produced can fix the parser to accommodate those cases and fix the wiki page. IIRC that was left because there was confusion over how to handle multi-word messages and optional token prefix. It feels like that confusion still exists, but I do not know enough to help, unfortunately. Questions are: do we assume that there is no token on NA and send the whole string as visible error message? The proposed patch will do that. I think that is a reasonable assumption going forward, but I do not know whether it is compatible with old helpers. Do we try to check the first word and see if it is possibly a token and use it as one when is found? How can we distinguish a token from a message when kv-pairs are not used? Or, always assume the first word is a token and send it as one? that would probably break NTLM so we would have to do it in the Negotiate swicth case. But would allow us to send the catenated token+message for error display in case we got it wrong and a FUD token in Negotiate should not hurt. ... and yes these are blind guess ideas. I have not investigated any side effects. I think there are at least two distinct areas here: 1) Old helpers (presumably not modifiable): We should do our best to support them. We need to know (and document) what their assumptions were to support them. I do not know that. 2) New helpers (presumably still modifiable): We should require kv-pair based format for tokens and messages for all new result codes. We may add new result codes to resolve ambiguous cases in (1), if any. My understanding is ERR is the only new result code right now. If the token is required for NA/ERR responses, then the helper is broken because it does not send one. If the token is not required, then Squid is broken because it requires one. Henrik says the token is not required [in Squid2]. It seems to be sending token NT_STATUS_NO_SUCH_USER. It is trying to send NT_STATUS_NO_SUCH_USER message. Henrick only said these two were valid: NA \n NA token message\n Henrik also said do not require any of them to be present but it is not clear to me whether NA foo\n means that foo is a token or message. Although I would go so far as to add this one is probably valid also: NA token\n or NA message\n ? Thank you, Alex.
Re: NA - token = fatalf
On 02/12/2013 03:33 PM, Henrik Nordström wrote: Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. It accepts NASPACENEWLINE as valid response. How will Squid-2 negotiate interpret the following? NASPACEsomething-without-spacesNEWLINE Will it assume that something-without-spaces is a blob? A message? Thank you, Alex.
Re: [PATCH] NA - token = fatalf
On 13/02/2013 3:31 p.m., Amos Jeffries wrote: On 13/02/2013 2:37 p.m., Alex Rousskov wrote: On 02/12/2013 03:33 PM, Henrik Nordström wrote: tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. Is the attached fix on the right track? It makes the token part of the helper response optional and, hence, removes the fatalf() message. No other changes were intended, but this trunk patch is untested. Thank you, Alex. See my other message.. Yes your patch looks reasonable design for removal of the *specific* fatal() on the ERR/NA switch case, but only because the normal handling of that case is auth failure and cleanup. The other fatal()'s on success should be removed as well and cannot ignore the token= or user= absence. ** Like Henrik said the message= portion may be empty or missing. If token is missing it is much more likely that message is also missing, otherwise on NA the first word of the message would very likely be wrongly mapped as the token. So the messageNote dereference will need to be protected as well in this change. Amos Alternative applied to trunk as rev.12686 (and 12687) Amos
NA - token = fatalf
Hello,
I got a complaint from an authentication helper author because his
helper crashes Squid with a NA NT_STATUS_NO_SUCH_USER *\n response
that lack a token field or kv-pair. The corresponding fatalf() is in
auth/negotiate/UserRequest.cc:
case HelperReply::Error: {
Note::Pointer messageNote = reply.notes.find(message);
Note::Pointer tokenNote = reply.notes.find(token);
if (tokenNote == NULL) {
/* protocol error */
fatalf(authenticateNegotiateHandleReply: *** Unsupported helper
response ***, '%s'\n, reply.other().content());
break;
}
I tried to understand whether this fatalf() is intentional but failed.
As far as I can tell, the helper documentation on wiki says these
self-contradictory things:
NA:Deprecated by ERR result from Squid-3.4 onwards.
token: Negotiate authenticator interface requires it on NA responses.
token: This field must not be sent on ERR responses.
token=...: This field is only used on OK responses.
The trunk code appears to say these things:
* ERR and NA results are treated the same way
* ERR and NA negotiate results require a token kv pair
Could somebody with better authentication and helper knowledge clarify
whether the token field is indeed required for Nagotiate ERR and NA
responses? If not, can we just remove the above quoted fatalf() blob and
make the following line conditional on the token presence?
lm_request-server_blob = xstrdup(tokenNote-firstValue());
Thank you,
Alex.
Re: NA - token = fatalf
tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Hello, Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. It accepts NASPACENEWLINE as valid response. NTLM is slightly different and do not expect a blob. Simply NASPACEmessageNEWLINE where message may be blank. Regards Henrik
[PATCH] NA - token = fatalf
On 02/12/2013 03:33 PM, Henrik Nordström wrote:
tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov:
Could somebody with better authentication and helper knowledge clarify
whether the token field is indeed required for Nagotiate ERR and NA
responses? If not, can we just remove the above quoted fatalf() blob and
make the following line conditional on the token presence?
Squid-2 negotiate expects
NASPACEblobSPACEmessageNEWLINE
but do not require any of them to be present.
Is the attached fix on the right track? It makes the token part of the
helper response optional and, hence, removes the fatalf() message. No
other changes were intended, but this trunk patch is untested.
Thank you,
Alex.
Do not FATAL and quit when handling an NA or ERR negotiate helper response
without a challenge token.
=== modified file 'src/auth/negotiate/UserRequest.cc'
--- src/auth/negotiate/UserRequest.cc 2013-01-28 16:56:05 +
+++ src/auth/negotiate/UserRequest.cc 2013-02-13 00:30:07 +
@@ -310,53 +310,51 @@
* challenge-response nature of the protocol.
* Just free the temporary auth_user after merging as
* much of it new state into the existing one as possible */
usernamehash-user()-absorb(local_auth_user);
/* from here on we are working with the original cached credentials. */
local_auth_user = usernamehash-user();
auth_user_request-user(local_auth_user);
} else {
/* store user in hash's */
local_auth_user-addToNameCache();
}
/* set these to now because this is either a new login from an
* existing user or a new user */
local_auth_user-expiretime = current_time.tv_sec;
auth_user_request-user()-credentials(Auth::Ok);
debugs(29, 4, HERE Successfully validated user via Negotiate. Username ' auth_user_request-user()-username() ');
}
break;
case HelperReply::Error: {
-Note::Pointer messageNote = reply.notes.find(message);
-Note::Pointer tokenNote = reply.notes.find(token);
-if (tokenNote == NULL) {
-/* protocol error */
-fatalf(authenticateNegotiateHandleReply: *** Unsupported helper response ***, '%s'\n, reply.other().content());
-break;
-}
-
/* authentication failure (wrong password, etc.) */
+
+Note::Pointer messageNote = reply.notes.find(message);
auth_user_request-denyMessage(messageNote-firstValue());
auth_user_request-user()-credentials(Auth::Failed);
+
safe_free(lm_request-server_blob);
-lm_request-server_blob = xstrdup(tokenNote-firstValue());
+Note::Pointer tokenNote = reply.notes.find(token);
+if (tokenNote != NULL)
+lm_request-server_blob = xstrdup(tokenNote-firstValue());
+
lm_request-releaseAuthServer();
debugs(29, 4, HERE Failed validating user via Negotiate. Error returned ' reply ');
}
break;
case HelperReply::Unknown:
debugs(29, DBG_IMPORTANT, ERROR: Negotiate Authentication Helper ' reply.whichServer ' crashed!.);
/* continue to the next case */
case HelperReply::BrokenHelper: {
/* TODO kick off a refresh process. This can occur after a YR or after
* a KK. If after a YR release the helper and resubmit the request via
* Authenticate Negotiate start.
* If after a KK deny the user's request w/ 407 and mark the helper as
* Needing YR. */
Note::Pointer errNote = reply.notes.find(message);
if (reply.result == HelperReply::Unknown)
auth_user_request-denyMessage(Internal Error);
else if (errNote != NULL)
auth_user_request-denyMessage(errNote-firstValue());
Re: NA - token = fatalf
On 13/02/2013 11:33 a.m., Henrik Nordström wrote: tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Hello, Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. It accepts NASPACENEWLINE as valid response. NTLM is slightly different and do not expect a blob. Simply NASPACEmessageNEWLINE where message may be blank. Regards Henrik Squid-3 should be identical. The token is required for Authenticate-Info to supply client with keytab identification in the reponse headers. A missing token= on the Negotiate response normally indicates that an NTLM helper has been wrongly configured on the Negotiate auth interface. Markus' negotiate_wrapper helper presents a dummy token when mapping NTLM responses to Squid. Yes you can remove these fatal() if you want, but it needs to result in authentication failure and squid.conf ERROR messages if you do so. The code for triggering all that side-effect is in the BrokenHelper use case which might need to become a separate error handling method. This also goes for the other identical fatal(Unsupported helper response) in Negotiate auth which would be worth removing in the same way. Amos
Re: NA - token = fatalf
On 02/12/2013 06:41 PM, Amos Jeffries wrote: On 13/02/2013 11:33 a.m., Henrik Nordström wrote: tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Hello, Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. It accepts NASPACENEWLINE as valid response. NTLM is slightly different and do not expect a blob. Simply NASPACEmessageNEWLINE where message may be blank. Regards Henrik Squid-3 should be identical. The token is required for Authenticate-Info to supply client with keytab identification in the reponse headers. Can you clarify (for those who do not know enough about authentication) whether the token is required only if the admin wants some optional functionality to work? Like some optional header to be sent by Squid to the user? Or is it required for the error page to be reliably displayed to the authenticating user at all? My understanding is that the admin does not want authentication to continue in this NA/ERR case. A missing token= on the Negotiate response normally indicates that an NTLM helper has been wrongly configured on the Negotiate auth interface. Markus' negotiate_wrapper helper presents a dummy token when mapping NTLM responses to Squid. I do not know whether this particular helper is misconfigured. And I cannot tell by reading the wiki page (for the reasons discussed earlier). I believe the helper works OK otherwise. In the NA case, the helper returns something like NA NT_STATUS_NO_SUCH_USER *\n Is that a valid NA response? If the token is required for NA/ERR responses, then the helper is broken because it does not send one. If the token is not required, then Squid is broken because it requires one. Henrik says the token is not required [in Squid2]. Yes you can remove these fatal() if you want, but it needs to result in authentication failure Will removal of fatal(), as in the posted patch, result in authentication failure? I assume it will, but, again, I am not an authentication expert. and squid.conf ERROR messages if you do so. Do you mean squid.cache ERROR message? At level 1? Does that imply that token is required? I doubt the admin would want tons of ERROR messages in the cache.log so they would probably have to fix their helper instead. But we should not force them to fix the helper if there is nothing wrong with it. The code for triggering all that side-effect is in the BrokenHelper use case which might need to become a separate error handling method. This also goes for the other identical fatal(Unsupported helper response) in Negotiate auth which would be worth removing in the same way. Tthis seems to imply that the helper is broken because the token should be required in all NA/ERR responses. Is that correct? Thank you, Alex.
Re: NA - token = fatalf
On 13/02/2013 3:00 p.m., Alex Rousskov wrote: On 02/12/2013 06:41 PM, Amos Jeffries wrote: On 13/02/2013 11:33 a.m., Henrik Nordström wrote: tis 2013-02-12 klockan 14:41 -0700 skrev Alex Rousskov: Hello, Could somebody with better authentication and helper knowledge clarify whether the token field is indeed required for Nagotiate ERR and NA responses? If not, can we just remove the above quoted fatalf() blob and make the following line conditional on the token presence? Squid-2 negotiate expects NASPACEblobSPACEmessageNEWLINE but do not require any of them to be present. It accepts NASPACENEWLINE as valid response. NTLM is slightly different and do not expect a blob. Simply NASPACEmessageNEWLINE where message may be blank. Regards Henrik Squid-3 should be identical. The token is required for Authenticate-Info to supply client with keytab identification in the reponse headers. Can you clarify (for those who do not know enough about authentication) whether the token is required only if the admin wants some optional functionality to work? Like some optional header to be sent by Squid to the user? Or is it required for the error page to be reliably displayed to the authenticating user at all? My understanding is that the admin does not want authentication to continue in this NA/ERR case. I'm not sure exactly what it is used for on failures, or if we need to send it at all. Care to experiment? From that and the snippet of RFC 4559 If a 401 containing a WWW-Authenticate header with Negotiate and gssapi-data is returned from the server, it is a continuation of the authentication request. It does not make any separation between challenge responses or re-try authenticate states (NA with a token). I am guessing that it could be used to negotiate acceptible GSSAPI sub-protocols when validation failure occurs. Or re-start of the authentication session if nonce sent by the client is broken. A missing token= on the Negotiate response normally indicates that an NTLM helper has been wrongly configured on the Negotiate auth interface. Markus' negotiate_wrapper helper presents a dummy token when mapping NTLM responses to Squid. I do not know whether this particular helper is misconfigured. And I cannot tell by reading the wiki page (for the reasons discussed earlier). I believe the helper works OK otherwise. In the NA case, the helper returns something like NA NT_STATUS_NO_SUCH_USER *\n Is that a valid NA response? Looks like it should be correct. Yes ERR token=NT_STATUS_NO_SUCH_USER message=* I spy the HelperReply.cc parse() NA case is not converting the responses. AF is doing so, but not NA. Neither is the Negotiate switch case. This would seem to be the main bug. IIRC that was left because there was confusion over how to handle multi-word messages and optional token prefix. Questions are: do we assume that there is no token on NA and send the whole string as visible error message? The proposed patch will do that. Do we try to check the first word and see if it is possibly a token and use it as one when is found? Or, always assume the first word is a token and send it as one? that would probably break NTLM so we would have to do it in the Negotiate swicth case. But would allow us to send the catenated token+message for error display in case we got it wrong and a FUD token in Negotiate should not hurt. ... and yes these are blind guess ideas. I have not investigated any side effects. If the token is required for NA/ERR responses, then the helper is broken because it does not send one. If the token is not required, then Squid is broken because it requires one. Henrik says the token is not required [in Squid2]. It seems to be sending token NT_STATUS_NO_SUCH_USER. Henrick only said these two were valid: NA \n NA token message\n Although I would go so far as to add this one is probably valid also: NA token\n Yes you can remove these fatal() if you want, but it needs to result in authentication failure Will removal of fatal(), as in the posted patch, result in authentication failure? I assume it will, but, again, I am not an authentication expert. As done yes it should. and squid.conf ERROR messages if you do so. Do you mean squid.cache ERROR message? At level 1? Does that imply that token is required? I doubt the admin would want tons of ERROR messages in the cache.log so they would probably have to fix their helper instead. But we should not force them to fix the helper if there is nothing wrong with it. Nix that now. The code for triggering all that side-effect is in the BrokenHelper use case which might need to become a separate error handling method. This also goes for the other identical fatal(Unsupported helper response) in Negotiate auth which would be worth removing in the same way. Tthis seems to imply that the helper is broken because the token should be required in all NA/ERR responses. Is that correct?
Re: NA - token = fatalf
ons 2013-02-13 klockan 14:41 +1300 skrev Amos Jeffries: Yes you can remove these fatal() if you want, but it needs to result in authentication failure NA is authentication failure. Must not result in anything else. Regards Henrik
