Allow the CA and CRL to be file paths #484
Conversation
jeremycline
force-pushed
the
fix-crl-handling
branch
from
fb3185d to
8970c34
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
jeremycline
force-pushed
the
fix-crl-handling
branch
5 times, most recently
from
a11c316 to
b7d20e3
Compare
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 fedora-infra#481 fixes fedora-infra#365 Signed-off-by: Jeremy Cline <[email protected]>
jeremycline
force-pushed
the
fix-crl-handling
branch
from
b7d20e3 to
e9caeae
Compare
jeremycline
changed the title
| 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. |
There was a problem hiding this comment.
Might be good to mention the deprecation here too.
There was a problem hiding this comment.
I made a note of it in the public interface
fedmsg/crypto/utils.py
Outdated
| ASCII encoding. | ||
|
|
||
| Returns: | ||
| str: |
There was a problem hiding this comment.
Might want to say that the string is the contents of the requested certificate.
fedmsg/crypto/x509.py
Outdated
| try: | ||
| ca_certificate, crl = utils.load_certificates(ca_location, crl_location) | ||
| os.write(fd, ca_certificate.encode('ascii')) | ||
| os.close(fd) |
There was a problem hiding this comment.
You might want to move this to a finally block. If the write() raises an IOError nothing will close the file.
fedmsg/crypto/x509.py
Outdated
| crl = M2Crypto.X509.load_crl(crl) | ||
| fd, crlfile = tempfile.mkstemp(text=True) | ||
| os.write(fd, crl.encode('ascii')) | ||
| os.close(fd) |
fedmsg/tests/crypto/test_utils.py
Outdated
|
|
||
| self.assertEqual(('fresh_ca', None), utils._cached_certificates[location]) | ||
| self.assertEqual('fresh_ca', ca) | ||
| self.assertTrue(crl is None) |
There was a problem hiding this comment.
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.
fedmsg/tests/crypto/test_utils.py
Outdated
|
|
||
| self.assertEqual((expected_cert, None), utils._cached_certificates[location]) | ||
| self.assertEqual(expected_cert, ca) | ||
| self.assertTrue(crl is None) |
There was a problem hiding this comment.
I unindented the bottom two, the top one pokes in the mocked dict so it needs to be inside the mock.
Signed-off-by: Jeremy Cline <[email protected]>
jeremycline
force-pushed
the
fix-crl-handling
branch
from
58c90ea to
aa360eb
Compare
|
Thanks for the review, @bowlofeggs, I think I've addressed everything and gotten the tests passing again. |
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 [email protected]