Skip to content
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

Initial PR for BER encoding for Director targets metadata #29

Merged
merged 17 commits into from
Feb 27, 2017

Conversation

awwad
Copy link
Owner

@awwad awwad commented Jan 27, 2017

The first of the three commits here so far is a few files directly from @trishankkarthik's uptane/asn1 repo. (Thanks, Trishank.) It includes asn.1 definitions for some of the metadata (There's more in that repo.). Unfortunately, there have been some bugs and I'm still sorting through them.

The second commit adds some functionality to pull it in, such that ber.encode_signed_json_metadata_as_ber(<filename of targets.json>) will correctly encode and re-sign targets role metadata... as long as there aren't any images listed. If there are image files listed, the encoder currently borks. I'll work on this ASAP.

There's a lot to still do here with targets.json, including:

  • debugging the above issue and any that follow (correcting the asn.1 formats)
  • plugging in signing key parameters so it's not hard-coded
  • considering security concerns with the new dependency (which may be used in the reference implementation...)

The third commit also pulls in timestamp.json BER encoding and signing, mostly for testing purposes. (It works, via the same call from the second commit, ber.encode_signed_json_metadata_as_ber(<filename of timestamp.json>), but I won't need it for the cross-CAN partial verification Secondary workflow. (PV Secondaries don't take in timestamp metadata - just timeserver attestations and Director targets metadata.) Most of the configuration in this third commit is from @trishankkarthik's uptane/asn1 repo again. (More will probably get pulled in in the coming days, too.)

After that, I'll move on to the timeserver attestations and then finish plugging this code into the live demo server code.

@awwad awwad self-assigned this Jan 27, 2017
@trishankkarthik
Copy link
Contributor

Let me know how I can help!

@awwad
Copy link
Owner Author

awwad commented Jan 27, 2017

It would be helpful to figure out why a targets.json metadata file with targets fails to be processed. I think it's just a bug in the format specification, but you're more familiar with it.

It's probably easiest to just run import uptane.ber_encoder.encode_signed_json_metadata_as_ber() and give it the filename of a targets.json file I've just emailed you as a sample to replicate the issue above.

If you would like, you can try running your asn1/README.py function and approaching things that way instead. You'd just need to fix your json.load calls for the json metadata to use 'r' instead of 'rb' reads. I don't remember if there were other fixes necessary.

@awwad
Copy link
Owner Author

awwad commented Jan 27, 2017

The demo Director now additionally generates targets.ber on top of targets.json, and hosts it in the same way.

The Primary does not yet retrieve this BER file from the Director, as there are questions about how to do this cleanly and securely. It could be retrieved as a target file, inhering the full TUF protections, but this would require that the map file (pinning.json) add a delegation to the Director alone for targets.ber, which would be an unfortunate addition of complexity.... We could otherwise download the file using some additional, TUF-independent logic (and then, ultimately, validate it on the Primary in a similar manner to a target file).

Hm. This bears some thought. Ideally, TUF would support the additional encoding so that it could be treated identically to JSON metadata, but that is not currently practical.

Right now, I'm leaning toward targets.ber being a target file, which will require at least two special exceptions in the logic for the director targets.json file:

  • the BER will have to be generated before the JSON since the JSON will have to include the BER and the BER can't really be generated in such a manner as to include a signature over itself
  • the BER target will not have an ECU assigned, which is currently expected for all Director target files

@trishankkarthik
Copy link
Contributor

Fixed the bug you sent in uptane/asn1@07a44b9

Just send me the JSON files that fail to convert to BER anymore, and I'll fix the bugs

@awwad
Copy link
Owner Author

awwad commented Jan 27, 2017

This is the error I get, which remains with those edits.

>>> ber.encode_signed_json_metadata_as_ber('targets.json')
Traceback (most recent call last):
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/univ.py", line 92, in prettyIn
    return int(value)
ValueError: invalid literal for int() with base 10: b'sha512'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/s/w/uptane/uptane/ber_encoder.py", line 71, in encode_signed_json_metadata_as_ber
    asn_signed = relevant_asn_module.get_asn_signed(json_signed)
  File "/Users/s/w/uptane/uptane/encoding/targetsmetadata.py", line 20, in get_asn_signed
    set_asn_targets(json_signed, targetsMetadata)
  File "/Users/s/w/uptane/uptane/encoding/targetsmetadata.py", line 172, in set_asn_targets
    hash['function'] = int(HashFunction(hash_function.encode('ascii')))
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/univ.py", line 22, in __init__
    self, value, tagSet, subtypeSpec
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/base.py", line 74, in __init__
    value = self.prettyIn(value)
  File "/Users/s/w/uptane/v3/lib/python3.5/site-packages/pyasn1/type/univ.py", line 95, in prettyIn
    'Can\'t coerce %r into integer: %s' % (value, sys.exc_info()[1])
pyasn1.error.PyAsn1Error: Can't coerce b'sha512' into integer: invalid literal for int() with base 10: b'sha512'
>>>

@awwad
Copy link
Owner Author

awwad commented Jan 27, 2017

Bah, I think that last one may have been a python3 incompatibility thing. It's odd: the timestamp metadata and no-image targets metadata validated in python3, but when you add target info, that's when it fails in python3? The encoding is completing without error in python2. I'll make sure it decodes correctly.

@trishankkarthik
Copy link
Contributor

Yeah, let's test on Python 2 first, so that we are not tripped by pyasn1's incompatibility with Python 3. Once we fix our bugs in Python 2, then we can figure out how to make pyasn1 work with Python 3...

@awwad
Copy link
Owner Author

awwad commented Jan 27, 2017

Yes, I'm working in Python2 now. I was lulled into a false sense of security by some seemingly-successful encodings in Python3 previously.

@trishankkarthik
Copy link
Contributor

Actually, it looks like pyasn1 is Python3-compatible, but my code isn't. In any case, we'll fix it later!

@awwad
Copy link
Owner Author

awwad commented Jan 30, 2017 via email

@awwad
Copy link
Owner Author

awwad commented Feb 1, 2017

I have a sizable bug in this. The signature isn't included in what I have the Director hosting. Working on it now.

@trishankkarthik
Copy link
Contributor

What's missing?

…osts correct BER encoding, signed by correct key
@awwad
Copy link
Owner Author

awwad commented Feb 2, 2017

Bugs I've found fixed.

  • The demo Director now correctly includes signatures in the targets.ber file that it generates and hosts, using the right key (whatever key the demo Director is using as its targets key). This BER file is generated every time a Director repository write occurs (i.e. when someone assigns a bundle to a vehicle), after the targets.json and other metadata files in JSON are written.
  • The type of metadata file for Director targets metadata is no longer incorrectly marked as "snapshot" in the ASN.1 conversion.

Additionally, code exists to convert back to a simple dict (compatible with the format we're using for JSON encoding), and I've tried it out with some sample Director files and confirmed that they're equal. (uptane.ber_encoder.convert_signed_ber_to_unsigned_json())

Remaining tasks:

  1. Test the signatures to make sure they're as expected.
  2. Plug in timeserver attestations next (should be much quicker, assuming the ASN.1 definition for them is correct - still quicker, either way, though).
  3. Primary must retrieve the BER targets file from the Director, decode it, validate its signature, and compare the data to the JSON data it retrieves to ensure that they are equal (otherwise additional attacks are possible).
  4. Fun Times With CAN Hardware
  5. Distributing the BER file through Sam's library (which is imported as a C lib in the Python).
  6. Rewrite of targetsmetadata.py and timeservermodule.py (for clarity, security, and python3 compatibility)

I'm already not using metadata.py and README.py from the uptane/asn1 repo (replaced by ber_encoder.py). There are a few lines in ber_encoder that are still just kinda magic (which isn't acceptable). Security review follows....

Much less urgent, and general (captured by other issues):
7. Add support for hardware identifiers (general - not specific to encoding; easy, but touches a number of places).
8. Add support for release counters (general - not specific to encoding)
9. Good unit testing.

Once I have some ECU Manifests from Sam's PV Secondary, I can also ensure that the Primary handles them correctly and that the Director knows how to verify their signatures and convert their contents.

