fixes apache::htpasswd

[savar]

encrypted passwords (crypt_password) were encrypted again
whereas unencrypted passwords (plain_password) weren't
encrypted at all

[savar]

the travis is not failing because of the code change

[alvagante]

I'm a bit concerned on the effect of the change on existing setups.
@israelriibeiro any remark on this?

[savar]

I tried to "tricks" around by using the "clear_password" but due to the missing single quotes in the command this didn't help and I got shell errors..

I have no Idea how to deal with this regarding backwards compatibility..

[israelriibeiro]

Hi, @savar and @alvagante , i changed this because, when i was testing nagios module, i didn't encryted the password. And i looked manual:

-p Use plaintext passwords. Though htpasswd will support creation on all platforms, the httpd daemon will only accept plain text pass‐ words on Windows, Netware and TPF.

Could you show, @savar , how you tested this?

[savar]

It's confusing with "crypt_password" and "clear_password". I tried to store an already encrypted password. So

$password = '$apr1$Q934vWD/vbvri$asuywqekvcidsasdq./qweqw.fasFFAs'

apache::htpasswd { 'xxxx':
  clear_password => $password, # this gave me a syntax error while executing the "exec" .. probably because of the missing single quotes around the password
}

apache::htpasswd { 'yyy':
  crypt_password => $password, # this was encrypt the already encrypted password
}

So maybe the only necessary change would be the single quotes around the password in line 88.

But it's still confusing what the clear_password/crypt_password means.. (also the doc is not really right.. it explains for "crypt_password": Crypted password .. which is not true.. it will be encrypted but you have to put in the clear text password).

So in my opinion it would be better to have something like:

apache::htpasswd { 'zzz':
   password         => $password,
   encrypt_password => false|md5|bcrypt(?|sha),
}

[alvagante]

Yes, you suggested params make more sense, but I think they would be acceptable in a future major release. For the current one, I'd rather document better the parameters and show some sample code.

[savar]

with my commit 0dc7a99 it is now working for me as well.. and it should be 99,9% backward compatible.. the only thing which won't work anymore is something like that:

  apache::htpasswd { 'testuser':
    clear_password => "somebad\\'thing",
    htpasswd_file  => $htpasswd_file,
  }

[savar]

Here is the error why I started the pull request (only one line of output but you where seeing the full "help" from the htpasswd command):

Notice: /Stage[main]/Role::Nmsserver/Apache::Htpasswd[someuser]/Exec[test -f /etc/apache2/some.passwd || OPT='-c'; htpasswd -bp $OPT /etc/apache2/some.passwd someuser $apr1$XrpakcPm$FhZVRp20ZKhnUCN1FdVc2f]/returns: Usage:

[savar]

I realize now that I probably could try to escape my "$" inside the password to not change it in any way.. if you think it is not worth to break backward compatibility right now I could try that as well..

[savar]

So it works for me now with escaping the $ characters ... so I close this by now.. but I hope it will be changed (and made more clear) in the future..

Sorry if I wasted your time..

[liquorvicar]

I've come across this issue and I agree with @savar here. From my experience the last commit in the htpasswd.pp manifest (22304f7) has broken BC and works differently from how you would expect by reading the current docs.

So I was using this module and I was passing a plain-text (unencrypted) password into the class using the $clear_password parameter. This was being encrypted and added to the specified .htpasswd file as expected. After an update to the module this behaviour was now breaking and my plain-text password was being added to the .htpasswd file unencrypted.

My man entry for htpasswd specifies the following, which I think makes things a little clearer:

-p Do not encrypt the password (plaintext, insecure).
So if we want whatever is specified by the user to be stored as is in the .htpasswd file then we need to use the '-p' switch.

From the manifest's usage docs, it's clear to me that if you want to pre-encrypt the password (and hence want the value you pass in to be stored as is) then you need to use $crypt_password :

Set crypted password to user 'my_user' on custom htpasswd file:
apache::htpasswd { 'myuser':
crypt_password => 'B5dPQYYjf.jjA',
htpasswd_file => '/etc/httpd/users.passwd',
}

If there are OSes where httpd can only accept plain-text passwords then these values need to be passed in using the $crypt_password option.

Yes, this is odd behaviour and I can see where @israelriibeiro might have got confused by this but, for me, this is a better workaround then changing the behaviour and breaking other people's implementations!

I like @savar suggestion for the future API and can see why it should wait for a future version (although could we introduce it now alongside the existing options which could be deprecated?) but can we revert this BC-breaking change for now (and maybe update the docs to make them clearer?). I'm happy to pick up where @savar got to and create a new PR if necessary.

[israelriibeiro]

@liquorvicar , i changed this cause, when i was testing nagios, it didn't encrypt password, but it should do it:

https://github.com/example42/puppet-nagios/blob/master/manifests/prerequisites.pp

Your manual and mine say the same thing: if you want to use plain text, or clear_password, you have to put -p, that's why i changed it. But i understood your problem, i did misspeak here, when i wrote:

https://github.com/example42/puppet-apache/blob/master/manifests/htpasswd.pp#L37

I should have said: set your password to be encrypted.

@alvagante , can i just change this in order to correct the problem i created?

Att,

Israel Ribeiro

[liquorvicar]

@israelriibeiro I'm sorry your change has broken BC on this module and (IMHO) it needs to be reversed. There is a lot of confusion as the original manifest/docs are not clear at all but I see it basically like this:

• $crypt_password is for passwords that are either pre-encrypted or do not need encrypting. Thus when this value is specified we need to add the -p switch to htpasswd to make sure the value does not get encrypted by htpasswd. It's not 100% obvious but the line in the docs which I quoted above where the value passed to $crypt_password is obviously pre-encrypted implies that this is the original intention of the manifest.
• $clear_password is for passwords that need encrypting, hence we must omit the -p flag so htpasswd can do it's job.
  If your nagios password needs encrypting, then you must use the $clear_password parameter to ensure that the '-p' flag is not added and hence it is encrypted by htpasswd.

I have done a PR to reverse the change, including changing the docs for the manifest to make it clearer.

Either way, as I said before, I support the idea to refactor this as per @savar's suggestion. Is there a plan for a new version of the module which we can target with that change?

[alvagante]

@israelriibeiro yes, please, submit a PR in line with was has been discussed here, thank you

[alvagante]

Actually @liquorvicar already submitted a PR:
#87
@israelriibeiro does this look ok for you
(sorry I've no time to test this directly and fully now)

[tuxmea]

ping
@israelriibeiro please chekc @alvagante last comment and check whether #85 is OK for you

[israelriibeiro]

In fact, it is. I'm sorry for the mess, i completly forgot about this.

[savar]

forgot about that.. so it looks it is not required anymore, right?