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

fix some python3 type issue on x509 #52014

Closed
wants to merge 2 commits into from
Closed

fix some python3 type issue on x509 #52014

wants to merge 2 commits into from

Conversation

arsiesys
Copy link

@arsiesys arsiesys commented Mar 7, 2019

fix some python3 type issue on x509

[salt/states/x509.py]
Behavior:
When you store the private key and a certificate in a same file, both have to be in the same format.
But in python3, when the private key is already existing and get using "get_pem_entry", it will return a bytes. A new generated one will be in str. That mean that at the second execution, your state will fail. We have to convert the get_pem_entry return in a str using .decode().

[salt/modules/x509.py]
Behavior:
At each state execution, salt is making a comparison of the current key a new generated one.
If one element (Subject, public key of the issuer...) differ, the key is replaced.
In python3, the local key is in bytes while the remote key is provided in str. The compraison then fail $
Fix:
- use .decode() on the return of get_public_key to be sure that the result will not be a bytes

@arsiesys
Copy link
Author

arsiesys commented Mar 7, 2019

Fix #52026

@alxwr
Copy link
Contributor

alxwr commented Apr 9, 2019

@arsiesys I opened a PR for 2019.2 based on your work. (#52456)

@dwoz dwoz added the v2019.2.1 unsupported version label Apr 12, 2019
@KChandrashekhar KChandrashekhar added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 18, 2019
@KChandrashekhar
Copy link

@arsiesys Thanks for providing fix. Can you add a test case ? We are now requiring test cases to be written for all PRs

@arsiesys
Copy link
Author

arsiesys commented Jul 3, 2019

@arsiesys Thanks for providing fix. Can you add a test case ? We are now requiring test cases to be written for all PRs

Hello @KChandrashekhar, I am sorry but I don't really have the bandwidth and time to work on it for now :(. As I am not really comfortable with the tests currently deployed, it would take time to learn this part.

Loïc Yavercovski and others added 2 commits July 6, 2019 22:23
[salt/states/x509.py]
Behavior:
When you store the private key and a certificate in a same file, both have to be in the same format.
But in python3, when the private key is already existing and get using "get_pem_entry", it will return a bytes. A new generated one will be in str. That mean that at the second execution, your state will fail. We have to convert the get_pem_entry return in a str using .decode().

[salt/modules/x509.py]
Behavior:
At each state execution, salt is making a comparison of the current key a new generated one.
If one element (Subject, public key of the issuer...) differ, the key is replaced.
In python3, the local key is in bytes while the remote key is provided in str. The compraison then fail $
Fix:
- use .decode() on the return of get_public_key to be sure that the result will not be a bytes
@arsiesys arsiesys requested a review from a team as a code owner July 6, 2019 23:43
@dwoz dwoz changed the base branch from 2019.2 to 2019.2.1 July 6, 2019 23:44
@dwoz
Copy link
Contributor

dwoz commented Jul 6, 2019

re-run full all

@dwoz dwoz removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jul 15, 2019
@waynew
Copy link
Contributor

waynew commented Jul 24, 2019

@arsiesys just FYI, we're working on getting 2019.2.1 stable, and then we'll look at merging these changes in.

@alxwr
Copy link
Contributor

alxwr commented Aug 12, 2019

@arsiesys Please also have a look at #52456. There are similar bugs in modules.x509.read_crl and modules.x509.crl_verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants