Re: [Postfixadmin-devel] SF.net SVN: postfixadmin:[560] trunk

[Jan R. <ja...@ro...>]

> Minor issue first: Postfixadmin uses 3 space characters per indention 
> level for historical reasons. No tabs please.
> (I did the whitespace fixes of your commit in SVN r561.)
> If you use vim as editor, the vim: line in each file should setup this 
> automatically (except if you have disabled modelines - which is the 
> case on most newer installations because modelines can be security 
> relevant somehow [don't ask for the details]).

I'm using eclipse. Ok, now I configured to use 3 spaces instead of tabs. 
Hope that works :-)
But as David mentioned recently. The whole file functions.inc.php uses 4 
spaces and so I continued using 4 spaces for now.

> Is there a special reason to always use the same static salt? Otherwise, 
> I'd prefer a random salt for security reasons.

Good point. I didn't thought that much about the crypt-flavor because 
I'm using unsalted md5s in my environment. I changed that to use a 
random salt in SVN r562.

> Also the PHP documentation about crypt() says that in some cases 
> (depending on the encryption method) the salt should be longer than two 
> characters.

Yes, but courier-authlib only supports two-character salts.

>> +      	if(stripos($flavor, 'md5raw') === 0) {
>> +         	$password = '{' . $flavor . '}' . md5($pw);
>> +      	} else if(stripos($flavor, 'md5') === 0) {
> 
> Why "else if" instead of "elseif"?
> 
>> +      		$password = '{' . $flavor . '}' . base64_encode(md5($pw,
>>                                                         TRUE)); 
>> +      	} else if(stripos($flavor, 'crypt') === 0) { 
> 
> Same question about "else if" again ;-)

Shouldn't else if and elseif be the same when you use curly braces? -> 
http://de.php.net/manual/en/control-structures.elseif.php

> Oh, and what happens if someone sets something like
>     $CONF['authlib_default_flavour'] = 'this-is-not-supported';
> (or just a typo like 'md6')?
> 
> I'd expect an "else" with a clear error message for this case...
> (Feel free to use die() since this is a configuration error.)

Another good point :-) I fixed this in SVN r562 too.

Greetings Jan