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 serial number writing into CRL files with the x509 module #47986

Merged
merged 9 commits into from Jun 12, 2018

Conversation

Projects
None yet
3 participants
@jeduardo
Contributor

jeduardo commented Jun 5, 2018

What does this PR do?

This PR removes code that is converting certificate serial numbers from hexadecimal to integer when writing CRL files using the x509 module.

What issues does this PR fix or reference?

This PR fixes #47984.

Previous Behavior

CRLs are saved with wrong serial numbers as per described in the referenced issue, thus being useless to actually revoke certificates.

New Behavior

The correct serial numbers are saved into the CRLs, as described in the next steps:

  1. Issue CRL through Salt state after patch is applied.
root@discordia:~# salt-call state.sls pki.crl
local:
----------
          ID: create root crl
    Function: x509.crl_managed
        Name: /srv/certs/root.crl
      Result: True
     Comment: File /srv/certs/root.crl is in the correct state
     Started: 23:56:33.864532
    Duration: 36.605 ms
     Changes:   
----------
          ID: create intermediate crl
    Function: x509.crl_managed
        Name: /srv/certs/intermediate.crl
      Result: True
     Comment: File /srv/certs/intermediate.crl updated
     Started: 23:56:33.901254
    Duration: 23.15 ms
     Changes:   
              ----------
              New:
                  ----------
                  Issuer:
                      ----------
                      C:
                          DE
                      CN:
                          Intermediate CA
                      L:
                          City
                      ST:
                          City
                  Last Update:
                      2018-06-05 21:56:33
                  Next Update:
                      2018-09-13 21:56:33
                  Revoked Certificates:
                      |_
                        ----------
                        57CE95B07CDC5DD8:
                            ----------
                            CRL entry extensions:
                                ----------
                                X509v3 CRL Reason Code:
                                    Key Compromise
                            Revocation Date:
                                2018-05-01 00:00:00
                  Signature Algorithm:
                      sha256WithRSAEncryption
                  Version:
                      1 (0x0)
              Old:
                  /srv/certs/intermediate.crl does not exist.

Summary for local
------------
Succeeded: 2 (changed=1)
Failed:    0
------------
Total states run:     2
Total run time:  59.755 ms
  1. Verify that certificate is correctly identified as revoked:
root@discordia:~# openssl verify -CAfile /srv/certs/bundleca.pem -CRLfile /srv/certs/intermediate.crl -crl_check /srv/certs/salt1.dev.infra.network.pem 
C = DE, CN = salt1.dev.infra.network, L = City, ST = City
error 23 at 0 depth lookup: certificate revoked
error /srv/certs/salt1.dev.infra.network.pem: verification failed
  1. Verify that the certificate is present in the CRL with the correct serial number:
root@discordia:~# openssl x509 -serial -in /srv/certs/salt1.dev.infra.network.pem -noout
serial=57CE95B07CDC5DD8
root@discordia:~# openssl crl -in /srv/certs/intermediate.crl -text -noout | grep Serial
    Serial Number: 57CE95B07CDC5DD8

Tests written?

Yes

Commits signed with GPG?

No

Stopped converting the certificate hexadecimal serial number to an in…
…teger in order to avoid breaking CRLs.

@rallytime rallytime requested a review from garethgreenaway Jun 6, 2018

@jeduardo jeduardo force-pushed the jeduardo:fix-certificate-serial-in-crl branch from 1d97029 to cdb8f62 Jun 7, 2018

@jeduardo

This comment has been minimized.

Contributor

jeduardo commented Jun 7, 2018

@garethgreenaway hey! In the process of writing some unit tests to validate the fix I ended up stumbling upon the realization that the CRL function might not be working for Salt 2018 and develop at all, since the OpenSSL bindings except to receive non-Unicode strings and inside Salt it seems to be all unicode now. So whilst the previous fix worked fine for Salt 2017, for Salt 2018 and develop it was simply a no go.

The unit tests and a real test instance confirm that everything should be working fine now.

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Jun 8, 2018

@jeduardo Looks good except for a couple lint issues. Once those are cleaned up we can get this merged. Thanks!

@jeduardo

This comment has been minimized.

Contributor

jeduardo commented Jun 8, 2018

@garethgreenaway hey thanks for the review! All lint issues should be fixed now, at least locally pylint gives me a 10/10.

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Jun 9, 2018

@jeduardo Awesome! Once the tests finish we should be good to go. Thanks again!

@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Jun 9, 2018

re-run all

@rallytime

One small request from me on the new tests. (Thank you for writing those!)

days_valid=100,
digest='sha512')
with salt.utils.fopen(ca_crl_file.name, 'r') as crl_file:

This comment has been minimized.

@rallytime

rallytime Jun 11, 2018

Contributor

This should use salt.utils.files.fopen instead. (The import above will need to be adjusted as well.)

This comment has been minimized.

@jeduardo

jeduardo Jun 11, 2018

Contributor

Hey @rallytime, fixed the call as you requested. I remember seeing it on this form on other files as well. lint runs with 10/10 and tests run on the local codebase. :)

I think the lint warnings should be updated though, because what made me use salt.utils.fopen was this lint warning here:

************* Module tests.unit.modules.test_x509
tests/unit/modules/test_x509.py:235: [W8470(resource-leakage), X509TestCase.test_create_crl] Resource leakage detected. Please use 'with salt.utils.fopen()' instead of 'with open()'. It assures salt does not leak file handles. 

This comment has been minimized.

@rallytime

rallytime Jun 11, 2018

Contributor

@jeduardo Ah, yes! Good catch. We didn't update those path names in the linter help messages.

@rallytime rallytime merged commit 6106a43 into saltstack:develop Jun 12, 2018

5 of 10 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5642 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10613 — FAILURE
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22536 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19696 — FAILURE
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25840 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17905 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23572 — SUCCESS
Details

rallytime added a commit that referenced this pull request Jun 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment