Add GPG signing subkey support #190

lukpueh · 2018-03-01T22:01:28Z

Description of the changes being introduced by the pull request:

GPG is often used together with subkeys, where a master key is associated with several sub keys with different capabilities (signing, encryption, both). This PR updates in-toto's gpg module to support subkeys and adds subkey trust to in_toto's link and layout signature verification.

The main chunks of this PR are:

Update gpg module to support exporting public key bundles (master key + sub keys) instead of the master public key only. 46d8ad1, 105d625
Update gpg module to better support signing with old versions of GPG that don't add the full keyid to the signature. 28ba993
Update signature verification methods to accept a public key bundle and also iterate over subkeys to find the right verification key for a signature. c63553c
Update link threshold verification to support gpg trust delegations. A good overview over the newly supported scenarios can be found in the related verifylib TestCase.
0a6dcac, 7324c76, 90ed9fa

As always, please consider the commit messages as source for additional info.

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

This commit extends RSA_PUBKEY_SCHEMA and DSA_PUBKEY_SCHEMA with an optional "subkeys" field that contains a dictionary with sub keyids as keys and subkeys in the corresponding schema as values.

GPG returns the entire pubkey bundle (main key plus subkeys) when any main or subkey keyid of that bundle is passed. This commit modifies our custom export pubkey function to also return the entire pubkey bundle, returned by gpg and not only the main pubkey, but only if the passed keyid matches the main key's keyid or any of the subkeys' keyids. Moreover, if a subkey keyid was passed, the user is warned, since s/he might be unintentally authorizing the mainkey or a sibling subkey. The commit includes - moving the parse_pubkeys_from_packets implementation to gpg_export_pubkey. - creating the key dictionary in our custom format in parse_pubkey_packet instead of gpg_export_key (this is consistent with the function parse_signature_packet, which also returns a signature dict in our custom format) - Fixes a unit test, that now fails with the newly added KeyNotFoundError

in_toto.metadata.Metablock's signature verification function used to fail if the passed verification key's keyid did not match any of the signatures contained in the Metablock's signature property. This commit extends the verification to also iterate over optional subkeys contained in the passed verification key. The entire keybundle is then passed on to the gpg verification function, where it extracts the matching key or subkey to verify the signature.

This commit extends verify_link_signature_thresholds' trust to automatically delegate trust to available gpg subkeys. That is, if a link is signed with a subkey, and the corresponding master key is authorized to sign that link, as per the layout, then the subkey is authorized implicitly.

Add rsa signing subkey (C5A0ABE6EC19D0D65F85E2C39BE9DF5131D924E9) to default test gpg rsa key (C5A0ABE6EC19D0D65F85E2C39BE9DF5131D924E9) and adopt unit tests. Gpg automatically uses a signing subkey if available, hence unit tests that used to use the corresponding default key are updated.

Link signing, per-step key authorization and the corresponding layout key store value together with main keys and subkeys allows for 8 combinations of which some are possible to verify and others not. See test case docstring for more info.

Add reference to RFC4880 section 4.2 that describes packet headers to in_toto.gpg.util.parse_packet_header

Add custom gpg exceptions that may be raised when parsing gpg packet payloads (e.g. for public key or signature packets). - PacketVersionNotSupportedError - SignatureAlgorithmNotSupportedError

This function is mostly identical with parse_pubkey_packet (which it will replace in a subsequent commit) except that it takes the payload of a pubkey packet instead of the whole packet. The payload can be obtained by using in_toto.gpg.util.parse_packet_header This function is useful for a function that decides whether to parse the payload of the packet based on info from the header.

This function takes the pubkey data received from the gpg export pubkey command and parses out the main pubkey and optional sub pubkeys.

Modify gpg_export_pubkey to call export_pubkey_bundle that implements parsing the main pubkey and optional subkeys from the output of the gpg export command.

The function is replaced by parse_pubkey_payload which the payload parsing routines were moved to and by parse_pubkey_bundle, which iterates over the entire pubkey data as returned by gpg's export command and parses the header for each packet, and depending on the packet type, then parses the payload.

We don't support elgamal keys (used for encryption rather than signing). The key is added to test an unsupported algorithm guard. This commit also updates the unittests for keybundle parsing.

