COmanage Developers List

[Scott Koranda <>]

> On 2/14/12 5:35 PM, Scott Koranda wrote:
> 
> >The JIRA issue is "Salts should be randomized on install":
> >
> >https://bugs.internet2.edu/jira/browse/CO-202
> >
> >Attached is a diff showing the implemenation.
> >
> >Please send me any comments and let me know if I can push the code.
> 
> str_shuffle, who knew?

An advantage of being a latecomer to PHP is that I have no old
PHP habits to overcome and get to look at the documentation
often.

> 
> OK, this is important, and probably inadvertent, but DON'T EVER
> COMMIT THIS CHANGE:
> 
> -     Configure::write('debug', 0);
> +     Configure::write('debug', 2);
> 
> Major security problems if any of us does. (I'm sure we all have
> this set locally in our working copies.)

Understood.

> 
> Also, your diff includes changes for Controller/AppController.php
> that I think you want to commit separately.

Yes, done.

> 
> +    if(file_exists($securitySaltFilename)){
> +      $handle = fopen($securitySaltFilename, "r");
> +      $saltLine = fgets($handle);
> +      fclose($handle);
> +
> +      $salt = trim($saltLine);
> +      if (strlen($salt) < 40){
> +        throw new ConfigureException("security salt must be 40 or
> more characters");
> +      }
> +      Configure::write('Security.salt', $salt);
> +    } else {
> +      Configure::write('Security.salt',
> 'DYhG93b0qyJfIxfs2guUoUubWwvniR2G0FgaC9mi');
> +    }
> 
> I think if !file_exists, we want to throw an exception, no? (ie: if
> somebody decides to throw away the salt file for some reason.)

No because core.php is processed even when running Setup or
Database so we have the bootstrap problem. Detecting that the
instance is a run of Setup seems more trouble then it is worth
and so I chose to use a default.

Instead I have added a line(s) so that if the file(s) is
missing a WARNING is logged.

> 
> Ditto for cipherSeed. Also, 29 != 40:

Got it. Thanks.

> 
> +      if (strlen($seed) < 29){
> +          throw new ConfigureException("security seed must be 40 or
> more digits");
> +      }
> 
> You could probably also simplify this a bit by using file_get_contents().
> 

I chose not to use file_get_contents so I did not have to
worry about dealing with multiple lines in a file and checking
to make sure there are no special characters (newlines). 

Thanks,

Scott