debops / ansible-pki

Bootstrap and manage internal PKI, Certificate Authorities and OpenSSL/GnuTLS certificates
GNU General Public License v3.0
65 stars 29 forks source link

Use Jinja `default` filter and added FIXME notes. #55

Closed ypid closed 8 years ago

ypid commented 8 years ago

What do you mean about the fixmes?

drybjed commented 8 years ago

For the static paths for scripts, yeah, I suppose a separate variable with ansible_local.root.lib could be used instead.

The omit use was consulted with @jimi-c - I needed a random string not stored anywhere so that it's random on each Ansible run, but the same across different Ansible tasks. Using hashed omit sounds good for this purpose, and I assume that the functionality won't be changed for the time being.

ypid commented 8 years ago

I assume that the functionality won't be changed for the time being.

@drybjed I don’t like to assume when working with security related stuff. Why not use the more explicit:

- name: Expose host FQDN and session token in temporary variable
  set_fact:
    pki_fact_fqdn: '{{ pki_fqdn }}'
    pki_fact_session_token: '{{ 999999999999999 | random }}'
drybjed commented 8 years ago

@ypid You are right, I stand corrected. The random variable method should be less prone to issues, and a set_fact task is already present, so why not use that. Although, any ideas to make the result a little more random? What do you think about something like this:

pki_fact_session_token: '{{ ((999999999999999 | random) | string + ansible_date_time.iso8601_micro) | md5 }}'

That should probably add a little more randomness, although this combines data from Ansible Controller and the remote host... I would prefer only the Ansible Controller input in this case. Any ideas how to get something without much overhead?

ypid commented 8 years ago

I think that using something so predictable like the date and time is a bad idea. Piping that into a broken hash function like md5 does not look too good either. We can let the random filter generate a bigger random number, I have no problem with that :wink:. I will do that when you are ok with that. The random filter in set_task is run on the remote system? Can we maybe delegate that to the Ansible controller?

drybjed commented 8 years ago

OK, so let's drop the date then. All filters should be executed on Ansible Controller, so I think random is run there as well.

ypid commented 8 years ago

Should we keep the md5 filter in this case?

drybjed commented 8 years ago

I've used the md5 filter to normalize the output, since the string is passed as an environment variable. I think that hashing that just in case is fine, and since it's just a one-time token, md5 should be OK since it's very fast. The strength of the hash function shouldn't matter since it's basically one time pad - nothing to crack.

ypid commented 8 years ago

Yes. I agree. Now that only a random value is passed, there should be no problem.

ypid commented 8 years ago

Updated. @drybjed What is the reason to use when: pki_register_facts.changed | bool in https://github.com/debops/ansible-pki/blob/b1599a62665d291e0a9e3718c6eb2609c7e11b8d/tasks/main.yml#L401. Any reason why you did not use when: pki_register_facts|changed as documented?

drybjed commented 8 years ago

@ypid I guess that I didn't know this syntax. If it works, you can switch to it if you want.

ypid commented 8 years ago

Done :wink: