Http backend #61

njgheorghita · 2018-07-02T23:32:12Z

What was wrong?

URI backend needed to serve content-addressed github URIs

How was it fixed?

Wrote backend that serves github URIs conforming to

https://raw.githubusercontent.com/user/repo/commit_hash/path/to/manifest.json

Cute Animal Picture

njgheorghita · 2018-10-14T20:42:12Z

@pipermerriam ping for review. The scheme i'm using here isn't quite exactly, what's discussed in the ERC 1319](ethereum/EIPs#1319), I'm not sure if that's a problem or not. IMO just using the raw.githubusercontent.com authority is simpler than creating/fetching blobs as suggested in the spec. Any thoughts? Obvs, i'd like to conform to the spec wherever possible, but this is also an area that imo doesn't require strict conformity.

I also thought about requiring a content hash fragment suffixing the github uri . . . i.e.
https://raw.githubusercontent.com/ethpm/ethpm-spec/481739f6138907db88602558711e9d3c1301c269/examples/owned/contracts/Owned.sol#bfdea1fa5f33c30fee8443c5ffa1020027f8813e0007bb6f82aaa2843a7fdd60
for an added layer of security, but I recently removed that since the commit-hash is already in the uri, didn't seem necessary to have two content hashes

pipermerriam · 2018-10-16T05:11:35Z

@njgheorghita on not including the hash in the URL: I don't think this is ok. While I agree that it doesn't allow for arbitrary modification of the resouce by the user, it doesn't provide protection against github serving up incorrect content. I acknowledge this probably isn't a concern we're likely to see realized, but I'd prefer we do it correctly and both require the hash as well as verifying the payload we receive matches said hash.

Which makes me realize that we should be doing the same with the IPFS gateway backends (and probably generically via any of the IPFS backends) just to be doubly sure that the content we receive is indeed the content that should have been at the address.

njgheorghita · 2018-10-16T21:56:32Z

@pipermerriam Makes sense, added the requirement for a valid content hash to the end of github uris. And added a validation to IPFS backend that the hash of the retrieved contents matches the content hash in the uri

pipermerriam

I evidently never committed these two comments.

pipermerriam · 2018-10-16T05:14:43Z

tests/ethpm/backends/test_http_backends.py

