GNOME Bugzilla – Bug 783781
[Negotiate] Authentication should success in some cases when gss_init_sec_context() returns error
Last modified: 2017-06-22 13:55:58 UTC
Unfortunately, so many programs (curl, Firefox) ignore the return token that is included in the response, so it is possible that there are servers that send back broken stuff. Try to behave in the right way (pass the token to gss_init_sec_context()), show a warning, but don't fail if the server returned 200. There is an internal Red Hat site that triggers the described situation and the "Invalid token was supplied: Unknown error" is being printed to the console. This change was suggested by Simo Sorce.
Created attachment 353737 [details] [review] Proposed patch
Comment on attachment 353737 [details] [review] Proposed patch ok (but maybe reorganize everything between soup_gss_client_step() and "out:" into a "switch (ret) { ..." ?)
(In reply to Dan Winship from comment #2) > ok (but maybe reorganize everything between soup_gss_client_step() and > "out:" into a "switch (ret) { ..." ?) Thank you for the review Dan. I'm attaching an updated patch that I will commit once the blocked bug will be committed.
Created attachment 354043 [details] [review] Proposed patch 2
Comment on attachment 354043 [details] [review] Proposed patch 2 >+ if (!soup_gss_build_response (conn, SOUP_AUTH (auth), &err)) { >+ /* FIXME: report further upward via (this patch currently has the other bug's patch in it too but I assume that will get fixed) >+ switch (ret) { >+ case AUTH_GSS_COMPLETE: >+ priv->is_authenticated = ret == AUTH_GSS_COMPLETE; aka: priv->is_authenticated = TRUE >+ case AUTH_GSS_CONTINUE: > conn->state = SOUP_NEGOTIATE_RECEIVED_CHALLENGE; >+ break; do you need to set priv->is_authenticated to FALSE in any of these other cases?
(In reply to Dan Winship from comment #5) > Comment on attachment 354043 [details] [review] [review] > Proposed patch 2 > > >+ if (!soup_gss_build_response (conn, SOUP_AUTH (auth), &err)) { > >+ /* FIXME: report further upward via > > (this patch currently has the other bug's patch in it too but I assume that > will get fixed) Yes, it got mixed up. I will attach an updated version. > >+ switch (ret) { > >+ case AUTH_GSS_COMPLETE: > >+ priv->is_authenticated = ret == AUTH_GSS_COMPLETE; > > aka: priv->is_authenticated = TRUE Obviously. > >+ case AUTH_GSS_CONTINUE: > > conn->state = SOUP_NEGOTIATE_RECEIVED_CHALLENGE; > >+ break; > > do you need to set priv->is_authenticated to FALSE in any of these other > cases? Yes. I will initialize priv->is_authenticated to FALSE.
Created attachment 354046 [details] [review] Proposed patch 3
Comment on attachment 354046 [details] [review] Proposed patch 3 oh, you don't have to set it at init time. priv will always be initialized to all-bits-0, so is_authenticated will start out as FALSE. I was just wondering if there were situations where it would get set TRUE in one round but then need to get set back to FALSE later
(In reply to Dan Winship from comment #8) > Comment on attachment 354046 [details] [review] [review] > Proposed patch 3 > > oh, you don't have to set it at init time. priv will always be initialized > to all-bits-0, so is_authenticated will start out as FALSE. I was just > wondering if there were situations where it would get set TRUE in one round > but then need to get set back to FALSE later I don't think that there will be situations like that. Once it will be set to TRUE, then this code won't be called again as there are checks in SoupAuth whether the auth is already authenticated.
Created attachment 354050 [details] [review] Proposed patch 4 Remove unnecessary initialization of is_authenticated.
(In reply to Tomas Popela from comment #10) > Created attachment 354050 [details] [review] [review] > Proposed patch 4 > > Remove unnecessary initialization of is_authenticated. Wrong patch attached..
Created attachment 354051 [details] [review] Proposed patch 5
Fixed with commit aefe8eac in the master branch.