Changing behavior of new fullchain argument. #34916

felixfontein · 2018-01-16T06:57:42Z

SUMMARY

This PR should initiate a discussion on whether #18996 might not be solved in a better way than in #22074, namely by not making the user choose between getting a leaf certificate or a fullchain certificate (i.e. certificate followed by intermediate certificates), but to allow the user to get both.

The approach in #22074 was to provide a new boolean option fullchain, which determines whether the file written to dest/cert will be a leaf certificate or a fullchain certificate.

This PR creates a new option fullchain_dest with alias fullchain (i.e. the boolean option is removed),

@resmo, @manicai: what do you think?

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

letsencrypt

ANSIBLE VERSION

devel

ADDITIONAL INFORMATION

The current fullchain option only exists in the devel branch (to my knowledge), not in any release branch. So changing it shouldn't hurt backwards compatibility. Everyone using the devel version will notice that something changed when 2.5 will be released, i.e. there should be no silent failures.

This PR is related to #34328, which adds another option chain_dest/chain which allows to write the intermediate certificate chain into a separate file. With both this and #34328 merged, the user can choose which one of certificate, fullchain, and chain to be written to disk, and is not forced to do additional file operations to extract the chain or create a fullchain certificate.

ansibot · 2018-01-16T06:57:56Z

cc @mgruener
click here for bot help

felixfontein · 2018-01-24T08:49:36Z

This PR must be either merged or closed by end of February 7th, since then there's community module freeze and we shouldn't change module options once they are released (fullchain will first appear in Ansible 2.5).

resmo

needs_info

resmo · 2018-01-24T09:59:39Z

lib/ansible/modules/web_infrastructure/letsencrypt.py

+      - "The destination file for the full chain (i.e. certificate followed
+         by chain of intermediate certificates)."
+      - "If C(dest) is specified, C(fullchain_dest) can be left away."
+    required: true


according code, it is not required.

See below. How should this (i.e. at least one of dest and fullchain is specified) be marked here? My idea was to mark both as required, but add some text that only one needs to be specified. Should I mark both as not required, and add a note to each that at least one of them must be specified?

yes, both not required, but put a note in the doc that "Either C(dest) or C(fullchain_dest) is required."

I've updated the documentation. I hope it's better explained now.

resmo · 2018-01-24T10:00:01Z

lib/ansible/modules/web_infrastructure/letsencrypt.py

+      - "The destination file for the full chain (i.e. certificate followed
+         by chain of intermediate certificates)."
+      - "If C(dest) is specified, C(fullchain_dest) can be left away."
+    required: true
    default: false


according code, there is no default, so can be left off.

Oops, I just forgot to remove this. The type isn't bool anymore anyway, so false makes no sense.

resmo · 2018-01-24T10:01:53Z

lib/ansible/modules/web_infrastructure/letsencrypt.py

@@ -950,7 +964,10 @@ def main():
    module.run_command_environ_update = dict(LANG='C', LC_ALL='C', LC_MESSAGES='C', LC_CTYPE='C')
    locale.setlocale(locale.LC_ALL, 'C')

-    cert_days = get_cert_days(module, module.params['dest'])
+    if module.params.get('dest'):


so it looks like they fullchain_dest and dest are also mutually exclusive, aren't they? require one of but not both?

Both dest and fullchain_dest are not required, but at least one of them must be specified. Both can be specified as well. If both are specified, the module assumes that both are equivalent for determining whether the certificate is new enough (if dest is given, the certificate it points to is checked, otherwise the certificate where fullchain_dest points to).

While only one of them is required, both will be created (if both are specified) if a new certificate is retrieved. So the user can decide whether she wants to work with leaf certificates, fullchain certificates, or both (without the need to manually concatenating or splitting files).

felixfontein · 2018-01-24T14:45:45Z

I rebased and squashed the commits.

felixfontein · 2018-01-25T13:25:04Z

bot_status

resmo · 2018-01-25T13:27:02Z

shipit

ansibot · 2018-01-25T13:32:44Z

Components

lib/ansible/modules/web_infrastructure/letsencrypt.py
support: community
maintainers: felixfontein mgruener resmo

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 1
shipit_actors (maintainers or core team members): resmo felixfontein
shipit_actors_other: []

click here for bot help

* Allowing to write intermediate certificate into file. * Fixing merge errors introduced in 25b1db5 (#34916).

* Allowing to write intermediate certificate into file. * Fixing merge errors introduced in 25b1db5 (ansible#34916).

resmo self-requested a review January 16, 2018 07:32

maxamillion removed the needs_triage Needs a first human triage before being processed. label Jan 16, 2018

felixfontein force-pushed the letsencrypt-change-fullchain-behavior branch from f2d1695 to 8a5cf0e Compare January 16, 2018 19:53

ansibot added the owner_pr This PR is made by the module's maintainer. label Jan 16, 2018

felixfontein force-pushed the letsencrypt-change-fullchain-behavior branch from 8a5cf0e to eef53a6 Compare January 17, 2018 08:11

felixfontein force-pushed the letsencrypt-change-fullchain-behavior branch from eef53a6 to d1abf9a Compare January 17, 2018 10:45

felixfontein mentioned this pull request Jan 22, 2018

Allowing to write intermediate certificate into file. #34328

Merged

resmo reviewed Jan 24, 2018

View reviewed changes

Changing behavior of new fullchain argument.

25b1db5

felixfontein force-pushed the letsencrypt-change-fullchain-behavior branch from 9544f33 to 25b1db5 Compare January 24, 2018 14:45

ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 25, 2018

ansibot merged commit e750559 into ansible:devel Jan 25, 2018

felixfontein deleted the letsencrypt-change-fullchain-behavior branch January 25, 2018 13:34

felixfontein added a commit to felixfontein/ansible that referenced this pull request Jan 25, 2018

Fixing merge errors introduced in 25b1db5 (ansible#34916).

d6e3528

ansibot pushed a commit that referenced this pull request Jan 26, 2018

Allowing to write intermediate certificate into file. (#34328)

a69445e

* Allowing to write intermediate certificate into file. * Fixing merge errors introduced in 25b1db5 (#34916).

Lujeni pushed a commit to Lujeni/ansible that referenced this pull request Feb 1, 2018

Changing behavior of new fullchain argument. (ansible#34916)

69b3b24

Lujeni pushed a commit to Lujeni/ansible that referenced this pull request Feb 1, 2018

Allowing to write intermediate certificate into file. (ansible#34328)

8a94345

* Allowing to write intermediate certificate into file. * Fixing merge errors introduced in 25b1db5 (ansible#34916).

ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 5, 2018

felixfontein mentioned this pull request Apr 7, 2018

Letsencrypt: add account management module #37275

Merged

dagwieers added the crypto Crypto community (ACME, openssl, letsencrypt) label Feb 7, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing behavior of new fullchain argument. #34916

Changing behavior of new fullchain argument. #34916

felixfontein commented Jan 16, 2018

ansibot commented Jan 16, 2018

felixfontein commented Jan 24, 2018

resmo left a comment

resmo Jan 24, 2018

felixfontein Jan 24, 2018

resmo Jan 24, 2018

felixfontein Jan 24, 2018

resmo Jan 24, 2018

felixfontein Jan 24, 2018

resmo Jan 24, 2018

felixfontein Jan 24, 2018

resmo Jan 24, 2018

felixfontein commented Jan 24, 2018

felixfontein commented Jan 25, 2018

resmo commented Jan 25, 2018

ansibot commented Jan 25, 2018

Changing behavior of new fullchain argument. #34916

Changing behavior of new fullchain argument. #34916

Conversation

felixfontein commented Jan 16, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Jan 16, 2018

felixfontein commented Jan 24, 2018

resmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Jan 24, 2018

felixfontein commented Jan 25, 2018

resmo commented Jan 25, 2018

ansibot commented Jan 25, 2018

Components

Metadata