Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warning for x509.certificate_managed in Salt 3001.1 #58165

Closed
m0duspwnens opened this issue Aug 10, 2020 · 5 comments · Fixed by #63099
Closed

Warning for x509.certificate_managed in Salt 3001.1 #58165

m0duspwnens opened this issue Aug 10, 2020 · 5 comments · Fixed by #63099
Assignees
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt severity-low 4th level, cosemtic problems, work around exists State-Module

Comments

@m0duspwnens
Copy link

Description
After updating to 3001.1, I now see a warning for x509.certificate_managed. This occurs whether or not replace: False is specified. Previously we didn't have to specify replace: False

salt-call state.apply ca
[WARNING ] State for file: /etc/pki/ca.crt - Neither 'source' nor 'contents' nor 'contents_pillar' nor 'contents_grains' was defined, yet 'replace' was set to 'True'. As there is no source to replace the file with, 'replace' has been set to 'False' to avoid reading the file unnecessarily.
[WARNING ] State for file: /etc/pki/ca.crt - Neither 'source' nor 'contents' nor 'contents_pillar' nor 'contents_grains' was defined, yet 'replace' was set to 'True'. As there is no source to replace the file with, 'replace' has been set to 'False' to avoid reading the file unnecessarily.

Setup
Attached ca.init.sls
ca.init.sls.txt

Steps to Reproduce the behavior
[INFO ] Completed state [/etc/pki/issued_certs] at time 16:58:30.441131 (duration_in_ms=2.562)
[DEBUG ] LazyLoaded x509.get_pem_entry
[DEBUG ] LazyLoaded x509.private_key_managed
[INFO ] Running state [/etc/pki/ca.crt] at time 16:58:30.445676
[WARNING ] State for file: /etc/pki/ca.crt - Neither 'source' nor 'contents' nor 'contents_pillar' nor 'contents_grains' was defined, yet 'replace' was set to 'True'. As there is no source to replace the file with, 'replace' has been set to 'False' to avoid reading the file unnecessarily.
[INFO ] Running state [/etc/pki/ca.crt] at time 16:58:30.463342
[INFO ] Executing state x509.certificate_managed for [/etc/pki/ca.crt]
[WARNING ] State for file: /etc/pki/ca.crt - Neither 'source' nor 'contents' nor 'contents_pillar' nor 'contents_grains' was defined, yet 'replace' was set to 'True'. As there is no source to replace the file with, 'replace' has been set to 'False' to avoid reading the file unnecessarily.
[INFO ] Certificate /etc/pki/ca.crt is valid and up to date
[INFO ] Completed state [/etc/pki/ca.crt] at time 16:58:30.472753 (duration_in_ms=9.411)

Expected behavior
Behavior is almost as expected minus the warning message.

Screenshots
NA

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
Salt Version:
           Salt: 3001.1

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.4.2
      docker-py: 2.6.1
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.11.1
        libgit2: Not Installed
       M2Crypto: 0.35.2
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.2
   mysql-python: 1.3.12
      pycparser: Not Installed
       pycrypto: Not Installed
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.8 (default, Apr  2 2020, 13:34:55)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.0.0
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.1.4

System Versions:
           dist: centos 7 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-1127.18.2.el7.x86_64
         system: Linux
        version: CentOS Linux 7 Core

Additional context
NA

@m0duspwnens m0duspwnens added the Bug broken, incorrect, or confusing behavior label Aug 10, 2020
@DmitryKuzmenko
Copy link
Contributor

The function docstring says that x509.certificate_managed state module accepts any kwargs supported by file.managed but as I can see in the code the only following arguments are passed to it:

        "user",
        "group",
        "mode",
        "makedirs",
        "dir_mode",
        "backup",
        "create",
        "follow_symlinks",
        "check_cmd",

I don't see replace there. This is wrong.

@DmitryKuzmenko DmitryKuzmenko added Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt severity-low 4th level, cosemtic problems, work around exists State-Module and removed needs-triage labels Aug 27, 2020
@DmitryKuzmenko DmitryKuzmenko removed their assignment Aug 27, 2020
@DmitryKuzmenko DmitryKuzmenko added this to the Approved milestone Aug 27, 2020
@alxwr
Copy link
Contributor

alxwr commented Sep 8, 2020

I'm experiencing the same bug. The error message ("[…] 'replace' has been set to 'False' […]") stems from file.manged. x509.certificate_managed uses file.managed while providing the contents.

Now, when a certificate is valid (not expired, nothing changed, etc.) there's nothing to do except maybe altering some attributes of the file itself. (Just using file.managed while passing all arguments through x509.certificate_managed.)

Well, if you don't set content in file.managed (because you don't need to), but omit the replace: False, you'll see this error in the logs.

AFAICS this is exactly what happens in https://github.com/saltstack/salt/blob/master/salt/states/x509.py#L675.

I might be able to come up with a PR, but I have severe time constraints. If anyone is faster than me to start working on this, please let me know beforehand so we don't double the work. :-) Thanks!

@OrangeDog
Copy link
Contributor

OrangeDog commented Nov 19, 2020

Still present in 3002.2

The solution is probably to correctly set content like the other x509 states.
At the moment it's immediately set to a newly-generated certificate (#52167).

@ymasson
Copy link
Contributor

ymasson commented Feb 2, 2021

is there a workaround available?

@OrangeDog
Copy link
Contributor

OrangeDog commented Aug 31, 2022

This sort of logic should work. The problem is right now you also have to avoid #62590, which this doesn't quite do, as this state is frequently used with x509.private_key_managed as a prereq.

if current_is_valid:
    file_args["contents"] = current_cert_pem
else:
    file_args["contents"] = new_cert_pem

... # do append_certs

if __opts__["test"]:
    if __salt__["file.file_exists"](name):
        file_args["replace"] = False
        ret = _certificate_file_managed(ret, file_args)
else:
    ret = _certificate_file_managed(ret, file_args)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt severity-low 4th level, cosemtic problems, work around exists State-Module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants