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

[Christian B. <pos...@cb...>]

Hello,

Am Mittwoch, 4. Februar 2009 schrieb Jan Röhrich:
> > 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.

Indeed, functions.inc.php uses 4 spaces. I just fixed the vim: comment - 
one file less to migrate from the historical 3 spaces ;-)
(If you find other files that use 4 spaces and have a vim: comment 
saying 3, feel free to fix the vim: comment.)

BTW: Your last commit added some tabs again - I replaced them with 
spaces.

> > 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.

Thanks!

> > 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.

Ah, OK. I added a comment about this to avoid similar questions in the 
future ;-)

> >> +      	if(stripos($flavor, 'md5raw') === 0) {
> >> +         	$password = '{' . $flavor . '}' . md5($pw);
> >> +      	} else if(stripos($flavor, 'md5') === 0) {
> >
> > Why "else if" instead of "elseif"?

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

Yes, they are the same. It's more a style question.

Since the two listed lines are the only ones in postfixadmin using "else 
if" instead of "elseif", I took the liberty to remove the space.

All these things are in SVN r564.

> > 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.

Thanks!

Another thing I just noticed:

        if(stripos($flavor, 'md5raw') === 0) {
            ...
        } elseif(stripos($flavor, 'md5') === 0) {
            ...
        } elseif(stripos($flavor, 'crypt') === 0) {
            ...

This code depends on the correct order (md5raw must be checked before 
md5) which of course works. However, it will also match something 
like "md52" as "md5".

I don't know how the field looks in courier-authlib style (can you post 
some examples?), but if there is a separator between the encryption 
method and the encrypted password, you should include the separator in 
the stripos() check.


Regards,

Christian Boltz
-- 
Graphisch??? Wie meinen? Hast du zuviel Fleisch von zu "gluecklichen"
Rindern gefuttert? *scnr*  Wozu zum Henker sollte man sowas brauchen?
Logo ginge auch per ASCII :)  (Logo?  welches Logo? Wozu ueberhaupt?)
[David Haller in suse-linux]