Skip to content

Repository tools #170

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

Merged
merged 213 commits into from
Mar 6, 2014
Merged

Repository tools #170

merged 213 commits into from
Mar 6, 2014

Conversation

vladimir-v-diaz
Copy link
Contributor

repository_tool.py
configurable cryptography
ed25519
Let pbkdf2 iterations vary

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Refactored the majority of affected modules.  Added optimized version of the reference implementation of ed25519.
Add new schema to formats.py and simplify input validation in keys.py

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Fix docstrings for whitespace consistency.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Fix docstrings for whitespace consistency.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Add missing doctest and minor edits.

Unverified

The signature in this commit could not be verified. Someone may be trying to trick you.
Add separate 'tuf.conf.py' options for key types.
Modify the object name of a schema so that FormatError error messages are clearer about which schema is expected.
test cases updated with configurable crypto changes.
Update docstring whitespace.
New ed25519 schemas to validate public and seed keys, and signatures.
Validate arguments using the newly added ed25519 schemas to formats.py.
test_verify_signature() incomplete.
Remove extra whitespace in __init__.py
Minor change to function names and argument validation.
Santiago's request:  The key-removal methods in repository_tool.py should raise an exception if the key argument has not been previously loaded.  They previously returned silently if the key was not found.
@trishankkarthik
Copy link
Contributor

Documentation says the attribute in tuf.repository_tool.Repository.write() is "consistent_snapshots", but the code says it's "consistent_snapshot" instead.

Fix write() parameter in the Consistent Snapshots section.
consistent_snapshots -> consistent_snapshot
@trishankkarthik
Copy link
Contributor

Cryptic error message when accidentally loaded public instead of private key. I think it's better to say "Sorry, this is not a private key."

# the top-level roles, but no targets.
repository = load_repository("path/to/repository/")

# Get a list of file paths in a directory, even those in sub-directories.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't recursive_walk=True then?

@vladimir-v-diaz
Copy link
Contributor Author

repository.targets.load_signing_key(targets_public)
Traceback (most recent call last):
File "", line 1, in
File "/home/vlad/projects/tuf/tuf/repository_tool.py", line 677, in
load_signing_key
raise tuf.Error(message)
tuf.Error: The private key is unavailable.

I guess the "unavailable" part can be interpreted multiple ways?
Changing it to "This is not a private key."

On Mon, Feb 24, 2014 at 10:55 AM, Trishank Karthik Kuppusamy <
[email protected]> wrote:

Cryptic error message when accidentally loaded public instead of private
keyhttps://github.com/theupdateframework/tuf/blob/77dfbc34bcf2dad729bc5df9409fb331d1e7d597/tuf/repository_tool.py#L673-677
.

Reply to this email directly or view it on GitHubhttps://github.com//pull/170#issuecomment-35900329
.

repository = load_repository("path/to/repository/")

# Get a list of file paths in a directory, even those in sub-directories.
# This must be relative to an existing directory in the repository, raise an exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constraint isn't actually enforced, is it? I can point it to a targets directory that is not relative to the repository, and it seems to work just fine, although I'm sure downloading the targets themselves will fail later due to mismatch of expectations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is enforced by add_target() and add_targets():

>>> target_files = repository.get_filepaths_in_directory('repository/metadata')
>>> repository.targets.add_targets(target_files)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/vlad/projects/tuf/tuf/repository_tool.py", line 1786, in add_targets
    raise tuf.Error(message)
tuf.Error: '/home/vlad/projects/tuf/tuf/repository/metadata/timestamp.json' is not under the Repository's targets directory: '/home/vlad/projects/tuf/tuf/repository/targets'
>>> 

Not sure if repository maintainers might want this to be a general-purpose method. For example, getting file paths in non-target directories. Should it also raise an exception here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Mon, Feb 24, 2014 at 11:38 AM, Vladimir Diaz [email protected]:

Not sure if repository maintainers might want this to be a general-purpose
method. For example, getting file paths in non-targets directory. Should it
also raise an exception here?

Not a problem here if a potential mistake is (certainly) caught later

Lines 204-213: Update comments for get_filepaths_in_directory() and add_targets() examples.
The previous exception raised when a non-signing key is loaded may be misinterpreted:
tuf.Error: The private key is unavailable.

Changed to: This is not a private key.
Add Table of Contents.
Add documentation for the Consistent Snapshots section.
Properly strip (again) the digest prepended to 'digest.filename' files.
The required '+1' appears to have been accidentally deleted in a recent commit:
298f52d#diff-59d384d80d746c800b16c8387756c0ccL2750
Thanks to Santiago for locating the bug.
Update keys.py and pycrypto_keys.py following Monzur's code review.
Update affected modules.
Add purpose section header.
Consistent snapshot section: file attributes (archive, hidden, access?) -> file hash.
Address issue #179
Lines 800, 803: (root|timestamp).digest.json -> digest.(root|timestamp).json
Rename the unused 'json_object' variable in util.py.

Roles are allowed to share verification keys.  Update repository_tool.py so that the targets role can successfully load an already recognized key when loading a repository.  Reported by Santiago.
@vladimir-v-diaz
Copy link
Contributor Author

Santiago reported that the targets.json role prevented a duplicate key from being loaded, whereas all other roles allowed repeated keys. This issue was fixed in 1a17ac9, but repository_tool.py should also at least log and/or print a message alerting the user.
Note: In practice, multiple roles may share keys (e.g., hashed bin delegations with online keys).

repository_tool.py methods that add keys to roles should also log a warning if it detects a shared key.
Add a console handler, and a function to disable it, to repository_tool.py.
Update _delete_obsolete_metatadata() docstring and comments in repository_tool.py.
Update section on supported cryptographic signatures.
Add some whitespace to improve readability.
@SantiagoTorres
Copy link
Member

All seems to work properly, the log warning messages with repeated keys also works.
Guess we are ready to merge.

Minor updates to comments of the previous repository_tool.py commit.
Update repository_tool-diagram.png to list disable_console_log_messages().
Rename disable_console_messages().
vladimir-v-diaz added a commit that referenced this pull request Mar 6, 2014
Thanks given to Trishank, Santiago, Justin, Zane, Monzur, Lai, Dennis, and the appsec students for reviewing & testing the changes made in this pull request.
@vladimir-v-diaz vladimir-v-diaz merged commit a479a48 into develop Mar 6, 2014
@trishankkarthik
Copy link
Contributor

Congrats!

On Wed, Mar 5, 2014 at 8:35 PM, Vladimir Diaz [email protected]:

Merged #170 #170.

Reply to this email directly or view it on GitHubhttps://github.com//pull/170
.

@vladimir-v-diaz vladimir-v-diaz deleted the repository-tools branch March 6, 2014 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants