Removed duplicate functions in TUF

[tobithegreat]

Signed-off-by: Oluwatobi Popoola popoolatobi@gmail.com

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes issue #: 656

Description of the changes being introduced by the pull request:
Removed functions that did not expand functionality to equivalent functions in securesystemslib.

Please verify and check that the pull request fulfills the following
requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[awwad]

@tobithegreat, would you like to look through and see where the tests failed as a result of this PR?

The automated testing / continuous integration we use helps identify possible issues with new pull requests. You can see on this page that the builds running the tests failed, and click Details (I'd look at the Travis results to start.) to see what happened. Sometimes code changes necessitate test changes -- for example, there may be test functions that test pieces of code that are no longer necessary and were removed -- and sometimes test failures indicate that there is a mistake and something was broken.

It should help to know that you can run the TUF tests locally by running python aggregate_tests.py from the tests directory in your local repository. You can run individual test modules similarly: python test_updater.py, for example, from inside the tests directory.

[tobithegreat]

Thanks, will do!

[tobithegreat]

@awwad sorry to bother, but I'm having trouble running the tests locally. Upon running test aggregate_tests.py from within the tests directory, I receive an ImportError. Stacktrace below:

Oluwatobis-MBP:tests oluwatobipopoola$ python aggregate_tests.py 
Traceback (most recent call last):
  File "aggregate_tests.py", line 93, in <module>
    suite = unittest.TestLoader().loadTestsFromNames(test_modules_to_run)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/loader.py", line 130, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/loader.py", line 91, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/Users/oluwatobipopoola/Desktop/tuf/tests/test_slow_retrieval_attack.py", line 59, in <module>
    import tuf.log
ImportError: No module named tuf.log

I suspect this has to do with my file hierarchy, or some directory referencing error?

[awwad]

Have you installed tuf? pip install -r dev-requirements.txt (from main repo directory)

…

On Sat, Nov 17, 2018 at 07:51 Oluwatobi Popoola ***@***.***> wrote: @awwad <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_awwad&d=DwMFaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=TDcSTJRuNI5ZvNxAx0mxnCxUMlZo8yHmkfDrHrT2en0&s=WxkP4YmqJt0japoPBt4bVtrTMgms7uvOO-upCzebUmM&e=> sorry to bother, but I'm having trouble running the tests locally. Upon running test aggregate_tests.py from within the tests directory, I receive an ImportError. Stacktrace below: Oluwatobis-MBP:tests oluwatobipopoola$ python aggregate_tests.py Traceback (most recent call last): File "aggregate_tests.py", line 93, in <module> suite = unittest.TestLoader().loadTestsFromNames(test_modules_to_run) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/loader.py", line 130, in loadTestsFromNames suites = [self.loadTestsFromName(name, module) for name in names] File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/loader.py", line 91, in loadTestsFromName module = __import__('.'.join(parts_copy)) File "/Users/oluwatobipopoola/Desktop/tuf/tests/test_slow_retrieval_attack.py", line 59, in <module> import tuf.log ImportError: No module named tuf.log I suspect this has to do with my file hierarchy? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_theupdateframework_tuf_pull_804-23issuecomment-2D439614379&d=DwMFaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=TDcSTJRuNI5ZvNxAx0mxnCxUMlZo8yHmkfDrHrT2en0&s=y-w9M_QeqyMwcy7oB8AMvqIGqHOk4YBYiukfDNdsjMM&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMpkOA8-2DLaEbwToelOOuOlKBjuiLdVCEks5uwAZQgaJpZM4YfmrW&d=DwMFaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=TDcSTJRuNI5ZvNxAx0mxnCxUMlZo8yHmkfDrHrT2en0&s=MCohhHOGqkoOEtKUas7DGCwNKHjDpMuefMzWG4U7yc4&e=> .

[tobithegreat]

Have you installed tuf? pip install -r dev-requirements.txt (from main repo directory)
…
On Sat, Nov 17, 2018 at 07:51 Oluwatobi Popoola @.***> wrote: @awwad https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_awwad&d=DwMFaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=TDcSTJRuNI5ZvNxAx0mxnCxUMlZo8yHmkfDrHrT2en0&s=WxkP4YmqJt0japoPBt4bVtrTMgms7uvOO-upCzebUmM&e= sorry to bother, but I'm having trouble running the tests locally. Upon running test aggregate_tests.py from within the tests directory, I receive an ImportError. Stacktrace below: Oluwatobis-MBP:tests oluwatobipopoola$ python aggregate_tests.py Traceback (most recent call last): File "aggregate_tests.py", line 93, in suite = unittest.TestLoader().loadTestsFromNames(test_modules_to_run) File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/loader.py", line 130, in loadTestsFromNames suites = [self.loadTestsFromName(name, module) for name in names] File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/unittest/loader.py", line 91, in loadTestsFromName module = import('.'.join(parts_copy)) File "/Users/oluwatobipopoola/Desktop/tuf/tests/test_slow_retrieval_attack.py", line 59, in import tuf.log ImportError: No module named tuf.log I suspect this has to do with my file hierarchy? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_theupdateframework_tuf_pull_804-23issuecomment-2D439614379&d=DwMFaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=TDcSTJRuNI5ZvNxAx0mxnCxUMlZo8yHmkfDrHrT2en0&s=y-w9M_QeqyMwcy7oB8AMvqIGqHOk4YBYiukfDNdsjMM&e=, or mute the thread https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMpkOA8-2DLaEbwToelOOuOlKBjuiLdVCEks5uwAZQgaJpZM4YfmrW&d=DwMFaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=TDcSTJRuNI5ZvNxAx0mxnCxUMlZo8yHmkfDrHrT2en0&s=MCohhHOGqkoOEtKUas7DGCwNKHjDpMuefMzWG4U7yc4&e= .

Ah, thank you. I had, but on a different computer than the one my local repo was on. Thanks!

[tobithegreat]

This latest commit updates all unit tests to reflect removal of duplication code.

Apparently, one of the AppVeyor build jobs failed.

Environment: PYTHON=C:\Python27, PYTHON_VERSION=2.7, PYTHON_ARCH=32
9 min 18 sec

---

Error:

======================================================================
ERROR: test_download_url_to_tempfileobj_and_lengths (test_download.TestDownload)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\tuf-j90sw\tests\test_download.py", line 137, in test_download_url_to_tempfileobj_and_lengths
    download.safe_download, self.url, self.target_data_length + 1)
  File "C:\Python27\lib\unittest\case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "c:\projects\tuf-j90sw\tuf\download.py", line 115, in safe_download
    return _download_file(url, required_length, STRICT_REQUIRED_LENGTH=True)
  File "c:\projects\tuf-j90sw\tuf\download.py", line 303, in _download_file
    average_download_speed=average_download_speed)
  File "c:\projects\tuf-j90sw\tuf\download.py", line 586, in _check_downloaded_length
    raise tuf.exceptions.SlowRetrievalError(average_download_speed)
SlowRetrievalError: Download was too slow. Average speed: 38.28114266658803 bytes per second.
----------------------------------------------------------------------
Ran 208 tests in 366.551s
FAILED (errors=1, expected failures=1)

Not sure how to deal with this error.

[awwad]

I re-ran the build and it passed. I think that was an unusual build error, and I'll keep an eye out for it if it happens again. Thanks for taking a look. I'll review this PR shortly. :)

[awwad]

The commit history is a bit hard to read:
https://github.com/theupdateframework/tuf/pull/804/commits

See the three upstream-to-local merge commits "Merge branch 'develop' of https://github.com/tobithegreat/tuf into develop..."? Two of them contain no changes, and the other contains changes. Generally, you shouldn't try to push merge commits like that back upstream: it's convoluted and makes the commit history hard to read. (If this isn't in our lab guidelines yet, it will be soon.)

Those merges happen when you pull from the repository. I advise instead fetching (git fetch --all), and then using git rebase <origin>/develop to keep the history clean. I'll give you some tips toward that end.

• For starters, you only really need to fetch new upstream code if you're concerned that it might impact what you're doing. You can instead rebase when you're ready to make a pull request.

• If you want to pull down from the remote repo, you can fetch (git fetch --all), finish up your commit or stash (git stash) what you're working on, then rebase your branch onto the tip of the remote branch (probably git rebase <origin>/develop in your case -- using whatever you've named the upstream theupdateframework repo (git remote -v to see your remotes), and then pop the stash again. If you encounter conflicts, you can fix them this way, in each commit.

• If some upstream-to-local merge commits have already been added with additional changes, as in this case, you can either rebase or, if necessary, squash the commits.

• Another, unrelated tip: it's usually best to start a feature branch when you do something like this. So, for example, before you start adding commits, you could have run git checkout -b remove_duplicate_ssl_funcs before adding your commits.

In this case, here's how I might go about fixing the PR (without creating a new one):

git checkout develop
git rebase tuf/develop
git rebase --continue
git push tobithegreat develop --force

Note that those names are based on this remote listing:

git remote -v
  ...
  tobithegreat	https://github.com/tobithegreat/tuf (fetch)
  tobithegreat	https://github.com/tobithegreat/tuf (push)
  tuf	https://github.com/theupdateframework/tuf (fetch)
  tuf	https://github.com/theupdateframework/tuf (push)

If any of that doesn't make sense to you, please feel free to ask about it!

[awwad]

When that's fixed, this looks good to me. Did you see any other functions that could be replaced by securesystemslib features?

[tobithegreat]

@awwad Wow, thanks for all the feedback! I'll be sure to make these changes.

Also, I updated a few lines in the test cases, that caused errors unrelated to my changes.
For example, on line 422, self.assertEqual(repo_lib.get_target_hash(filepath), target_hash) was changed to self.assertEqual(securesystemslib.util.get_target_hash(filepath), target_hash), since get_target_hash is a function not available in repository_lib.py.

Please let me know if this change was mistaken!

[awwad]

get_target_hash is in repository_lib.py here. If it weren't there, that test would have generated an error. 😄 (So it's important that it be tested in TUF, so long as it remains in TUF....)

However, get_target_hash is an example of another function that is available in securesystemslib, and therefore may not need to be in TUF itself. Would you like to look and see if removing get_target_hash in TUF and swapping in SSL's makes sense?

[tobithegreat]

My mistake! Here is the exact error:

======================================================================
ERROR: test_get_target_hash (test_repository_lib.TestRepositoryToolFunctions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/oluwatobipopoola/Desktop/tuf/tests/test_repository_lib.py", line 422, in test_get_target_hash
    self.assertEqual(repo_lib.get_target_hash(filepath), target_hash)
AttributeError: 'module' object has no attribute 'get_target_hash'

And I'm happy to see if it can be replaced.

[lukpueh]

As @awwad has mentioned, we are currently working on git history lab guidelines. @tobithegreat, it would be awesome if you could take a look at the work-in-progress document and tell us if it is helpful as it is, or if it would need some more information. You can make comments in the corresponding PR (secure-systems-lab/lab-guidelines#14).

[tobithegreat]

Hello again!
Thanks once again for explaining everything in such depth. I'm still learning best git practices, and this was a big help.
I ran the steps you outlined above. My remotes are as follows:

origin	https://github.com/tobithegreat/tuf.git (fetch)
origin	https://github.com/tobithegreat/tuf.git (push)
tuf	https://github.com/theupdateframework/tuf (fetch)
tuf	https://github.com/theupdateframework/tuf (push)

• running git push origin develop --force returned Everything up-to-date. Is the PR more clean now?

[awwad]

No; you can see the git history in the commits page here, with (roughly) the same merge commits. I can't really say what went wrong without more information, though I have guesses.... Can you send me the output of these commands?

• git status
• git log --oneline --decorate --graph --all (You only need to go back far enough to show tuf/develop and 3-4 lines after origin/develop.)

I assume the output of git remote -v is the same; if not, show me that, too.

[tobithegreat]

Output of git status:

On branch develop
Your branch is up to date with 'origin/develop'.

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	tests/ssl_certs/proxy_certs/
	tests/tmp3_z3I6/
	tests/tmp4h_R6v/
	tests/tmp7kFriU/
	tests/tmp9FB297/
	tests/tmpBXCty8/
	tests/tmpDAQS4e/
	tests/tmpDUlfUf/
	tests/tmpF2CMve/
	tests/tmpGymUM5/
	tests/tmpRwnumb/
	tests/tmpTHQYss/
	tests/tmpVnsdRq/
	tests/tmpWshCrP/
	tests/tmp_9OMiX/
	tests/tmpdfZ32c/
	tests/tmphhumHv/
	tests/tmprriis8/
	tests/tmpv6ZqrR/
	tests/tmpyV4HGM/
	tests/tmpzG5YOY/
	virtualenv-15.0.3.tar.gz

nothing added to commit but untracked files present (use "git add" to track)

Some of the output from git log --oneline --decorate --graph --all

*   30f9561 (HEAD -> develop, origin/develop, origin/HEAD) Merge branch 'develop' of https://github.com/tobithegreat/tuf into develop
|\  
| *   4c10eea Merge branch 'develop' of https://github.com/tobithegreat/tuf into develop
| |\  
* | \   4e2294a Merge branch 'develop' of https://github.com/tobithegreat/tuf into develop
|\ \ \  
| |/ /  
|/| /   
| |/    
| * d8f8763 Removed duplicate functions in TUF
* | d33024c Removed duplicate functions in TUF
|/  
| *   05a9aa2 (refs/stash) WIP on develop: fffc533 Merge pull request #803 from theupdateframework/test_delay_increases
| |\  
|/ /  
| * b9e70d2 index on develop: fffc533 Merge pull request #803 from theupdateframework/test_delay_increases
|/  
*   fffc533 Merge pull request #803 from theupdateframework/test_delay_increases
|\  
| * 8866abb (origin/test_delay_increases) test: remove port collison chance and lengthen delays for AppVeyor

[awwad]

git fetch --all
git rebase theupdateframework/develop

[tobithegreat]

Thanks!
I ran those commands, and git push origin develop --force after. It reduced the number of commits in the PR from 5 to 2, although the build is failing tests now.

[awwad]

Good and bad news. 😄

The one test that failed on AppVeyor was a fluke I'm familiar with, so I restarted it and it succeeded.

(A server didn't start up on AppVeyor within the 3s expected window.... I should adjust those subprocess calls to wait for some flexible period of time, but that's not related to this. 😄 )

There are meaningful errors in the Travis-CI build, though. Travis-CI is running pylint and that is complaining that there are calls in developer_tool.py to methods that (now) don't exist. Those calls should also be fixed in this PR, so that's a good build failure. (You can see the lint failures in the Travis-CI build logs.)

When you add that commit (if you want to), make sure to use an expressive commit message.

[tobithegreat]

Finished the commit and fixed the tests, but it was signed off incorrectly. Sorry to ask again, but is there an efficient way to correct the commit without clogging up the history again?

[awwad]

Yes: git commit -s --amend git push origin develop --force

…

On Fri, Nov 30, 2018 at 15:30 Oluwatobi Popoola ***@***.***> wrote: Finished the commit and fixed the tests, but it was signed off incorrectly. Sorry to ask again, but is there an efficient way to correct the commit without clogging up the history again? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_theupdateframework_tuf_pull_804-23issuecomment-2D443330280&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=hmz4JvrTgnHGI7997wkSaDC_mqLo7ECf04VAtaOzw0w&s=viaWHRU5my-gqkPYwXKiNcosHnO3WJKhF4MYV0x5u_g&e=>, or mute the thread <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AMpkOPupr42V7We4OWpCSsZMWueYX7B9ks5u0ZVygaJpZM4YfmrW&d=DwMCaQ&c=slrrB7dE8n7gBJbeO0g-IQ&r=lmb4ogFqgMptuKWQok14rSU0yJ3xUDL7l3ttC3XYeMw&m=hmz4JvrTgnHGI7997wkSaDC_mqLo7ECf04VAtaOzw0w&s=QSmlLPuZL3DTYkZGYE8TDCN8E-AxdZIBPEGmtKTf-xc&e=> .

[tobithegreat]

Ran those commands and force pushed once again. This time is states that the DCO is missing.

Really sorry about this, if there's something very obvious I am not seeing.

[awwad]

((Meta: DCO sign-off requirements for every contributor for every commit are becoming a nuisance.... It'd be nice to be able to satisfy the checks sometimes by just signing off on the merge of the PR.... These sign-off requirements are part of CNCF best practices, though.))

@JustinCappos: this makes for difficult pedagogy. I don't know if it's reasonable to expect students to deal with this level of history editing in git, so I'm curious to know what you think. Note that once there's a commit missing a sign-off, the commit history has to be changed if we want to satisfy the DCO bot, whether by squashing, rebasing, etc. (Edit: I suppose we could instead tell people they have to create a fresh PR, but that's pretty awful.)

[tobithegreat]

Thanks very much for your assistance.
I definitely could have done more reading in the docs to be more prepared to make contributions. I'll be sure to do so in the future.

[lukpueh]

Closing in favor of an updated version of this PR in #919. Thanks for the efforts!