Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Add client as parent class for primary and secondary #192

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from

Conversation

tanishqjasoria
Copy link

Purpose of the PR:

Refactor primary.py and secondary.py to pull out common elements
Address Issue #14

Summary of Changes:

  1. Add a client class implementing the common methods used by both
    Primary and Secondary
  2. Primary and Secondary inherits from the client class.

Client class aim to implement the common methods used by
by both the Primary and Secondary. They would inherit the
Client class and would implement additional methods as per
their requirements.
Add two duplicate functions
 - refresh_toplevel_metadata
 - get_validated_target_info
Method get_target_list_from_director gets the list of latest
updates to be install in the vehicle
This method would be used to verify timeserver signature on the
timeserver_attestation before we proced to update the time
of the client.
This method would be used to update the time of the client
after the timeserver signature is verified and the list
of nounce is verified by the Primary or Secondary.
Two important changes were introduced
1. Primary inherited from the Client class and upate the
   constructor of the primary class
2. Earlier the key of Primary was refered to as primary_key, to improve
   consistency in Client, Primary and Secondary, change it to ecu_key
Remove
1. refresh_toplevel_metadata
2. get_validated_target_info
Modify update_time to use method verify_timeserver_signature
and update_verified_time from Client to verify signature of
the timeserver and to update the time after verification process
respectively.
Done to accomodate for changes made in Primary.
In Primary, we now refere private key of Primary
as ecu_key insted of primary_key
There was a copy of previous implemented version of this
method in Primary, which needed to be removed so that
Primary can use Client class implemtation of the method,
by default.
Now metadata distributed to the partial verification secondaries
would be in the form of a archive with the structure almost
similar to that of the distributed archive for full verification.
Here only director repo would be present and it would only have
the targets metadata.

This is done so that the secondary could obtain, save and expand
the metadata recieved from primary in a similar fashion as that
of full verification. It would further help in simplifying the
partial verification process
Now primary is sending archive for partial verification
metadata as well.
Used various tuf low level functions to verify the targets metadata
supplied by the director repository. It complies with the latest
Uptane standard for design.
Earlier even if no target_info was provided the
validated_targets_for_this_ecu would be emptied.

Now if new target info is present, then the list
validated_targets_for_this_ecu would be updated
Earlier the test data was stores in seperate lists like
TEMP_CLIENT_DIRS, vins, ecu_serials and secondary_instances

Now all the data corresponding to a particular secondary is
stored in a dictionary and all the dictionaries are further
stored in a list TEST_INSTANCES
Update the function get_partial_metadata_fname to
get_partial_metadata_archive_fname
Earlier the partial metadata supplied by the primary was
just director_targets file but now the primary provides a
archive with the structure similar to that of full metadata
Modified metadata files to include a possible update
for the newly included partial verifying secondary ECU.
…t the update

This included fileinfo corresponds to the info of the
update that the partial verifying ECU would donwnload
as per the new metadata.
The tests for partial verification ECU are carried out in the
similar fashion as that of the full verification ECU.
Due to some discrepency in json and der metadata
the tests failed while working with der formats
1. Removed methods which are implemented in theclient class
2. Modify update_time and fully_validate_metadata methods to
   use function from the client class
Earlier the private key of the primary ECU was used by the
name 'primary_key'. Now it has been changed to 'ecu_key'
@tanishqjasoria tanishqjasoria changed the title Add client as parent class for primary and secondar Add client as parent class for primary and secondary May 30, 2019
Syntax for initialization of parent from child class is different
in python2.
Copy link
Collaborator

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Thanks for the clean PR, @tanishqjasoria! Please address my comments below and this should be good to merge.

IMO you can further remove redundancy in the Primary/Secondary class docstrings. Instead of duplicating docstrings, point the reader to the docstrings of the common Client parent class.

And you can also remove check_match calls in Primary/Secondary constructors for arguments that are checked in the parent class as well.

uptane/clients/client.py Show resolved Hide resolved
uptane/clients/client.py Outdated Show resolved Hide resolved
uptane/clients/client.py Show resolved Hide resolved
uptane/clients/client.py Outdated Show resolved Hide resolved
uptane/clients/client.py Outdated Show resolved Hide resolved
uptane/clients/client.py Outdated Show resolved Hide resolved
uptane/clients/client.py Outdated Show resolved Hide resolved
uptane/clients/primary.py Outdated Show resolved Hide resolved
uptane/clients/client.py Outdated Show resolved Hide resolved
uptane/clients/secondary.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, @tanishqjasoria!

Please address whitespace comment below and update class docstrings in Primary and Secondary to no longer list methods and fields that were moved to Client.

raise uptane.Error('Unexpected behavior: did not receive target info from'
' Director repository (' + repr(self.director_repo_name) + ') for '
'a target (' + repr(target_filepath) + '). Is pinned.json configured '
'to allow some targets to validate without Director approval, or is'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a space after is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On a side-note, it's a good idea to have the separating space between strings over multiple lines either all in the end or in the beginning (I am slightly in favor of the former, as you did in all but one lines here).

@lukpueh
Copy link
Collaborator

lukpueh commented Jul 10, 2019

Btw. don't worry about the failing Travis build due to missing Python 3.4. I'll fix this in https://github.com/lukpueh/uptane/tree/fix-ci-cd

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants