Addition of ASN.1 format with BER/DER encoding to TUF #3
Conversation
| @@ -0,0 +1,54 @@ | |||
| { | |||
There was a problem hiding this comment.
Metadata files used by the tests are usually saved in tests/repository_data/repository/*. Is there a reason why they are saved in repository_data/*?
There was a problem hiding this comment.
The files in tests/repository_data/repository/ constitute the metadata files that represent a single consistent repository. These new files are not related to that specific repository. I didn't want them to be confused, and since this is temporary, tests/repository_data felt adequate. I would like ultimately to use test files from several repositories, each in a directory in repository_data corresponding to their repository name, with all metadata files required for that repository, so that we can test them in full with the map file.
There was a problem hiding this comment.
(Do you think it would be better to just stick them in with the 'repository' stuff?)
There was a problem hiding this comment.
I was thinking of having separate directories (e.g., tests/repository_data/repository, tests/repository_data/director) to test the map file. In PR #430 separate directories are created on the client side, but multiple server instances (on different ports) are started over the same tests/repository_data/repository data, which would be more ideal.
There was a problem hiding this comment.
correction: "which is not ideal... we should also have separate repositories on the server side"
tuf/sig.py
Outdated
| @@ -60,6 +60,21 @@ def get_signature_status(signable, role=None, repository_name='default'): | |||
| all the keys that are valid, invalid, unrecognized, unauthorized, or | |||
| generated using an unknown method. | |||
|
|
|||
| PLEASE NOTE that when running TUF with DER metadata (setting | |||
| tuf.conf.METADATA_FORMAT == 'DER'), this function can only be called | |||
There was a problem hiding this comment.
According to code below, DER should be lower case.
| }, | ||
| "roles": [ | ||
| { | ||
| "backtrack": true, |
There was a problem hiding this comment.
This attribute was renamed to "terminating". This set of metadata will have to be regenerated if merging with TUF's repository.
There was a problem hiding this comment.
That's true. One of sadly many things that will have to be adjusted when the merge happens.
| @@ -0,0 +1,86 @@ | |||
| { | |||
There was a problem hiding this comment.
Root metadata is now always written as if if consistent snapshots = True. Therefore, expect to have 1.uptane_director_root.json, 2.uptane_director_root.json for this repository.
There was a problem hiding this comment.
That was a change made after the fork occurred, and will be another thing to unite in the merge.
| @@ -0,0 +1,26 @@ | |||
| { | |||
There was a problem hiding this comment.
Is this valid metadata? Not all metadata is listed in the snapshot metadata, at least as named on disk. For example, why don't I see targets_simpler.json listed anywhere?
There was a problem hiding this comment.
targets_simpler.json isn't part of a complete test repository's data, no; it's for an individual test prodding just targets.json.
The other files are parts of repositories and are prefixed with the repository of which they're a part:
- uptane_director_root.json
- uptane_director_snapshot.json
- ...
- uptane_mainrepo_root.json
- ...
Should they remain (if we decide Uptane gets that kind of privilege - instead of generalizing a bit), we'd want them in folders in repository_data like this:
- repository_data/uptane_director:
- root.json
- ...
- repository_data/uptane_imagerepo:
- root.json
- ...
- repository_data/repository: <--- may or may not want to rename to 'simple', 'default', etc.
- root.json
- ...
- repository_data/der_repository:
- root.der
- ...
| except Exception as exception: | ||
| raise tuf.InvalidMetadataJSONError(exception) | ||
|
|
||
| else: | ||
| # Ensure the loaded 'metadata_signable' is properly formatted. Raise | ||
| # 'tuf.FormatError' if not. | ||
| tuf.formats.check_signable_object_format(metadata_signable) | ||
| tuf.formats.check_signable_object_format(metadata_signable) # TODO: <~> This needs some way of knowing that it has a piece of DER-signed data so that it can re-encode 'signed' as DER before checking it. |
There was a problem hiding this comment.
The use of in-line comments seems excessive. According to our code style guidelines, they should be used sparingly: https://github.com/secure-systems-lab/code-style-guidelines#inline-comments
There was a problem hiding this comment.
I agree. It's a habit I have for temporary comments that I need to resolve before the code reaches merge-worthy state. Same goes for the '<~>' mark: generally, these are things I do not want to allow in, and would grep for.
tuf/repository_lib.py
Outdated
|
|
||
| elif tuf.conf.METADATA_FORMAT == 'der': | ||
| written_metadata_content = asn1_codec.convert_signed_metadata_to_der( | ||
| metadata_signable) # TODO: <~> CURRENTLY WORKING HERE |
There was a problem hiding this comment.
I guess we can remove this now.
tuf/repository_lib.py
Outdated
| written_metadata_content = asn1_codec.convert_signed_metadata_to_der( | ||
| metadata_signable) # TODO: <~> CURRENTLY WORKING HERE | ||
| else: | ||
| raise tuf.Error('Unsupported metadata format in configuration. Unable to ' |
There was a problem hiding this comment.
Might be helpful to state exactly which configuration file. tuf.conf.py? Note: It's now named tuf.settings.py
There was a problem hiding this comment.
True. Fixed and will be in future push.
| @@ -54,6 +54,10 @@ | |||
| # be saved to 'LOG_FILENAME' | |||
| LOG_FILENAME = 'tuf.log' | |||
|
|
|||
| # This is the format/encoding of metadata that TUF will use. The options are | |||
| # 'json' and 'der'. DER is an encoding of ASN.1 used for the Uptane project. | |||
| METADATA_FORMAT = 'json' # 'der' | |||
There was a problem hiding this comment.
Do we expect the format of all metadata on a repository to be equal? For instance, must it all be JSON, or all DER? No mixing of formats?
There was a problem hiding this comment.
Yes; currently, the formats of all metadata files must be consistent.
They're all JSON or all DER. (Except the map (still pinned.json in this
fork) file, which is never signed and sent anywhere. That's just always
JSON in this codebase.)
There was a problem hiding this comment.
We should verify with Justin that this is what we eventually want. I also made this assumption with YAML, but only because it was simpler to implement. If we eventually want to allow mixing of metadata formats, we'll have to modify the design a bit.
There was a problem hiding this comment.
Agreed. I told him that that was the direction I'd develop in - at any one time, all metadata would be in the same format. We didn't talk about what the eventual state would be, though.
| def test_1_root_partial_convert(self): | ||
| # Test 1: only_signed conversion PyDict -> ASN1 BER of Root | ||
| partial_der_conversion_tester( | ||
| 'tests/repository_data/repository/metadata/root.json', self) |
There was a problem hiding this comment.
In the unit tests, we usually move repository data to a temporary directory and then work with it from there. First moving it to a temp directory prevent the original files from being changed in unexpected ways. If the repository data is only being read here, I guess it's okay to use the original files.
… some critical-todo markers; other minor comment clarifications
| <Side Effects> | ||
| None. | ||
|
|
||
| <Return> |
There was a problem hiding this comment.
We moved util.py to securesystemslib, so this function should ideally return any DER file. In TUF's case, we'd most likely always use this function to fetch a file in tuf.formats.SIGNABLE_SCHEMA format.
There was a problem hiding this comment.
I'm not sure that's possible in the case of DER, unfortunately. You have to provide an ASN1 definition for the metadata in order to decode DER, and it's pretty rigid.
When you decode using pyasn1, you provide the expected ASN.1 class of the result. Here's a simple example, decoding a token (nonce-like things for the Timeserver-Primary-Secondary workflow):
decoded = pyasn1.codec.der.decoder.decode(
token_der,
uptane_asn1_definitions.Token())| # Decode the DER into an abstract ASN.1 representation of its data, | ||
| # then convert that into a basic Python dictionary representation of the | ||
| # data within. | ||
| #return asn1_codec.convert_signed_der_to_dersigned_json(der_data) |
There was a problem hiding this comment.
Commented line of code.
tuf/util.py
Outdated
| None. | ||
|
|
||
| <Returns> | ||
| Deserialized object. For example, a dictionary. |
There was a problem hiding this comment.
asn1_codec.convert_signed_der_to_dersigned_json() returns a tuf.formats.signable_schema object. Might be clearer to say that instead of just "dictionary". Consider the name asn1_codec.convert_signed_der_to_tuf_signable(), or something along those lines.
There was a problem hiding this comment.
- You're right that the doc should be better. In the next round of pushes, it'll be:
"""
Deserialized object, conforming to tuf.formats.SIGNABLE_SCHEMA.
The signatures contained in the returned object, if any, will have been
unchanged. If, for example, the signatures were over a DER object,
they will remain that way, even though the 'signed' portion will no longer
be in DER.
""""convert_signed_der_to_dersigned_json" is meant to indicate that the Python dictionary returned retains a signature over the DER format ("dersigned"). Perhaps it's too ambitious a name. :P
| @@ -241,7 +244,7 @@ def __init__(self, updater_name): | |||
| # Load pinned.json, which is required per TAP #4 and determines which | |||
| # which targets should be sought from which repository(/ies). | |||
| self._load_pinned_metadata(os.path.join( | |||
There was a problem hiding this comment.
Note: In the pull request that implements TAP 4 (the map file), the path to the pinned file is given as an argument to MultiRepoUpdater(). See theupdateframework#430 (comment)
We can decide on which to approach to use once we merge.
…r_file, and tuf.util.load_file. Removal of some extraneous comments. Removal of urgent flag on TODO in tuf.keys.verify_signature.
…itive; add _ to name of private function ensure_valid_metadata_type_for_asn1
| # being assumed that the key values are hex strings ('f9ac1325...') but an | ||
| # RSA public key value is e.g. '-----BEGIN PUBLIC | ||
| # KEY------\nMIIBojANBgk...\n...' | ||
| def test_1_root_partial_convert(self): |
There was a problem hiding this comment.
All these test cases fail with a PyAsn1Error exception. It seems to complain about an incompatible value in the keyid field of the signature.
======================================================================
ERROR: test_11_root_uptane_partial_convert (__main__.TestASN1Conversion)
Test 11: only_signed conversion PyDict -> ASN1 BER of Root
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/test_asn1_codec.py", line 66, in test_11_root_uptane_partial_convert
'tests/repository_data/uptane_mainrepo_root.json', self)
File "tests/test_asn1_codec.py", line 118, in partial_der_conversion_tester
role_signable_pydict, only_signed=True)))
File "/home/vlad/projects/vladforks/tmp/tuf/tuf/asn1_codec.py", line 248, in convert_signed_metadata_to_der
asn_signed = relevant_asn_module.get_asn_signed(json_signed) # Python3 breaks here.
File "/home/vlad/projects/vladforks/tmp/tuf/tuf/encoding/rootmetadata.py", line 26, in get_asn_signed
snapshotPublicKeyid, targetsPublicKeyid, rootMetadata)
File "/home/vlad/projects/vladforks/tmp/tuf/tuf/encoding/rootmetadata.py", line 123, in set_keys
tag.tagFormatSimple, 1))
File "/home/vlad/projects/vladforks/tmp/tuf/env/local/lib/python2.7/site-packages/pyasn1/type/univ.py", line 1945, in __setitem__
self.setComponentByName(idx, value)
File "/home/vlad/projects/vladforks/tmp/tuf/env/local/lib/python2.7/site-packages/pyasn1/type/univ.py", line 2037, in setComponentByName
self._componentType.getPositionByName(name), value, verifyConstraints, matchTags, matchConstraints
File "/home/vlad/projects/vladforks/tmp/tuf/env/local/lib/python2.7/site-packages/pyasn1/type/univ.py", line 2497, in setComponentByPosition
raise error.PyAsn1Error('Component value is tag-incompatible: %r vs %r' % (value, componentType))
PyAsn1Error: Component value is tag-incompatible: OctetString(tagSet=TagSet((), Tag(tagClass=128, tagFormat=0, tagId=1)), hexValue='94c836f0c45168f0a437eef0e487b910f58db4d462ae457b5730a4487130f290') vs OctetString(tagSet=TagSet((), Tag(tagClass=128, tagFormat=0, tagId=1)))There was a problem hiding this comment.
That's odd. Of the 11, only 2 fail for me (and why they do is documented). The other 9 succeed. You're running Python2, so it's not that.... Investigating.
There was a problem hiding this comment.
My installed Python programs...
PyNaCl==1.0.1
appdirs==1.4.2
argparse==1.2.1
cffi==1.7.0
cryptography==1.4
enum34==1.1.6
idna==2.4
ipaddress==1.0.18
iso8601==0.1.11
packaging==16.8
pluggy==0.4.0
py==1.4.32
pyasn1==0.2.3
pycparser==2.17
pycrypto==2.6.1
pyparsing==2.2.0
six==1.10.0
tox==2.6.0
-e [email protected]:awwad/tuf.git@8440e822589ff78319574199d99a97018d4c6b81#egg=tuf-origin/pinning_w_ber
virtualenv==15.1.0
wsgiref==0.1.2There was a problem hiding this comment.
I just pulled in your latest commits and am now on commit 892c0bc
I still get the PyAsn1Error exception.
There was a problem hiding this comment.
I've reproduced it in a fresh environment and I'm trying to figure out what the difference is. Thanks for pointing it out.
There was a problem hiding this comment.
pinning_w_ber is based on pinning_w_dirtywrites. Could there be something in the base branch that's changed?
There was a problem hiding this comment.
No, I have one failing and one performing as expected even with both of them referencing the exact same tuf install.
There was a problem hiding this comment.
/: They were using different pyasn1 versions after all. It turns out that something in pyasn1 0.2.3 (update from a week or two ago) breaks the conversion code we're using. So for now, I'll make sure the installation instructions specify pyasn1==0.2.2, which works.
Some of the inherited code from an import from early uptane/asn1 has been removed. Specifically, tuf/encoding/metadata.py (which had some testing and time handling functions, wrappers, and hacks).
Also cut out some unnecessary imports.
Reverts one of the changes in commit 45b7a59 An extra decode('utf-8') was added to util.load_json_string, breaking behavior in Python3.
Partial
Creating and verifying signatures should now work in Python2 and
Python3, whether over JSON or ASN.1/DER representations.
The ed25519 library was unhappy when receiving a string in Python3,
so encode('utf-8') had to be run, once, in either format.
Reverted piece is keys.py, ll753-757,893-897, from commit eed6d48 These commits from the sprint broke demo execution, but testing didn't catch it. I'll review another way to try to get the unicode/str/bytes encoding right for Python3 ASN.1/DER and provide that later.
force_treat_as_pydict for create and verify sig in keys.py
Add from __future__ imports for print_function and unicode_literals.
as a precaution. Doesn't correct existing issue, but best to be consistent about this to avoid future headaches.
The motivation for this format and encoding is to deliver data to a low-resource ECU (Electrical Control Unit, a general term for most of the electronic components in an automobile). Sam Lauzon at UMTRI has produced a client that is partially compliant with the Partial Verification model of Uptane's update validation, but expects this class of metadata.
Using this code, the Uptane demo and reference implementation now operate using ASN.1/DER (ref1, ref2) or Python dictionary/JSON metadata, whichever is preferred. The default is now DER.
Comments on development are largely here in the Uptane PR.
This pull request is not expected to be merged (as there is no need to merge into pinning_w_dirtywrites); it is specifically here for review of changes made in these commits.
Vlad, don't panic at the size.... Try to just focus on the core TUF changes and let me know how you feel about them? Please do read the linked thread, though, for the limitations (delegations, RSA keys, etc.)