Added support for specifying/limiting content-checksums used by Pulp. #894

ggainey · 2020-09-08T21:04:49Z

settings.ALLOWED_CONTENT_CHECKSUMS now drives the other checksum-related
fields of Artifact (DIGEST_FIELDS, COMMON_DIGEST_FIELDS, RELIABLE_DIGEST_FIELDS)

closes #5216

pulpbot · 2020-09-08T21:10:27Z

Attached issue: https://pulp.plan.io/issues/5216

mdellweg

Thank you for letting me get a very early look on this brill PR.

Running it in the FIPS environment exposed at least one more location where a not allowed hash algorithm is used: pulpcore/app/files.py

docs/settings.rst

pulpcore/app/settings.py

pulpcore/tests/unit/serializers/test_content.py

pulpcore/app/models/content.py

ggainey · 2020-09-09T17:54:38Z

Thank you for letting me get a very early look on this brill PR.

Running it in the FIPS environment exposed at least one more location where a not allowed hash algorithm is used: pulpcore/app/files.py

Concur - see changes in files.py with latest repush

docs/settings.rst

daviddavis · 2020-09-09T19:05:34Z

This code looks great to me. Impressive turnaround time on this.

Question: would it be possible to test this out by defining ALLOWED_CONTENT_CHECKSUMS in https://github.com/pulp/pulpcore/blob/master/.travis/settings.py.j2?

pulpcore/app/settings.py

docs/settings.rst

pulpcore/app/models/content.py

pulpcore/app/settings.py

docs/settings.rst

pulpcore/app/settings.py

pulpcore/app/models/content.py

mdellweg · 2020-09-10T11:27:16Z

I can confirm, this peachy PR is passing the pulp_file test suite (unit & functional) on a the centos8-fips box.

pulpcore/app/models/content.py

pulpcore/tests/unit/models/test_content.py

pulpcore/tests/unit/serializers/test_content.py

ggainey · 2020-09-10T17:08:44Z

This code looks great to me. Impressive turnaround time on this.

Question: would it be possible to test this out by defining ALLOWED_CONTENT_CHECKSUMS in https://github.com/pulp/pulpcore/blob/master/.travis/settings.py.j2?

@daviddavis , I have...no insight here. I've done a lot of local testing by setting in /etc/pulp/settings.py.

mdellweg · 2020-09-10T17:34:48Z

This code looks great to me. Impressive turnaround time on this.
Question: would it be possible to test this out by defining ALLOWED_CONTENT_CHECKSUMS in https://github.com/pulp/pulpcore/blob/master/.travis/settings.py.j2?

@daviddavis , I have...no insight here. I've done a lot of local testing by setting in /etc/pulp/settings.py.

You would add that to the plugins template_config and regenerate the travis directory.

Like here: https://github.com/pulp/pulpcore/blob/master/template_config.yml#L30

pulpcore/app/models/content.py

bmbouter · 2020-09-10T20:51:14Z

pulpcore/app/models/content.py

+# All available digest fields ordered by algorithm strength.
+_DIGEST_FIELDS = []
+for alg in ("sha512", "sha384", "sha256", "sha224", "sha1", "md5"):
+    if alg in settings.ALLOWED_CONTENT_CHECKSUMS:
+        _DIGEST_FIELDS.append(alg)
+
+# All available digest fields ordered by relative frequency
+# (Better average-case performance in some algorithms with fallback)
+_COMMON_DIGEST_FIELDS = []
+for alg in ("sha256", "sha512", "sha384", "sha224", "sha1", "md5"):
+    if alg in settings.ALLOWED_CONTENT_CHECKSUMS:
+        _COMMON_DIGEST_FIELDS.append(alg)
+
+# Available, reliable digest fields ordered by algorithm strength.
+_RELIABLE_DIGEST_FIELDS = []
+for alg in ("sha512", "sha384", "sha256"):
+    if alg in settings.ALLOWED_CONTENT_CHECKSUMS:
+        _RELIABLE_DIGEST_FIELDS.append(alg)
+
+# Digest-fields that are NOT ALLOWED
+_FORBIDDEN_DIGESTS = set(ALL_KNOWN_CONTENT_CHECKSUMS).difference(
+    set(settings.ALLOWED_CONTENT_CHECKSUMS)
+)


ty for all this.

pulpcore/constants.py

bmbouter · 2020-09-10T21:08:27Z

pulpcore/tests/unit/serializers/test_content.py

@@ -2,6 +2,8 @@

 import mock
 from pulpcore.app.serializers import ArtifactSerializer
+from pulpcore.app import models


The test suite is expected to be run from a third party box and may not have pulpcore packages installed on it, or if they are, they may not be configured the same way. In those environments this import will fail or return incorrect data.

To resolve this, I think do two things:

Have the test suite expect that ALLOWED_CONTENT_CHECKSUMS = {"sha1", "sha224", "sha256", "sha384", "sha512"}. This is the same approach used by the import/export tests which require the user to set settings to specific values for the test suite to pass.

Have travis fulfill that requirement by adding in the var here:

pulpcore/.travis/install.sh

Line 78 in ab3e4eb

pulp_settings: {"allowed_export_paths": ["/tmp"], "allowed_import_paths": ["/tmp"]}

This last thing would need to go into the plugins template_config.yaml to survive regenerating the ci files.

Also i just realize that this is a unit test. And i fail to see why it should be ok to import ArtifactSerializer but not Artifact.
Is there any context missing?

Updated template_config.yml and regen'd. Turns out you cannot use sets and dynaconf together; tweaked ALLOWED_CONTENT_CHECKSUMS to be a list, explicitly used a set() around it wherever I needed to use difference/intersection.

Also, yes, as noted by @mdellweg , these are unit-tests not functional-tests, so it's ok to rely on being able to import directly (as they all do)

ggainey · 2020-09-11T19:58:23Z

This code looks great to me. Impressive turnaround time on this.
Question: would it be possible to test this out by defining ALLOWED_CONTENT_CHECKSUMS in https://github.com/pulp/pulpcore/blob/master/.travis/settings.py.j2?

@daviddavis , I have...no insight here. I've done a lot of local testing by setting in /etc/pulp/settings.py.

You would add that to the plugins template_config and regenerate the travis directory.

Like here: https://github.com/pulp/pulpcore/blob/master/template_config.yml#L30

And done!

mdellweg · 2020-09-14T10:51:41Z

I was finally (sort of) able to pass the functional testsuite of pulpcore (pagination fails due to remaining repos from earlier tests of the same run) in fips environment.

bmbouter · 2020-09-14T14:49:37Z

@ggainey is there a functional test that can assert that a sync'd content unit does not have 'md5' set?

ggainey · 2020-09-14T14:55:07Z

@ggainey is there a functional test that can assert that a sync'd content unit does not have 'md5' set?

I only have unit-tests in this commit. Hm.

bmbouter · 2020-09-14T18:50:29Z

@ggainey is there a functional test that can assert that a sync'd content unit does not have 'md5' set?

I only have unit-tests in this commit. Hm.

I think you could likely extend this test to check on the md5 aspects of things pretty easily. https://github.com/pulp/pulpcore/blob/master/pulpcore/tests/functional/api/test_crd_artifacts.py#L52-L57 wdyt?

ggainey · 2020-09-15T19:02:49Z

@ggainey is there a functional test that can assert that a sync'd content unit does not have 'md5' set?

I only have unit-tests in this commit. Hm.

I think you could likely extend this test to check on the md5 aspects of things pretty easily. https://github.com/pulp/pulpcore/blob/master/pulpcore/tests/functional/api/test_crd_artifacts.py#L52-L57 wdyt?

Fine idea - added testing for "did you get md5 back from a valid upload?" and a new test for "trying to upload while specifying md5 as the checksum should fail" (both assuming md5 is not present in ALLOWED_CONTENT_CHECKSUMS)

pulpcore/tests/functional/api/test_crd_artifacts.py

bmbouter · 2020-09-15T19:54:39Z

I'm LGTM once the Artifact.__init__ docstring is added. Thank you @ggainey !

bmbouter

Thank you for this very excellent piece of work. I really appreciate how you brought this together so quickly. 👍 🥇 🚀

settings.ALLOWED_CONTENT_CHECKSUMS now drives the other checksum-related fields of Artifact (DIGEST_FIELDS, COMMON_DIGEST_FIELDS, RELIABLE_DIGEST_FIELDS) closes #5216

ggainey force-pushed the 5216_fips_checksums branch 2 times, most recently from b556eac to 1e04525 Compare September 9, 2020 00:23

mdellweg requested changes Sep 9, 2020

View reviewed changes

ggainey force-pushed the 5216_fips_checksums branch from 1e04525 to 432a759 Compare September 9, 2020 17:53

ggainey force-pushed the 5216_fips_checksums branch from 432a759 to f69103e Compare September 9, 2020 18:02