Bug 10692 - wbcCredentialCache fails if challenge_blob is not first
Summary: wbcCredentialCache fails if challenge_blob is not first
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 4.1.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-07 20:33 UTC by David Woodhouse
Modified: 2014-07-17 18:42 UTC (History)
1 user (show)

See Also:


Attachments
git-am fix for master. (1.04 KB, patch)
2014-07-08 23:38 UTC, Jeremy Allison
no flags Details
git-am fix for 4.1.next/4.0.next (4.01 KB, patch)
2014-07-11 22:12 UTC, Jeremy Allison
no flags Details
patch for 4.1.next, 4.0.next (3.00 KB, patch)
2014-07-14 17:50 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Woodhouse 2014-07-07 20:33:01 UTC
In wbcCredentialCache() about line 1228 of wbc_pam.c:

	for (i=0; i<params->num_blobs; i++) {
		if (strcasecmp(params->blobs[i].name, "initial_blob") == 0) {
			initial_blob = &params->blobs[i];
			break;
		}
		if (strcasecmp(params->blobs[i].name, "challenge_blob") == 0) {
			challenge_blob = &params->blobs[i];
			break;
		}
	}

Don't break out of the loop as soon as you've found one.
Comment 1 Jeremy Allison 2014-07-08 23:38:00 UTC
Created attachment 10093 [details]
git-am fix for master.

Is this the fix you had in mind ?
Comment 2 David Woodhouse 2014-07-09 11:39:23 UTC
Yes, that fixes the issue here. Thanks.
Comment 3 Jeremy Allison 2014-07-11 22:12:40 UTC
Created attachment 10103 [details]
git-am fix for 4.1.next/4.0.next

Patchset that went into master. Applies cleanly to 4.1.next and 4.0.next.
Comment 4 David Woodhouse 2014-07-12 00:50:28 UTC
Hm, I'm not sure I like that last one ("reject unknown named blobs"). You've changed the backward compatibility story.

As things stand, if we add a new optional blob in future, clients can just provide it and know that old implementations of libwbclient will ignore it.
Now you've made sure they'll bail out when they see the new blob.

I was actually thinking (vaguely) of doing precisely that.  I've just hooked up Simo's GSS-NTLMSSP to use libwbclient (http://git.infradead.org/users/dwmw2/gss-ntlmssp.git) and it has some limitations like not allowing us to support the MIC or channel bindings... which may have been solved by allowing the client to pass the client_target_info in to wbcCredentialsCache() instead of letting winbind generate it for itself.

I hadn't really fully thought that plan through though; maybe it couldn't have worked anyway...
Comment 5 Stefan Metzmacher 2014-07-12 16:36:53 UTC
(In reply to comment #4)
> Hm, I'm not sure I like that last one ("reject unknown named blobs"). You've
> changed the backward compatibility story.
> 
> As things stand, if we add a new optional blob in future, clients can just
> provide it and know that old implementations of libwbclient will ignore it.
> Now you've made sure they'll bail out when they see the new blob.
> 
> I was actually thinking (vaguely) of doing precisely that.  I've just hooked up
> Simo's GSS-NTLMSSP to use libwbclient
> (http://git.infradead.org/users/dwmw2/gss-ntlmssp.git) and it has some
> limitations like not allowing us to support the MIC or channel bindings...
> which may have been solved by allowing the client to pass the
> client_target_info in to wbcCredentialsCache() instead of letting winbind
> generate it for itself.
> 
> I hadn't really fully thought that plan through though; maybe it couldn't have
> worked anyway...

Ok, Jeremy I guess it's better to revert the last patch in that case...
Comment 6 David Woodhouse 2014-07-12 17:05:22 UTC
(In reply to comment #5)
> Ok, Jeremy I guess it's better to revert the last patch in that case...

Well, I did say I hadn't really thought it through. In practice even if I do go with the idea of passing in the client_target_info, I'm probably still better off checking the libwbclient version *first* to see if it's supported, rather than just passing it in anyway and then looking at the response to see if winbind was new enough to do the right thing.

But that specific example aside, it's still a change in your backward compatibility story that needs to be done intentionally if at all.
Comment 7 Jeremy Allison 2014-07-14 16:29:49 UTC
I hadn't considered the backwards compatibility story either. I'll revert that last patch and upload a new version without it.

Thanks for the careful review David !
Comment 8 Jeremy Allison 2014-07-14 17:50:31 UTC
Created attachment 10107 [details]
patch for 4.1.next, 4.0.next

Removed last (reverted) component.
Comment 9 Karolin Seeger 2014-07-15 04:09:21 UTC
Pushed to autobuild-v4-1-test.
Comment 10 Karolin Seeger 2014-07-15 04:09:48 UTC
(In reply to comment #9)
> Pushed to autobuild-v4-1-test.

and autobuild-v4-0-test
Comment 11 Karolin Seeger 2014-07-17 18:42:29 UTC
Pushed to both branches.
Closing out bug report.

Thanks!