@awwad
Copy link
Owner Author

awwad commented Feb 3, 2017

Regarding Item 3:
Shifting tacks slightly: have decided that the Primary will retrieve BER data via TUF, treating the BER file like any other metadata file (rather than attempting to duplicate protections through a separate mechanism). Consequently, I'm moving ASN.1/BER support into TUF. The first commit for that is here.

@awwad
Copy link
Owner Author

awwad commented Feb 7, 2017

I've added all the code to TUF that should be required to make it fully ASN.1/BER-compliant (all metadata types). I'm still debugging some stuff. The commits now pushed here cover most of it, though that code is still using some external stuff modified from uptane/asn1 for the metadata definitions in ASN.1. I'm pulling in the necessary code next.

@awwad
Copy link
Owner Author

awwad commented Feb 8, 2017

Good news:
I've worked through the myriad bugs and gotten TUF working with ASN.1/BER as best I can tell! (It's been somewhat ugly, but when there's time, I think I have an image of how this should all look when it's refactored.)

These changes to TUF are visible here in awwad/tuf@pinning_w_ber. (I've tried to start splitting my commits into one for each individual issue, so hopefully that helps.)

As for hooking back up to Uptane, the quick test I ran with the demo Image Repository seemed to work, but more testing is merited tomorrow (Thursday), along with reconciling Uptane with it (which should be quick). I'll hold onto the Uptane-side commits until more testing.

(This also includes the switch to octal from hexstrings, which hopefully saves some space.)

…s) to allow signing and verifying of signatures over Python dictionaries even if the TUF metadata format is ber.
…f TUF for BER encoding; many of these files will likely be made obsolete shortly.
…ne. This will likely be removed or migrated into TUF's ASN.1 format testing.
…accomodate limitations of current ASN1 encoding
@awwad
Copy link
Owner Author

awwad commented Feb 27, 2017

I'm using DER instead of BER now and that seems to have had no impact. TUF itself supports DER now. I've kept the newest stuff to awwad/uptane instead of uptane/uptane because it doesn't connect all the way through the Secondaries yet, and I don't want to break people's stuff if they're experimenting with it.

The Director and Image Repo generate metadata live and sign and host it as DER. The Primary downloads it and fully validates the DER metadata. Distribution to Secondaries (FV and C PV) comes next. The FV Secondary validation should be trivial as long as there are no more weird pyasn1 conversion bugs. Roping in the C & CAN code will be a bit trickier, but that's the next focus.

There were still more bugs in the ASN1 conversion, needless to say. I've worked through every bug I've yet found (commits here - pushed those from before and after I was sick) with the exception of three things that aren't of urgent import (though I'll certainly get to them after we have a working integrated demo that uses Sam's PV Secondary):

  • delegations (in the Image Repository) still break things. For now, I've excluded that.
  • RSA keys aren't supported yet. ed25519 keys are what we're using for now. (LMK if that's an issue, Sam. The format of keyid values is slightly different and the conversion code needs to be updated to support that.)
  • custom fileinfo that is not specifically used by Uptane is not supported. (TUF allows arbitrary custom fileinfo; ASN1 conversion requires stricter typing and so I have to add some code to support this optional feature. Uptane uses ecu_serial currently in custom fileinfo, and that is supported now.)

…ification of switch to ASN.1/DER from Python dictionaries/JSON (and how to switch back if desired); how to perform a normal update in a console demonstration
@awwad awwad merged commit 63fb045 into develop Feb 27, 2017
@awwad
Copy link
Owner Author

awwad commented Feb 27, 2017

The Full Verification Secondary now handles DER metadata properly, meaning that the Uptane reference implementation and demo now support DER in all components. The console demo including one attack (whose instructions are now in the main README.md) now functions by default in DER (and is still working).

This has been merged into the develop branch on awwad/uptane and has now been pushed to uptane/uptane.

The caveats above about delegations, arbitrary custom info, and RSA keys still hold.

This PR is resolved and I'll open a new one for those three issues when they're ready. The next order of business, though, is working on integrating Sam's PV Secondary demo again!

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.

2 participants