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

[2018.3.4] x509.certificate_managed comparisons fail on py3 #52180

Closed
OrangeDog opened this issue Mar 14, 2019 · 7 comments
Closed

[2018.3.4] x509.certificate_managed comparisons fail on py3 #52180

OrangeDog opened this issue Mar 14, 2019 · 7 comments
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged stale
Milestone

Comments

@OrangeDog
Copy link
Contributor

After some debugging why it always replaced the certificate, I have identified two type differences between current_comp and new_comp.
https://github.com/saltstack/salt/blob/v2018.3.4/salt/states/x509.py#L526

In new_comp, "Public Key" is a str but in current_comp it is bytes.
In new comp, "X509v3 Extensions" is a dict but in current_comp it is OrderedDict

Probably related: #50680

@clinta
Copy link
Contributor

clinta commented Mar 14, 2019

I'm not seeing these type differences. I am seeing differences in the Subject Hash and Issuer Hash which show up only under python3.

@clinta
Copy link
Contributor

clinta commented Mar 14, 2019

I think the problem is the order of the subject elements. While this happened to always be in the same order in python2, it appears to be in a non-deterministic order in python3, which changes the subject hash.

@OrangeDog
Copy link
Contributor Author

No, all the actual values (including those hashes) I get are the same.
Investigating with pdb shows these above type differences.

Salt Version:
           Salt: 2018.3.4

Dependency Versions:
       M2Crypto: 0.32.0
         Python: 3.6.7 (default, Oct 22 2018, 11:32:17)
/etc/{{ grains['host'] }}.crt:
  x509.certificate_managed:
    - ca_server: ca
    - signing_policy: www
    - public_key: /etc/{{ grains['host'] }}.key
    - days_remaining: 7
    - CN: {{ grains['fqdn'] }}
    - subjectAltName: 'DNS:{{ grains['fqdn'] }}, DNS:{{ grains['host'] }}'
x509_signing_policies:
  www:
    - signing_private_key: /etc/ca.key
    - signing_cert: /etc/ca.crt
    - days_valid: 90
    - O: ...
    - L: ...
    - ST: ...
    - C: ...
    - basicConstraints: 'critical, CA:false'
    - keyUsage: 'critical, digitalSignature, keyEncipherment'
    - subjectKeyIdentifier: 'hash'
    - authorityKeyIdentifier: 'keyid, issuer:always'

@Ch3LL Ch3LL added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Mar 14, 2019
@Ch3LL Ch3LL added this to the Blocked milestone Mar 14, 2019
@clinta
Copy link
Contributor

clinta commented Mar 14, 2019

Interesting, I'm not seeing the same thing on 2019.2 Python 3.5.2.

@alxwr
Copy link
Contributor

alxwr commented Apr 9, 2019

This seems to be a duplicate of #52026.

@OrangeDog
Copy link
Contributor Author

If of interest, I fixed it locally with a custom state module.
Note that I removed some functionality I don't need in order to simplify the task.

@stale
Copy link

stale bot commented Jan 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Jan 8, 2020
@stale stale bot closed this as completed Jan 15, 2020
glynnforrest added a commit to glynnforrest/salt that referenced this issue Apr 10, 2020
The function now displays clearer error messages when a problem occurs
and informative messages when comparing an existing certificate.

test=True is now supported.

It fixes saltstack#52180, saltstack#39608, saltstack#41858 and others:

* Error messages from the x509 module calls are written directly to
the certificate file - fixed, the certificate file is only created
when the x509 module calls succeed.
* Certificates are created when no changes are required - fixed, the
comparison logic has been updated.

The `managed_private_key` option has been removed due to the added
complexity. The functionality can easily be replicated with an
additional call to `x509.private_key_managed`. According to the comment
at saltstack#39608 (comment)
`managed_private_key` has not worked since at least v2016.11.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged stale
Projects
None yet
Development

No branches or pull requests

4 participants