Replaces a 0 with a 9 as mandated by RFC4880 section 5.2.3.1 Furthermore updates more comments and removes an obsolete variable assignment.

The field can be used for the short gpg fingerprint as contained in the gpg signature type 16 issuer subpacket (see rfc4880 5.2.3.1). However, the short fingerprint should not be used for signature verification, as keys and signatures must be matched by their full keyids. The short keyid should only be used determine the full keyid.

So far parse_signature_packet only returned the full keyid as taken from subpacket 33, which is not part of rfc4880 but an experimental addition only available in newer versions of gpg https://archive.cert.uni-stuttgart.de/openpgp/2016/06/msg00004.html This commit also parses the short keyid from subpacket 16 (see of rfc4480 5.2.3.1) and returns it as additional optional field in the signature dictionary. The commit also updates the parse_subpackets function to return the payload without its first byte, which describes the subpacket type and is already returned separately, i.e.: i.e. [(<type>, <type><payload>), ...] -> [(<type>, <payload>), ...]

We used to require the caller to supply a gpg keyid, to identify the signing key's full keyid (via pubkey export), when creating a signature with versions of gpg that don't add full keyids to signatures. This approach is not only inconvenient but also error prone when dealing with key bundles. Since a caller could e.g. pass the master key's keyid, while gpg actually signs with a subkey, which would entail storing the wrong keyid (passed master) for the signature (automatically used sub). This commit changes the signing function to make use of the short keyid, which per rfc4880 section 5.2.3.1 should always be on the signature, when the full keyid is not present. With the help of the short keyid we export a public key bundle, i.e all associated master and subkeys and filter out the full keyid with the matching suffix, which we then add to the signature. This change also makes the unit test case handling obsolete that suppressed signing tests with defaulft keyid for older versions of gpg (travis). These case handlings are also removed by this commit.

Since gpg version case handling was removed in a previous commit the related functions get_versions and is_version_fully_supported aren't used any more. This commit basic tests for these functions to maintain coverage. However, we might consider removing the two functions altogether.

The 4 test scenarios that include signing with a master key, did not work as expected, since there is no way (known to me) to sign with a gpg master key, if the key has a signing subkey. This commit changes the MMM (see TestCase docstring) scenario to use a key without subkeys and the MMS, MSM and MSS scenarios to just assert for a mismatch of passed key and signing key. Since gpg automatically uses a subkey if available, the scenarios as they were, would translate to scenarios where a subkey is used intentionally, like so: MMM -> SMM MMS -> SMS MSM -> SSM MSS -> SSS

coveralls · 2018-03-01T22:05:22Z

Coverage remained the same at 100.0% when pulling 93b78d7 on lukpueh:gpg-subkeys into 47685a9 on in-toto:develop.

coveralls · 2018-03-01T22:05:22Z

Coverage remained the same at 100.0% when pulling 90ed9fa on lukpueh:gpg-subkeys into e53abb1 on in-toto:develop.

coveralls · 2018-03-01T22:05:22Z

Coverage remained the same at 100.0% when pulling 90ed9fa on lukpueh:gpg-subkeys into e53abb1 on in-toto:develop.

vladimir-v-diaz · 2018-03-07T15:16:40Z

in_toto/gpg/constants.py

@@ -27,6 +27,7 @@
 PACKET_TYPES = {
    'signature_packet': 0x02,
    'main_pubkey_packet': 0x06,


I recommend using the standard name for this, like master_pubkey_packet.

vladimir-v-diaz

Here is my review of formats.py.

vladimir-v-diaz · 2018-03-07T15:24:57Z

in_toto/gpg/formats.py

+  """Helper method to extend the passed public key schema with an optional
+  dictionary of sub public keys "subkeys" with the same schema."""
+  schema = pubkey_schema
+  subkey_schema_tpl =  ("subkeys", ssl_schema.Optional(


What is tpl? tuple? I would just add the two extra letters to avoid confusion.

vladimir-v-diaz · 2018-03-07T15:27:07Z

in_toto/gpg/formats.py

@@ -69,7 +84,9 @@
 )


-RSA_PUBKEY_SCHEMA = ssl_schema.Object(
+# Internal temporary rsa public key schema, required for self-referential


Why is this temporary? The comment should explain. It might help a future developer working on this to understand why it's temporary, and why it should later be removed.

vladimir-v-diaz · 2018-03-07T15:29:27Z

in_toto/gpg/formats.py

+        )
+      )
+  # TODO: Find a way that does not require to access a proteced member
+  schema._required.append(subkey_schema_tpl) # pylint: disable=protected-access


typo: proteced

vladimir-v-diaz · 2018-03-07T15:33:59Z

in_toto/gpg/formats.py

+        )
+      )
+  # TODO: Find a way that does not require to access a proteced member
+  schema._required.append(subkey_schema_tpl) # pylint: disable=protected-access


