MD5 compatibility fixes (was: [SCM] Samba Shared Repository - branch master updated)

[Alexander Bokovoy]

On Sat, Dec 8, 2012 at 5:27 PM, Jelmer Vernooij <jelmer at samba.org> wrote:
> On Sat, Dec 08, 2012 at 05:17:17PM +0200, Alexander Bokovoy wrote:
>> On Sat, Dec 8, 2012 at 5:12 PM, Jelmer Vernooij <jelmer at samba.org> wrote:
>> > On Sat, Dec 08, 2012 at 04:55:23PM +0200, Alexander Bokovoy wrote:
>> >> On Sat, Dec 8, 2012 at 2:44 PM, Jelmer Vernooij <jelmer at samba.org> wrote:
>> >> > On Sat, Dec 08, 2012 at 01:31:03PM +0100, Andreas Schneider wrote:
>> >> >> Changeset truncated at 500 lines:
>> >> >>
>> >> >> diff --git a/buildtools/wafsamba/samba_utils.py b/buildtools/wafsamba/samba_utils.py
>> >> >> index c1869df..cab87a4 100644
>> >> >> --- a/buildtools/wafsamba/samba_utils.py
>> >> >> +++ b/buildtools/wafsamba/samba_utils.py
>> >> >> @@ -388,9 +388,17 @@ def RUN_COMMAND(cmd,
>> >> >>  # make sure we have md5. some systems don't have it
>> >> >>  try:
>> >> >>      from hashlib import md5
>> >> >> +    try:
>> >> >> +        foo = md5.md5('abcd')
>> >> >> +    except ValueError:
>> >> >> +        raise
>> >> > This looks really odd - what does the 'except ValueError: ; raise' bit do?
>> >> > Wouldn't the behaviour of this code be the same without it?
>> >> No, it is not.
>> >>
>> >> FIPS mode is preventing certain (outdated) cryptographic operations to
>> >> be used. When operating system runs in FIPS mode all access to actual
>> >> banned crypto algorithms will cause failures.
>> >>
>> >> In FIPS mode Python md5 module can be imported without any issues and
>> >> ValueError exception will be raised when actually attempting to call
>> >> to md5 function. Thus, we would never see that the code would actually
>> >> be unusable with existing check.
>> >>
>> >> This leads to sudden failure of configure stage later because waf uses
>> >> hash function (md5 by default) to notice differences between objects'
>> >> states. The hash calculation is done deep in waf code causes unhandled
>> >> exception in FIPS mode and that one causes early exit from
>> >> run_c_code() routine, leaving unbalanced "enter directory - leave
>> >> directory" pairs. The latter causes sudden removal of the temporary
>> >> directory while it is in use by the configure check which, in turn,
>> >> causes os.getcwd('.') to throw exception (since current directory was
>> >> just deleted) and abort the execution.
>> > That bit seems reasonable; I'm curious about the re-raising of the
>> > exception.
>> >
>> >> The way I've coded this fix we are re-using existing infra in wafsamba
>> >> to replace hash function. I'm re-raising exception that FIPS mode may
>> >> raise so that we are falling through to actual replacement code.
>> > What is the point of re-raising the exception? This:
>> >
>> > try:
>> >         x = md5.md5('foo')
>> > except ValueError:
>> >         raise
>> >
>> > should be equivalent to this:
>> >
>> > x = md5.md5('foo')
>> >
>> > If not, can you explain what the difference is?
>> The latter will trigger for all kinds of exceptions while the former
>> is only covering ValueError ones.
> But then you just reraise the exception, which results in the same behaviour
> as the default behaviour for exceptions that aren't caught.
>
> ValueError will be treated exactly same way as KeyError, ImportError, etc.
> and just be propagated up.
>
>> Perhaps you are right in that if we ever trigger any exception
>> accessing md5.md5() we would need to force replacement route
>> unconditionally. I wanted to codify explicit error so that we don't
>> forget why we are doing all this.
> A comment explaining what we are looking for and why would be better
> in that case. I at least didn't understand the intent of this code
> (hence my mail :-).
Right. I've pushed to autobuild a simple patch that removes inner
try:except: pair and adds a comment there.
Once it goes through, I'll add the patch to the bug we have for 4.0
for this one.
-- 
/ Alexander Bokovoy