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

Support gpg signatures (copied from in-toto project) #163

Conversation

danixeee
Copy link
Contributor

@danixeee danixeee commented Mar 5, 2019

This PR relates to #55. I have mostly just copied and modified (where needed) gpg signing implementation from in-toto project and kept it in separate module. Relevant tests are also added with some new ones to increase test coverage to 100%.

I have slightly changed SIGNATURE_SCHEMA in order to support adding gpg signatures with append_signature function to TUF metadata.

I need some directions, do we want gpg as a separate module, or gpg/formats.py should be added to existing formats.py, etc. ?

  • 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

@lukpueh
Copy link
Member

lukpueh commented Mar 7, 2019

@danixeee, thanks for your PR! It would be good to know the in-toto commit id at which you copied the gpg subpackage. It seems like it was pre-in-toto/in-toto@4830a3a, which fixed minor packet parsing bugs and added support for new format lengths (see in-toto/in-toto#255)

Please also take notice of two further gpg related PRs that are still under active development in-toto/in-toto#245 and in-toto/in-toto#257.

@danixeee
Copy link
Contributor Author

danixeee commented Mar 8, 2019

@lukpueh Thank you for a quick response. Last commit that I pulled is in-toto/in-toto@32ec88b, so, yes, it was before those fixes.

Do you recommend waiting for those PRs to get merged and then to update the all changes on securesystemslib's side? Also, do you have an estimated timeline for those PRs?

NOTE: I needed to change gpg.process.run method, because tests are failing for python 3.4 ( subprocess.run is introduced in python version 3.5).

@lukpueh
Copy link
Member

lukpueh commented Mar 11, 2019

I'd at least pull in the bug fixes from in-toto/in-toto#255.

As for the other PRs, I'd pause here until they are merged, but they still need some work, so it depends on how urgently we want gpg in securesystemslib.

Note that especially in-toto/in-toto#257 brings up a couple of further issues (see Caveats/TODOs in the PR description, and one of @aaaaalbert's review comments). I would like to at least tackle the "proper testing" todo item over in the in-toto repo. But I'm open for ticketizing and working off the other issues here.

Regarding subprocess.run, on the in-toto project we just dropped Python 3.4 support for that reason. I don't know if we want to do the same thing here (@awwad, @SantiagoTorres, @trishankatdatadog?). Btw. I made a note about that issue in the corresponding PR (see in-toto/in-toto#231).

@trishankatdatadog
Copy link
Contributor

@lukpueh That's fine, let's drop 3.4

@danixeee danixeee force-pushed the danixeee/integrate-gpg-from-in-toto branch 2 times, most recently from 842e729 to 6a6aa9e Compare March 13, 2019 11:07
@danixeee
Copy link
Contributor Author

I have added the latest changes from in-toto which are related to GPG code (32ec88b...ab1e904) and removed python 3.4 from tox.ini and .travis.yml.

@lukpueh CI failed because test coverage dropped to 99% (on my local PC is 100%, not sure what is the difference), so, do we need more tests or we should skip coverage for that part?

@lukpueh
Copy link
Member

lukpueh commented Mar 13, 2019

Oh yes, that's due to a pragma: no cover I removed. Coverage should actually drop on your local PC too.

I meant to add a test in in-toto/in-toto#257 or a follow-up PR. But I'd of course appreciate help. :)

Note that it might be a bit tricky to test, because those lines are only touched if a signature carries subpacket 33 (Issuer Fingerprint), which is not part of RFC4880 (but is mentioned in its update draft since RFC488-bis-01). I haven't checked yet which implementations have adopted it. I have also considered to manually craft test signature data for parse_signature_packet to also test the other conditions that currently exempt from coverage.

@danixeee danixeee force-pushed the danixeee/integrate-gpg-from-in-toto branch from 184d4e7 to 56bbc48 Compare March 20, 2019 14:01
@danixeee
Copy link
Contributor Author

@lukpueh I would love to help, but I am not an expert on this subject. :)

@lukpueh
Copy link
Member

lukpueh commented Mar 28, 2019

@danixeee, sorry for my late reply! GPG self-signature verification has been reviewed and merged with in-toto/in-toto#257. That PR also removes a bunch of pragma: no covers and instead adds tests. :)

I also just wrapped up key expiration parsing (from verified self-signatures) and verification. The PR can be found at in-toto/in-toto#266. Once that's reviewed (feel free to leave comments) and merged over there, I'd say we move everything over here, including related issues/feature requests such as in-toto/in-toto#263 and in-toto/in-toto#126.

@lukpueh
Copy link
Member

lukpueh commented Aug 13, 2019

Closing here in favor of #174. Thanks for leading the initiative, @danixeee! :)

Note that I didn't adapt SIGNATURE_SCHEMA in #174 as you did here, because it felt like a bigger change. I'd rather do this in a separate PR, maybe as part of a broader formats/schema revision. Do you think you can work around this by adding something like below to your taf.formats and patching append_signature accordingly?

ANY_SIGNATURE_SCHEMA = securesystemslib.schema.OneOf([
  securesysemslib.formats.SIGNATURE_SCHEMA,
  securesysemslib.formats.GPG_SIGNATURE_SCHEMA,
])

Also, I suspect you will have to do something similar for securesystemslib.formats.ANYKEY_SCHEMA.

@lukpueh lukpueh closed this Aug 13, 2019
@danixeee danixeee deleted the danixeee/integrate-gpg-from-in-toto branch August 21, 2019 12:07
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.

3 participants