+@pytest.mark.parametrize(
+    "uri",
+    (
+        "https://raw.githubusercontent.com/ethpm/ethpm-spec/3945c47dedb04930ee12c0281494a1b5bdd692a0/examples/owned/1.0.0.json",  # noqa: E501


Worth pointing out that this test will fail occasionally in CI since it makes network requests. I think that's ok for now, but it might be worth leaving a note in the test indicating as much.

pipermerriam · 2018-10-16T05:17:59Z

tests/ethpm/utils/test_uri_utils.py

+        ("https://raw.githubusercontent.com", False),
+        # valid github urls
+        ("http://raw.githubusercontent.com/any/path", True),
+        ("https://raw.githubusercontent.com/any/path", True),


Allowing this will result in people not using content addressed URIs, which will result in people not correctly linking to commit based files which will lead to people using URIs like https://raw.githubusercontent.com/ethpm/ethpm-spec/master/examples/owned/1.0.0.json which will lead to people getting content that isn't guaranteed to remain static and I don't think that's something we want to allow.

pipermerriam · 2018-10-17T16:58:52Z

docs/uri_backends.rst

+
+.. py:method:: BaseURIBackend.can_resolve_uri(uri)
+
+   Returns a bool indicating whether or not this backend is capable of resolving the given URI to a manifest.


No need to cleanup now but:

Wrong ?tense?. I think the official term is "imperitive present tense". See: Add BeaconStateMachine outline ethereum/py-evm#1373 (comment)

These are more suited for API docs and thus can be done with autodoc and docstrings. Take a look at how @cburgdorf has structured the documentation in py-evm

👍 autodoc, moving all (or as many) of the docs over to autodoc is next on my todo list

pipermerriam · 2018-10-17T17:03:43Z

docs/uri_backends.rst

+- Go to the target manifest in your browser.
+- Press ``y`` to generate the permalink in the address bar.
+- Replace ``"github"`` with ``"raw.githubusercontent"``, and remove the ``"blob"`` namespace from the URI.
+- Suffix the URI with ``#`` followed by the ``keccak`` hash of the bytes found at the Github URI.


Not for this PR, but it'd be pretty cool to provide a utility on the GithubOverHTTPSBackend that did this automatically with a uri and supported a handful of formats.

>>> GithubOverHTTPSBackend.get_permalink_with_content_hash('https://github.com/ethpm/ethpm-spec/blob/master/examples/owned/1.0.0.json') "https://raw.githubusercontent.com/ethpm/ethpm-spec/3945c47dedb04930ee12c0281494a1b5bdd692a0/examples/owned/1.0.0.json#0x01cbc2a69a9f86e9d9e7b87475e2ba2619404dc8d6ee3cb3a8acf3176c2cace1"

Should be able to:

parse the repository name and file path and branch

use the github API to pull the latest commit hash

generate the raw.github.com URL

pull the file and compute the keccak of the contents.

This would be something well defined enough that it could be bountied.

pipermerriam · 2018-10-17T17:05:05Z

ethpm/backends/http.py

+        return False
+
+    def fetch_uri_contents(self, uri: str) -> bytes:
+        http_uri, validation_hash = uri.split("#")


You should use urllib.parse.urlpase and pull the fragment from that return value.

pipermerriam · 2018-10-17T17:06:09Z

ethpm/backends/ipfs.py

+        validation_hash = generate_file_hash(contents)
+        if validation_hash != ipfs_hash:
+            raise ValidationError(
+                "Hashed IPFS contents retrieved from uri: {0} "


If you're python 3.6+ you can start using f-strings for formatting: https://realpython.com/python-f-strings/

pipermerriam · 2018-10-17T17:07:29Z

ethpm/validation.py

+        raise ValidationError(
+            "Invalid content-addressed URI. "
+            "Validation hash:{0} does not match the hash of URI contents: {1}.".format(
+                decoded_validation, hashed_contents


Probably good to output these in their hex encoded formats for friendlier error messages.

pipermerriam · 2018-10-17T17:08:51Z

tests/conftest.py

@@ -104,6 +104,12 @@ def _get_factory(package, factory_name):
    return _get_factory


+@pytest.fixture
+def owned_contract():
+    with open(V2_PACKAGES_DIR / "owned" / "contracts" / "Owned.sol") as file_obj:


I think you can also do (V2_PACKAGES_DIR / "owned" / "contracts" / "Owned.sol").open() but I'm undecided on whether I think it's a good pattern. The outer parenthesis wrapping is sort of awkward. Not advocating change, just bringing up my thoughts.

Agreed on the parenthesis wrapping being weird, but i'm a fan of pathlib and think the weirdness is worth it

pipermerriam · 2018-10-17T17:11:31Z

tests/ethpm/test_dependencies.py

@@ -43,7 +43,7 @@ def test_get_dependency_package(dependencies):

 def test_validate_build_dependencies(dummy_ipfs_backend, piper_coin_manifest):
    result = validate_build_dependency(
-        "standard-token", "ipfs://QmVu9zuza5mkJwwcFdh2SXBugm1oSgZVuEKkph9XLsbUwg"
+        "standard-token", "ipfs://QmVu9zuza5mkJwwcFdh2SXBugm1oSgZVuEKkph9XLsbUwg#0x123"


What's this doing?

😬 nothing 😬

njgheorghita force-pushed the http-backend branch from 5c0d25a to b0e11e6 Compare July 2, 2018 23:32

njgheorghita added Ready For Review and removed Ready For Review labels Jul 2, 2018

njgheorghita force-pushed the http-backend branch 6 times, most recently from 4ab4671 to b0534f6 Compare July 13, 2018 16:16

njgheorghita force-pushed the http-backend branch 6 times, most recently from ae865bf to 40d2703 Compare July 18, 2018 16:58

njgheorghita added the Ready For Review label Jul 18, 2018

njgheorghita requested review from davesque and pipermerriam July 18, 2018 17:01

njgheorghita force-pushed the http-backend branch 3 times, most recently from 025d161 to 8a06f33 Compare July 26, 2018 13:57

njgheorghita force-pushed the http-backend branch 9 times, most recently from c511a6b to 2482b30 Compare October 14, 2018 20:24

njgheorghita force-pushed the http-backend branch 3 times, most recently from d7e9ea0 to b5c6c98 Compare October 14, 2018 20:32

njgheorghita force-pushed the http-backend branch 2 times, most recently from 6a4d4f2 to 04c78eb Compare October 16, 2018 15:30

njgheorghita added 4 commits October 16, 2018 09:30

Prototype uri backend

aaf3e05

Implement fetch ipfs resources via URI backend

c24a878

Write local ipfs node backend and remove http reqs from tests

2e7d1fa

Write github uri backend

a5664fa

njgheorghita force-pushed the http-backend branch 4 times, most recently from 6cc6b74 to a2fe704 Compare October 16, 2018 21:54

njgheorghita force-pushed the http-backend branch 2 times, most recently from 5277de2 to ac6b813 Compare October 16, 2018 22:04

pipermerriam reviewed Oct 17, 2018

View reviewed changes

pipermerriam approved these changes Oct 17, 2018

View reviewed changes

Add ipfs hash validation

1f977b5

njgheorghita force-pushed the http-backend branch from ac6b813 to 1f977b5 Compare October 17, 2018 19:15

njgheorghita merged commit 19cc54a into ethpm:master Oct 17, 2018

njgheorghita deleted the http-backend branch October 17, 2018 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http backend #61

Http backend #61

njgheorghita commented Jul 2, 2018 •

edited

Loading

njgheorghita commented Oct 14, 2018

pipermerriam commented Oct 16, 2018

njgheorghita commented Oct 16, 2018

pipermerriam left a comment

pipermerriam Oct 16, 2018

pipermerriam Oct 16, 2018

pipermerriam Oct 17, 2018

njgheorghita Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

pipermerriam Oct 17, 2018

njgheorghita Oct 17, 2018

pipermerriam Oct 17, 2018

njgheorghita Oct 17, 2018


		.. py:method:: BaseURIBackend.can_resolve_uri(uri)

		Returns a bool indicating whether or not this backend is capable of resolving the given URI to a manifest.

Http backend #61

Http backend #61

Conversation

njgheorghita commented Jul 2, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

njgheorghita commented Oct 14, 2018

pipermerriam commented Oct 16, 2018

njgheorghita commented Oct 16, 2018

pipermerriam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njgheorghita commented Jul 2, 2018 •

edited

Loading