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

Add GPGSigner implementation #341

Merged
merged 3 commits into from
Jul 8, 2022
Merged

Conversation

MVrachev
Copy link
Collaborator

Related to: theupdateframework/python-tuf#1263

Description of the changes being introduced by the pull request:

The GPGSigner is an implementation of the new "Signer" interface
that was merged in #319
and could be found in securesystemslib.signer.py

This is the next logical step given that securesystemslib supports GPG
and we want this interface to have implementations for all signature
types which are already supported by securesystemslib.

While implementing the GPGSigner, I wanted to make sure that one can
easily sign a portion of data, receive a Signature object and use the
information stored in that object to verify the signature.

To verifty a GPG signature, one have to use
securesystemslib/gpg/functions.verifty_signature():

def verify_signature(signature_object, pubkey_info, content):

There, the signature_object argument should be in the
securesystemslib.formats.GPG_SIGNATURE_SCHEMA format:
GPG_SIGNATURE_SCHEMA = SCHEMA.Object(

I searched for a way to easily retrieve the additional fields in the
GPG_SIGNATURE_SCHEMA - other_headers and info from the keyid stored
in the "Signature" object returned by the "sign" operation.
Unfortunately, right now there is no function that I can use for that
purpose.
The only option I was left with, was to create a new class:
GPGSignature where we can store those additional fields returned
from securesystemslib.gpg.functions.create_signature() which we call
during the "sign" process.

Please verify and check that the pull request fulfils the following requirements:

  • 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

@MVrachev MVrachev changed the title Add GPGSigner implementation. Add GPGSigner implementation Feb 25, 2021
Copy link
Member

@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.

Cool stuff, @MVrachev! Thanks for adding this wrapper. Except for some minor documentation-related nits, the implementation looks good to me.

Regarding testing, I don't think it is necessary to copy/paste so much of test_gpg. In terms of tested functionality (lines of code) it seems redundant, while adding extra review/maintenance effort.

IMO it should be enough to add one exemplary test case for one key type only, doing

  • signature creation
  • maybe some vetting of the signature object ... were the fields assigned as expected? etc
  • signature conversion (to dict)
  • successful and unsuccessful signature verification

I suppose you could do this twice, one time with the default key and one time with a passed keyid.

securesystemslib/signer.py Outdated Show resolved Hide resolved
securesystemslib/signer.py Outdated Show resolved Hide resolved
securesystemslib/signer.py Outdated Show resolved Hide resolved
securesystemslib/signer.py Outdated Show resolved Hide resolved
securesystemslib/signer.py Outdated Show resolved Hide resolved
securesystemslib/signer.py Outdated Show resolved Hide resolved
securesystemslib/signer.py Outdated Show resolved Hide resolved
@MVrachev
Copy link
Collaborator Author

MVrachev commented Apr 16, 2021

I have addressed all comments made by @lukpueh and fixed all documentation issues he pointed out.

Also, I removed a big chunk of the test files as they were replicating the same tests in test_gpg.py.
I left only GPGRSA tests testing that signing and verification can be done with default and custom keys.

@MVrachev
Copy link
Collaborator Author

MVrachev commented Feb 8, 2022

@lukpueh do we want to move this forward or it's not as much as important as originally discussed in theupdateframework/python-tuf#1263

@lukpueh
Copy link
Member

lukpueh commented Feb 8, 2022

It's not a priority. But let's keep the PR around.

Copy link
Member

@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.

@MVrachev, would you mind resolving the conflict here so that we can merge this PR?

@PradyumnaKrishna, pinged me about it, as he might find this useful when implementing DSSE (#370).

securesystemslib/signer.py Outdated Show resolved Hide resolved
@MVrachev
Copy link
Collaborator Author

@lukpueh and @PradyumnaKrishna I am sorry I responded so late...
I rebased on top of master and fixed the docstring.

PradyumnaKrishna added a commit to PradyumnaKrishna/securesystemslib that referenced this pull request Jun 29, 2022
Signed-off-by: Pradyumna Krishna <git@onpy.in>
Copy link
Member

@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 reviving the PR, @MVrachev. I have one minor change request. Otherwise this looks great.

securesystemslib/signer.py Outdated Show resolved Hide resolved
@MVrachev
Copy link
Collaborator Author

MVrachev commented Jul 8, 2022

@lukpueh removed the info field as you have suggested.

The GPGSigner is an implementation of the new "Signer" interface
that was merged in secure-systems-lab#319
and could be found in securesystemslib.signer.py

This is the next logical step given that securesystemslib supports GPG
and we want this interface to have implementations for all signature
types which are already supported by securesystemslib.

While implementing the GPGSigner, I wanted to make sure that one can
easily sign a portion of data, receive a Signature object and use the
information stored in that object to verify the signature.

To verifty a GPG signature, one have to use
securesystemslib/gpg/functions.verifty_signature().
There, the signature_object argument should be in the
securesystemslib.formats.GPG_SIGNATURE_SCHEMA format.
I searched for a way to easily retrieve the additional fields in the
GPG_SIGNATURE_SCHEMA -"other_headers" and "info" from the keyid stored
in the "Signature" object returned by the "sign" operation.
Unfortunately, right now there is no function that I can use for that
purpose.
The only option I was left with, was to create a new class:
"GPGSignature" where we can store those additional fields returned
from securesystemslib.gpg.functions.create_signature() which we call
during the "sign" process.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Test all variations of the GPG schema securesystemslib curretnly
supports. Make sure we can easly sign, receive an object from the sign
operation and use the information stored from that object to verify
the signature.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Copy link
Member

@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!

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