I don't quite understand this use of _required(). The comment above stated that this is optional, but then this _required() call is here.

vladimir-v-diaz · 2018-03-07T15:41:19Z

in_toto/gpg/constants.py

@@ -27,6 +27,7 @@
 PACKET_TYPES = {
    'signature_packet': 0x02,
    'main_pubkey_packet': 0x06,
+    'pub_subkey_packet': 0x0E,


Are these arbitrarily chosen hexadecimal integers, or do they match the ones defined in RFC 4880?

I'll add a pointer to the RFC.

vladimir-v-diaz

Here is my review of util.py.

vladimir-v-diaz · 2018-03-07T16:02:42Z

in_toto/gpg/util.py

  """
  <Purpose>
-    Parse an RFC4880 packet header and return its payload
+    Parse an RFC4880 packet header and return its payload, length and type.

  <Arguments>


expected_type is not listed under <Arguments>.

Added in 93b78d7

vladimir-v-diaz · 2018-03-07T16:14:09Z

in_toto/gpg/util.py

@@ -118,11 +117,11 @@ def parse_packet_header(data, expected_type):
    packet_length = data[1]
    ptr = 2

-  if packet_type != expected_type: # pragma: no cover
+  if expected_type != None and packet_type != expected_type: # pragma: no cover


You should probably also document the supported values for expected_type.

True, @SantiagoTorres, you mentioned that you considered removing this argument. Should we do that? Otherwise, I can add the documentation.

I think that for now is worth documenting. The value of expected type is one of the enums here

Added values to docstring in 93b78d7

vladimir-v-diaz

Here is my review of verifylib.py.

vladimir-v-diaz · 2018-03-07T16:46:43Z

in_toto/verifylib.py

@@ -282,6 +283,10 @@ def verify_link_signature_thresholds(layout, chain_link_dict):
    link dictionary containing only authorized links whose signatures
    were successfully verified.

+    NOTE: Note if the layout's key store (`layout.keys`) lists a key `K`, with
+    a subkey `K'`, then `K'` is authorized implicitly, to sign any link that
+    `K` is authorized to sign. The inverse is not true.


Can you explain why K is not implicitly trusted if K' is?

vladimir-v-diaz · 2018-03-07T17:00:53Z

in_toto/verifylib.py

        log.info("Skipping link. Keyid '{0}' is not authorized to sign links"
-            " for step '{1}'".format(keyid, step.name))
+            " for step '{1}'".format(link_keyid, step.name))


Would it help to append the list of authorized keyids?

I think it's okay that way.

vladimir-v-diaz · 2018-03-08T20:52:29Z

Okay, I reviewed the code changes and glanced at the tests. I just had minor comments.

Add missing argument description (expected_type).

lukpueh · 2018-03-09T20:39:38Z

Thanks for your review @vladimir-v-diaz. Merging ...

The gpg utility functions `is_version_fully_supported` and `get_version`, where used in in-toto tests to case-handle different gpg versions. With in-toto/in-toto#190 this is no longer needed. See most notably: in-toto/in-toto@28ba993 in-toto/in-toto@c3a40b8 To reduce the API surface and maintenance burden this commit removes those functions. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

The gpg utility functions `is_version_fully_supported` and `get_version` were used in in-toto tests to case-handle different gpg versions. With in-toto/in-toto#190 this is no longer needed. See in-toto/in-toto@28ba993, in-toto/in-toto@c3a40b8. To reduce the API surface and maintenance burden this commit removes those functions. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

SantiagoTorres and others added 21 commits February 26, 2018 16:50

WIP: gpg: add signing subkey support

0717af7

Add optional subkeys to gpg rsa/dsa pubkey schema

d0ed762

This commit extends RSA_PUBKEY_SCHEMA and DSA_PUBKEY_SCHEMA with an optional "subkeys" field that contains a dictionary with sub keyids as keys and subkeys in the corresponding schema as values.

Add custom KeyNotFoundError error to gpg module

b2a2f26

Add unittests for gpg subkey verification

7324c76

Link signing, per-step key authorization and the corresponding layout key store value together with main keys and subkeys allows for 8 combinations of which some are possible to verify and others not. See test case docstring for more info.

Add better rfc refs to gpg util function

a4b1e54

Add reference to RFC4880 section 4.2 that describes packet headers to in_toto.gpg.util.parse_packet_header

Add custom gpg packet parsing exceptions

ee59275

Add custom gpg exceptions that may be raised when parsing gpg packet payloads (e.g. for public key or signature packets). - PacketVersionNotSupportedError - SignatureAlgorithmNotSupportedError

Add gpg parse_pubkey_bundle function

105d625

This function takes the pubkey data received from the gpg export pubkey command and parses out the main pubkey and optional sub pubkeys.

Use parse bundle in gpg_export_pubkey

e344f7f

Modify gpg_export_pubkey to call export_pubkey_bundle that implements parsing the main pubkey and optional subkeys from the output of the gpg export command.

Add elgamal subkey to rsa main test key

269f322

We don't support elgamal keys (used for encryption rather than signing). The key is added to test an unsupported algorithm guard. This commit also updates the unittests for keybundle parsing.

Fix minor bug in gpg signature subpacket parser

137d328

Replaces a 0 with a 9 as mandated by RFC4880 section 5.2.3.1 Furthermore updates more comments and removes an obsolete variable assignment.

vladimir-v-diaz reviewed Mar 7, 2018

View reviewed changes

lukpueh added 5 commits March 9, 2018 14:02

Replace occurences of gpg main key with master key

0909b22

Add RFC4880 references comments to gpg constants

90da843

Rephrase comments for self-referential gpg formats

4a30303

Add note about sub keytrust to verify link sigs

d224022

Update gpg.util.parse_packet_header docstring

93b78d7

Add missing argument description (expected_type).

lukpueh force-pushed the gpg-subkeys branch from a2416b6 to 93b78d7 Compare March 9, 2018 20:26

lukpueh merged commit 3244e1a into in-toto:develop Mar 9, 2018

lukpueh mentioned this pull request Feb 9, 2023

Remove obsolete gpg version utils secure-systems-lab/securesystemslib#504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GPG signing subkey support #190

Add GPG signing subkey support #190

lukpueh commented Mar 1, 2018

coveralls commented Mar 1, 2018 •

edited

Loading

coveralls commented Mar 1, 2018

coveralls commented Mar 1, 2018

vladimir-v-diaz Mar 7, 2018

vladimir-v-diaz left a comment

vladimir-v-diaz Mar 7, 2018

vladimir-v-diaz Mar 7, 2018

vladimir-v-diaz Mar 7, 2018

vladimir-v-diaz Mar 7, 2018

vladimir-v-diaz Mar 7, 2018

lukpueh Mar 7, 2018

vladimir-v-diaz left a comment

vladimir-v-diaz Mar 7, 2018

lukpueh Mar 9, 2018

vladimir-v-diaz Mar 7, 2018

lukpueh Mar 7, 2018

SantiagoTorres Mar 9, 2018

lukpueh Mar 9, 2018

vladimir-v-diaz left a comment

vladimir-v-diaz Mar 7, 2018

vladimir-v-diaz Mar 7, 2018

lukpueh Mar 9, 2018

vladimir-v-diaz commented Mar 8, 2018

lukpueh commented Mar 9, 2018

Add GPG signing subkey support #190

Add GPG signing subkey support #190

Conversation

lukpueh commented Mar 1, 2018

coveralls commented Mar 1, 2018 • edited Loading

coveralls commented Mar 1, 2018

coveralls commented Mar 1, 2018

Choose a reason for hiding this comment

vladimir-v-diaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-v-diaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-v-diaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-v-diaz commented Mar 8, 2018

lukpueh commented Mar 9, 2018

coveralls commented Mar 1, 2018 •

edited

Loading