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

Allow the CA and CRL to be file paths #484

Merged
merged 2 commits into from Oct 9, 2017

Conversation

Projects
None yet
2 participants
@jeremycline
Member

jeremycline commented Sep 29, 2017

This also changes how they are cached in order to better handle expired
or rotated CRLs/CAs.

If the CA/CRL is a file, it is read into a cache and used until a
message fails validation. At that point, the cache is invalidated and
the CA/CRL is reloaded. If the message still fails validation, we mark
it as invalid and continue.

If the CA/CRL is a URL, the file is downloaded and cached in memory just
like the file approach.

It would be nice if the process halted when a fatal error was
encountered (like the CRL being expired), but unfortunately there's no
way to communicate that to moksha. Once we drop moksha we can do that
with a set of fedmsg exceptions, but for now logging at the error level
is the only thing we can do.

fixes #481
fixes #365

Signed-off-by: Jeremy Cline jeremy@jcline.org

@jeremycline jeremycline force-pushed the jeremycline:fix-crl-handling branch from fb3185d to 8970c34 Sep 29, 2017

@codecov

This comment has been minimized.

codecov bot commented Sep 29, 2017

Codecov Report

Merging #484 into develop will decrease coverage by 4.02%.
The diff coverage is 46.87%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #484      +/-   ##
===========================================
- Coverage    58.82%   54.79%   -4.03%     
===========================================
  Files           29       29              
  Lines         1831     1847      +16     
  Branches       303      302       -1     
===========================================
- Hits          1077     1012      -65     
- Misses         667      749      +82     
+ Partials        87       86       -1
Impacted Files Coverage Δ
fedmsg/crypto/x509.py 20% <10.71%> (-68.05%) ⬇️
fedmsg/crypto/x509_ng.py 100% <100%> (ø) ⬆️
fedmsg/crypto/utils.py 58.46% <64%> (ø) ⬆️
fedmsg/meta/__init__.py 77.69% <0%> (-1.54%) ⬇️
fedmsg/crypto/gpg.py 84.78% <0%> (-1.09%) ⬇️
fedmsg/core.py 45.05% <0%> (-0.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73425a9...8970c34. Read the comment docs.

@codecov

This comment has been minimized.

codecov bot commented Sep 29, 2017

Codecov Report

Merging #484 into develop will increase coverage by 0.44%.
The diff coverage is 88.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #484      +/-   ##
===========================================
+ Coverage    58.82%   59.26%   +0.44%     
===========================================
  Files           29       29              
  Lines         1831     1851      +20     
  Branches       303      302       -1     
===========================================
+ Hits          1077     1097      +20     
  Misses         667      667              
  Partials        87       87
Impacted Files Coverage Δ
fedmsg/crypto/x509_ng.py 100% <100%> (ø) ⬆️
fedmsg/crypto/x509.py 90.35% <100%> (+2.3%) ⬆️
fedmsg/crypto/utils.py 58.46% <65.21%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73425a9...aa360eb. Read the comment docs.

@jeremycline jeremycline force-pushed the jeremycline:fix-crl-handling branch 5 times, most recently from a11c316 to b7d20e3 Oct 2, 2017

Allow the CA and CRL to be file paths
This also changes how they are cached in order to better handle expired
or rotated CRLs/CAs.

If the CA/CRL is a file, it is read into a cache and used until a
message fails validation. At that point, the cache is invalidated and
the CA/CRL is reloaded. If the message still fails validation, we mark
it as invalid and continue.

If the CA/CRL is a URL, the file is downloaded and cached in memory just
like the file approach.

It would be nice if the process halted when a fatal error was
encountered (like the CRL being expired), but unfortunately there's no
way to communicate that to moksha. Once we drop moksha we can do that
with a set of fedmsg exceptions, but for now logging at the error level
is the only thing we can do.

fixes #481
fixes #365

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the jeremycline:fix-crl-handling branch from b7d20e3 to e9caeae Oct 2, 2017

@jeremycline jeremycline changed the title from [WIP] Allow the CA and CRL to be file paths to Allow the CA and CRL to be file paths Oct 2, 2017

Args:
location (str): The location to load. This can either be an HTTPS URL or an absolute file
path. This is intended to be used with PEM-encoded certificates and therefore assumes
ASCII encoding.

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 2, 2017

Member

Might be good to mention the deprecation here too.

This comment has been minimized.

@jeremycline

jeremycline Oct 3, 2017

Member

I made a note of it in the public interface

ASCII encoding.
Returns:
str:

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 2, 2017

Member

Might want to say that the string is the contents of the requested certificate.

try:
ca_certificate, crl = utils.load_certificates(ca_location, crl_location)
os.write(fd, ca_certificate.encode('ascii'))
os.close(fd)

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 2, 2017

Member

You might want to move this to a finally block. If the write() raises an IOError nothing will close the file.

This comment has been minimized.

@jeremycline
crl = M2Crypto.X509.load_crl(crl)
fd, crlfile = tempfile.mkstemp(text=True)
os.write(fd, crl.encode('ascii'))
os.close(fd)

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 2, 2017

Member

Similar problem here.

self.assertEqual(('fresh_ca', None), utils._cached_certificates[location])
self.assertEqual('fresh_ca', ca)
self.assertTrue(crl is None)

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 2, 2017

Member

Since these assertions are indented, they still live inside the mock and thus don't necessarily prove that the function worked properly. I suggest de-indenting them.

self.assertEqual((expected_cert, None), utils._cached_certificates[location])
self.assertEqual(expected_cert, ca)
self.assertTrue(crl is None)

This comment has been minimized.

@bowlofeggs

bowlofeggs Oct 2, 2017

Member

These should also be unindented.

This comment has been minimized.

@jeremycline

jeremycline Oct 3, 2017

Member

I unindented the bottom two, the top one pokes in the mocked dict so it needs to be inside the mock.

Address pull request comments
Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the jeremycline:fix-crl-handling branch from 58c90ea to aa360eb Oct 3, 2017

@jeremycline

This comment has been minimized.

Member

jeremycline commented Oct 3, 2017

Thanks for the review, @bowlofeggs, I think I've addressed everything and gotten the tests passing again.

@bowlofeggs

gg ez

@jeremycline jeremycline merged commit 3c55150 into fedora-infra:develop Oct 9, 2017

2 of 3 checks passed

codecov/patch 88.23% of diff hit (target 100%)
Details
codecov/project 59.26% (+0.44%) compared to 73425a9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremycline jeremycline deleted the jeremycline:fix-crl-handling branch Oct 9, 2017

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