Project

General

Profile

Review #544

Install and configure ca-certificates

Added by Pierre-Louis Bonicoli over 2 years ago. Updated about 2 years ago.

Status:
Resolved
Priority:
Normal
Category:
-
Start date:
2017-05-16
Branch:
ca-certificates

Description

Please, could you review ca-certificates branch in admin repository ?


Related issues

Related to DuckCorp Infrastructure - Enhancement #533: Install a Jessie LXC container with systemd enabled in order to test/validate Burp setupResolved2017-05-09

Actions
Copied from DuckCorp Infrastructure - Review #543: Install apt-transport-https before updating cacheResolved2017-05-16Actions
Copied to DuckCorp Infrastructure - Review #545: s/ansible_hostname/inventory_hostname/Resolved2017-05-16Actions

History

#1

Updated by Pierre-Louis Bonicoli over 2 years ago

  • Copied from Review #543: Install apt-transport-https before updating cache added
#2

Updated by Pierre-Louis Bonicoli over 2 years ago

  • Branch set to ca-certificates
#3

Updated by Pierre-Louis Bonicoli over 2 years ago

  • Copied to Review #545: s/ansible_hostname/inventory_hostname/ added
#4

Updated by Marc Dequènes over 2 years ago

  • Status changed from New to In Progress
+  debconf: "name=ca-certificates setting={{ 'ca-certificates/' + item.0 }} vtype={{ item.1 }} value='{{ item.2 }}'" 

Use YAML structures please, my eyes are bleeding.

Historically I setup ca-certificates/enable_crts to an empty list, but several services communicate with the outside and I had to revert at least on some hosts.
So I wonder about trust_new_crts because this would mean we trust the list at the time of first installation and not the Debian updates. So to me either we trust all or nothing. Could you explain your rationale?

#5

Updated by Pierre-Louis Bonicoli over 2 years ago

Marc Dequènes wrote:

[...]
Use YAML structures please, my eyes are bleeding.

Done (branch updated)

Historically I setup ca-certificates/enable_crts to an empty list, but several services communicate with the outside and I had to revert at least on some hosts.
So I wonder about trust_new_crts because this would mean we trust the list at the time of first installation and not the Debian updates. So to me either we trust all or nothing. Could you explain your rationale?

Updated: I added a trust_distribution_crts: when trust_distribution_crts is set to no, enable_crts is set to an empty string (like new_crts).

#6

Updated by Marc Dequènes over 2 years ago

Pierre-Louis Bonicoli wrote:

Use YAML structures please, my eyes are bleeding.

Done (branch updated)

Sorry, I can't see any difference. Now there are two debconf actions with a string.

Updated: I added a trust_distribution_crts: when trust_distribution_crts is set to no, enable_crts is set to an empty string (like new_crts).

This is a very good idea.
Additionally I would prefer the variable to be a real boolean and to convert it to the format the underlying software expect. This would avoid having "true", "yes", True, 1, "1"… and provide a coherent API across all roles.

#7

Updated by Pierre-Louis Bonicoli about 2 years ago

Marc Dequènes wrote:

Pierre-Louis Bonicoli wrote:

Use YAML structures please, my eyes are bleeding.

Done (branch updated)

Sorry, I can't see any difference. Now there are two debconf actions with a string.

Oups, updated again.

Updated: I added a trust_distribution_crts: when trust_distribution_crts is set to no, enable_crts is set to an empty string (like new_crts).

This is a very good idea.
Additionally I would prefer the variable to be a real boolean and to convert it to the format the underlying software expect. This would avoid having "true", "yes", True, 1, "1"… and provide a coherent API across all roles.

What is a true boolean ? Is a YAML boolean a real one ?

#8

Updated by Marc Dequènes about 2 years ago

Pierre-Louis Bonicoli wrote:

Oups, updated again.

Thanks.

What is a true boolean ? Is a YAML boolean a real one ?

YAML is typed so there are booleans, not just empty-for-false or interpreted strings. So currently you are using yes but it is quoted, so it is a string. Unquote it and it becomes a boolean value. Please see http://yaml.org/refcard.html

So, you can then use when: trust_distribution_crts, which is simpler, does not need any filter (you already used defaults/main.yml and it is a boolean already). If the user provides crap, I think it is a waste of energy to guess or check all possible mistakes. The documentation should say it is a boolean and the user MUST provide a YAML boolean or face an unwanted behavior.

Another comment: I think Update ca-certificates configuration should be a handler; this way we can reuse it if needed.

#9

Updated by Pierre-Louis Bonicoli about 2 years ago

The variable could be defined in a INI-like inventory (or on the command line using -e).
Upstream strongly recommends to use bool filter.

I have updated the code (e31ccf820ec759847b58b3a21e648a6f707d301c):
  • YAML boolean are used but I have kept the bool filter
  • Update ca-certificates configuration is now an handler

Once reviewed and before merging I will squash this commit.

#10

Updated by Marc Dequènes about 2 years ago

Ok, I understand the logic. Nevertheless I think inside the role we should take advantage of types. Which means we should use an intermediate variable to avoid having type-related filters all over the place (at least when the same variable is used multiple times). This way it is prepared safely and the rest of the code stays readable.

Also you're right we should fortify our role APIs.

The changes are fine and you can merge.

#11

Updated by Pierre-Louis Bonicoli about 2 years ago

  • Status changed from In Progress to Resolved

Merged, thanks for the reviews.

#12

Updated by Pierre-Louis Bonicoli about 2 years ago

  • Related to Enhancement #533: Install a Jessie LXC container with systemd enabled in order to test/validate Burp setup added

Also available in: Atom PDF