Added checks for gpg key expiration in signature verification

[michizhou]

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes issue #236:

Description of the changes being introduced by the pull request:
Implemented new checks to verify key expiration and included it as part of the signature verification process. Added warnings to be issued at time of signing and whenever public keys are extracted. Parsed expiration times at signature creation and included it into format of signature data structure.

Noteworthy:
I used a scheme that involves going through all valid self-signatures of a public key's user ID packets with set key expiration times and taking the key expiration time of the most recent signature to help assign expiration dates to public keys, prioritizing signatures with expiration times that had a Primary User ID flag set. To refine my use cases, I conducted an investigation to determine whether User ID packets having its key expiration time and Primary User ID flag set in different self-signatures was a common occurrence by analyzing public keys and signatures in real world keychains.

I made these preliminary findings regarding relative frequencies of key expiration dates and Primary User ID flags in self-signatures for public keys. Out of 50k signatures and ~1100 User ID packets:

686 self-signatures had a Primary User ID flag set
437 self-signatures had a Primary User ID flag set but no key expiration time (most of these were associated with keys that did not have any set expiration time)
1795 self-signatures had a set key expiration time (249 of which also have a Primary User ID flag set)
11 User ID packets had key expiration time and Primary User ID flag set in different self-signatures

Please verify and check that the pull request fulfills 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

[lukpueh]

Thanks for the PR, @michizhou! As you can see, both AppVeyor and Travis CI builds fail. Please fix all issues. The build logs should give you some ideas, where to look.

[michizhou]

@lukpueh I am not too sure how to fix these issues. They all stem from errors with importing some modules, yet IIRC, the import statements affected are the same as before (i.e. never modified). So I am not sure why they are giving off errors now. I will investigate and try my best to find where the problem is. If I can't find them I'll let you know.

[lukpueh]

@michizhou, please read the linter error carefully. The import errors are caused by earlier syntax errors.

[lukpueh]

@michizhou, please take a look at the discussion about rebasing after having submitted a PR secure-systems-lab/lab-guidelines#14 (comment). I did tell you to make use of rebase to get rid of merge commits in your branches and to keep your history clean. However, that doesn't mean that you should stop adding new commits.

I see that you've been force-pushing the same commit again and again, probably to troubleshoot/fix the build errors? But it's totally okay to create separate commits for your troubleshooting changes. That way I (and other developers) can better track your progress and jump in to help.

It might also be a good idea to test locally before pushing to GitHub. See #216 (comment) for some instructions.

[michizhou]

@lukpueh Most of the updates I force-pushed for my commits have been corrections for small, trivial mistakes in formatting and variable assignments/declarations with no substantial changes in code content or structure. As of now most of the build errors seem related to the signature and key formats in format.py (specifically, the creation_date field in the key format and the key_expire_time field in the signature format). I am honestly stuck and don't know how to fix these errors. I really need you to take a look and let me know how to fix them, since I cannot do it on my own. I also don't have local testing enabled on my device, and won't be able to install it until next week with Santiago's help.

[lukpueh]

Sure, no problem. I see a lot of ValueError: invalid literal for int() with base 10: '''' in your travis log. That means you're trying to convert an empty string into an integer, i.e. there is no expiration date, but you are trying to verify it.
You probably should only assign the expiration date if it is there (and only check it if it was assigned).
You can make fields in formats.py optional.

I also just realized that it is indeed better to add the key expiration date to the public key dict (as I had suggested originally) and not to the signature, because the signature object itself is not signed, whereas a public key, at least when embedded in an in-toto layout, is.
Parsing the expiration date at the time of key export means that you would have to parse the self-signature that is part of an exported public key.

As for signature expiration dates, it wouldn't really make sense to add it to the key, and it's unsafe to add it to the signature, so I think we should just ignore it. TBH I don't know if anyone even uses these.

[michizhou]

@lukpueh With regards to signature expiration dates, I am actually only adding the key expiration times to the key creation dates to get key expiration dates, without making use of signature expiration times beyond assignment. In any case, I will remove the signature expiration time field since we won't use it.

With regards to adding the key expiration date to the public key dict, although I agree and understand your approach, I am afraid it is impossible with the Version 4 packets, since key expiration times (i.e. time in seconds for which key is valid after its creation date) are only found in the signature packets. Because of this, at parsing of the public key packet for construction of the public key dict, we cannot derive the key expiration date for addition to the public key dict. The only way to derive key expiration date would be to add the key expiration time to the signature dict and then add it (if it exists and is not 0) to the creation date of the public key dict in the gpg_verify_signature method of functions.py.

[michizhou]

@lukpueh So the errors related to the key expiration checks in signature verification have disappeared, but there seems to be a lot of FormatError: Object did not match a recognized alternative. in my travis log now. I am assuming that they have stem from the fact that I changed the format of the public key and/or signature dict data structures, though I am not sure how to fix these errors. I would really appreciate it if you could offer constructive advice on what I should do to resolve them. Thanks!

[lukpueh]

Parsing the expiration date at the time of key export means that you would have to parse the self-signature that is part of an exported public key.

Take a look at any public key in your gpg keychain (or the in-toto test gpg keychain), I bet that they all have a self-signature attached, because GnuPG automatically creates a self-signature when you create a key. You can use the following command, to list all public keys and their attached signatures in a keychain. Change </path/to> to the path to your local in-toto repository).

gpg --list-sigs --homedir </path/to>/in-toto/tests/gpg_keyrings/rsa

You can also export a single public key, as we do in our in_toto.gpg module, and look at all the packets that are included. You will see that the key and their subkeys have a self-signature attached.

gpg --homedir </path/to>/in-toto/tests/gpg_keyrings/rsa --export 8465A1E2E0FB2B40ADB2478E18FB3F537E0C8A17  | gpg --list-packets

[lukpueh]

Re FormatError: Object did not match a recognized alternative:
Please take a look at the traceback information in the build log, it should give you enough information to trace back the objects, on which format validation failed. Compare those objects with the formats you have defined in formats.py and see why they don't match.

Please also address the comments I left above.

[michizhou]

@lukpueh Are you saying that to derive the key expiration date at the time of key export I would have to use the key expiration time from the exported key's self-signature rather than the signature that is generated at line 98 in the gpg_sign_object method of functions.py? How would I access the key's self-signature? Also are the creation dates in UNIX/POSIX format or ISO8601 format? Let me know.

[lukpueh]

Are you saying that to derive the key expiration date at the time of key export I would have to use the key expiration time from the exported key's self-signature rather than the signature that is generated at line 98 in the gpg_sign_object method of functions.py?

Yes!

How would I access the key's self-signature?

Well, that's kind of the exercise here. :) Take a look at the functions in gpg.common. You will have to modify the parse_pubkey_* functions to not only return the key and subkey payloads but also their attached self-signatures. Once you have found the right byte range(s) in the passed stream (which comes from gpg --export) you should be able to pass it to parse_signature_packet to extract the
embedded key's expiration time, as you are doing now.

Also are the creation dates in UNIX/POSIX format or ISO8601 format?

What do you think? :)

[michizhou]

@lukpueh Just to let you know, I am still in the process of working through the build errors, so I am not quite done with all my changes as of yet. From what I can tell so far, it seems that my changing of the format for the key creation date field of the public key data structure didn't mitigate the FormatErrors, while my attempt at accessing the public key self-signature was unsuccessful too. I am working on an alternative approach to getting the self-signature while I continue troubleshooting the FormatErrors.

[coveralls]

Coverage remained the same at 100.0% when pulling fcc7650 on michizhou:check-key-expiration into 32ec88b on in-toto:develop.

[michizhou]

@lukpueh First of all, I want to deeply apologize for burdening you with my subpar commits from the past week. I have managed to resolve almost all build error issues by refactoring significant parts of the code, so when you have the time I would appreciate it if you could review my changes.

As a heads up, in a future commit I am planning to include a new data structure designed to store all parsed public keys and incorporating it into the signature packet parsing process to properly match up self-signatures with their respective public keys in order to allow for dynamic updates of key expiration dates and times from self-signatures that could then be passed onto their public keys.

[lukpueh]

Hey @michizhou, no need to apologize. You're getting there! ;) I saw that your travis builds still fail. This is due to a new version of pylint that flags unnecessary pass statements. I fixed it in #246. It is already merged, so please rebase on the tip of in-toto/in-toto@develop.

[lukpueh]

Thanks for the changes, @michizhou. This looks a lot better. Please address my comments and add tests.

[michizhou]

@lukpueh I have resolved most of the comments in your review with my latest changes to the code and also added a new test suite in the gpg test script as per your suggestion. I have also addressed some of your comments with my own thoughts, which I would like you to consider. If you have any additional concerns about the code in its current state, please let me know in your next review. Thanks!

[michizhou]

@lukpueh I have pushed a new commit that includes the new data structure used for storing all parsed public keys and incorporates it into the signature packet parsing process to properly match up self-signatures with their respective public keys as I mentioned in #245 (comment). Along with my previous commit (a44ce75), I would greatly appreciate it if you could review and share your thoughts on all of my changes and whether further improvements need to be made. Thanks!

[lukpueh]

@michizhou, tests are still failing.

[michizhou]

@lukpueh I am fully aware that the tests are failing at the moment. In adding new gpg tests in the test script two weeks ago @SantiagoTorres mentioned I could make use of mock.patch to streamline the test script code. However, my changes using mock.patch brought about the current build errors.

I let him know about the errors, and as of now I am still in correspondence with him on troubleshooting the build errors, which is still in progress. I expect to resolve the issue within the next few days, after which I intend to update the code so that it won't produce any build errors.

[michizhou]

@lukpueh I managed to resolve the build errors with the mock patch in my tests. When you have the chance, feel free to look over my recent changes and share your thoughts.

[lukpueh]

Awesome! Thanks, @michizhou. Can you please take a look at the coverage report and add tests so that all your code additions are covered.

[michizhou]

@lukpueh Not a problem. I will try to add tests to cover for the code additions not yet accounted for.

[michizhou]

@lukpueh I sincerely and deeply apologize for not getting back to you sooner. I ended up having to not only add more tests to increase coverage but also had to update the keyring I used for my tests with a new one that was compatible with the SHA256 hash algorithm used by the rest of the code. I have also refactored the signature packet processing block to integrate more steps into signature verification.

As for the build errors, I am not quite sure why they are showing up, since when I locally tested all my modified code on my own machine it passed with no issues. It might have to do with the new keyring directory I added to ensure compatibility with the tests that I mentioned above. Please let me know if you think there is a need for additional changes to address other issues I haven't mentioned.

[lukpueh]

@michizhou, the reason why your tests fail on travis and appveyor is that they unfortunately can't handle the keyring format you're using in tests/gpg_keyrings/new_keyr.

You have to generate the test keys on a system with a version of gpg below 2.1 (see 87e8c4b and What’s new in GnuPG 2.1) . Btw. you don't need to create a new keyring, you can also add test keys to one of the existing test keyrings e.g.:

# In the root of the in-toto project on a system with gpg <2.1
gpg --homedir tests/gpg_keyrings/rsa --gen-key

[SantiagoTorres]

The issue is that the certification signatures in the existing keyrings for subkeys use sha1 as the signing algo, which we don't support.

…

On Wed, Jan 30, 2019 at 08:22:20AM -0800, lukpueh wrote: @michizhou, the reason why your tests fail on travis and appveyor is that they unfortunately can't handle the keyring format you're using in `tests/gpg_keyrings/new_keyr`. You have to generate the test keys on a system with a version of gpg below 2.1 (see 87e8c4b and [*What’s new in GnuPG 2.1*](https://www.gnupg.org/faq/whats-new-in-2.1.html)) . Btw. you don't need to create a new keyring, you can also add test keys to one of the existing test keyrings e.g.: ```bash # In the root of the in-toto project on a system with gpg <2.1 gpg --homedir tests/gpg_keyrings/rsa --gen-key ``` -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #245 (comment)

[lukpueh]

The hashing algorithm is only relevant to verify the self-signature, and not to parse its sub-packets (such as the expiration date). I was able to parse the keys' expiration dates from self-signatures with digest algo 2, 8 and 10 respectively (that's SHA1, SHA256 and SHA512), using gpg key rings below and above version 2.1, and I did not have to modify any hashing related code in our gpg module.

However, by not verifying the self-signature, i.e. if the expiration date actually belongs to the corresponding key, an attacker who controls our gpg public keys might forge them. So a la long we should try to support other hashing algorithms and self-signature verification. The former might be as easy as updating the used hashing algorithms in our gpg_verify_signature functions. The latter seems more complicated. At least from a first search through RFC4880, I couldn't figure out how to do that. Additionally, it might be hard to test in our CI/CD systems. Because as far as I can see, gpg does not let you customize self-signature hashing algorithm, but just uses the default of your gpg version.

@michizhou, please ignore self-signature verification for now and just try to parse the expiration date.
To do that, create another key in the old key ring format. You can create a new key ring or add it to the existing keyring(s). Make sure that the attached self-certificates actually have expiration dates, which btw. the keys in your new_keyr key ring don't have. Please also add expiration dates to subkeys and parse them appropriately (see 5.2.1 for the key bundle structure). You will also need to re-enable the supported self signatures types that you have commented out.

And last but not least, please be very careful with pragma: no cover. Although your tests pass locally with 100% coverage they actually never successfully extract an expiration date.

[lukpueh]

Taken over by #266