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

Adds keyid_hash_algorithms to returned key objects #37

Merged
merged 2 commits into from
Jun 14, 2017

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Jan 27, 2017

Fixes #36 and, once integrated and TUF is updated to use the new version, theupdateframework/python-tuf#412.

The functions listed below all return key objects that can carry an optional keyid_hash_algorithms field according to their schema. With this PR we always add the keyid_hash_algorithm field to the returned object, in:

interface.import_ed25519_privatekey_from_file
interface.import_ecdsa_privatekey_from_file
keys.generate_ecdsa_key
keys.import_rsakey_from_public_pem
keys.import_rsakey_from_pem
keys.import_ecdsakey_from_private_pem
keys.import_ecdsakey_from_public_pem

e.g.:

>>> from securesystemslib import interface
>>> interface.generate_and_write_ed25519_keypair('testkey', 'pw')
>>> interface.import_ed25519_privatekey_from_file('testkey', 'pw')
{
  u'keytype': u'ed25519',
  u'keyid': u'8f68ef937c031aad4862aa0bac8b45b719c4f2ba8f186b289ba66991f82c8041',
  u'keyval': {
    u'public': u'78fa9cf9b787beb754806c15cf88950a5e3c30ff8cad57dba79c623fd8c5ae00',
    u'private': u'f8919b50f6ea8c4e2060e6177970b7e8cde97950ff284973fbe54485e78689c1'
  },
  u'keyid_hash_algorithms': [u'sha256', u'sha512']
}

>>> interface.import_ed25519_publickey_from_file('testkey.pub')
{
  u'keytype': u'ed25519',
  u'keyid': '8f68ef937c031aad4862aa0bac8b45b719c4f2ba8f186b289ba66991f82c8041',
  u'keyval': {
    u'public': u'78fa9cf9b787beb754806c15cf88950a5e3c30ff8cad57dba79c623fd8c5ae00'
  },
  u'keyid_hash_algorithms': [u'sha256', u'sha512']
}

The PR also changes PUBLIC_KEY_SCHEMA to accept the optional keyid_hash_algorithm (like ANYKEY_SCHEMA).

The reason for adding this field is to be able to associate a key with e.g. a signature created by that key, but listing a different keyid, because a different keyid hash algorithm was used. By trying all possible algorithms, the key can still be matched.

PUBLIC_KEY_SCHEMA used to behave like KEY_SCHEMA (without
optional private portion).
This commit changes it to behave like the newer ANYKEY_SCHEMA
(without optional private portion) which adds an optional
keyid_hash_algorithm.
The functions listed below all return key objects that can carry
an optional keyid_hash_algorithms field according to their schema.

This commit adds the keyid_hash_algorithm field to the returned
object of:

interface.import_ed25519_privatekey_from_file
interface.import_ecdsa_privatekey_from_file
keys.generate_ecdsa_key
keys.import_rsakey_from_public_pem
keys.import_rsakey_from_pem
keys.import_ecdsakey_from_private_pem
keys.import_ecdsakey_from_public_pem

The reason to add this field is to be able to associate a key
with e.g. a signature created by that key, but listing a different
keyid, because a different keyid hash algorithm was used.
By trying all possible algorithms, the key can still be matched.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ceefa7c on lukpueh:update-import-keys into b535fc2 on secure-systems-lab:master.

@vladimir-v-diaz
Copy link
Contributor

Can you please include a code snippet of the changed behavior, similar to how @Awaad demonstrated the problem in the original issue?
theupdateframework/python-tuf#412

@vladimir-v-diaz vladimir-v-diaz merged commit ceefa7c into secure-systems-lab:master Jun 14, 2017
@@ -237,10 +237,11 @@
keyval = KEYVAL_SCHEMA,
expires = SCHEMA.Optional(ISO8601_DATETIME_SCHEMA))

# Like KEY_SCHEMA, but requires keyval's private portion to be not set or empty
# Like ANYKEY_SCHEMA, but requires keyval's private portion to be not set or empty
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: I don't see any major issues with this pull request, but I will make minor edits to your pull request for any minor issues. I will share my in-line comments on this pull request for documentation purposes.

This SCHEMA is more like KEY_SCHEMA, since ANYKEY_SCHEMA contains an attribute (keyid) that ANYKEY_SCHEMA doesn't. PUBLIC_KEY_SCHEMA matches KEY_SCHEMA, and will now differ with the keyid_hash_algorithms and keyval attributes.

@@ -237,10 +237,11 @@
keyval = KEYVAL_SCHEMA,
expires = SCHEMA.Optional(ISO8601_DATETIME_SCHEMA))

# Like KEY_SCHEMA, but requires keyval's private portion to be not set or empty
# Like ANYKEY_SCHEMA, but requires keyval's private portion to be not set or empty
PUBLIC_KEY_SCHEMA = SCHEMA.Object(
object_name = 'KEY_SCHEMA',
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong object_name. Should be PUBLIC_KEY_SCHEMA.

@@ -1316,6 +1326,11 @@ def import_rsakey_from_pem(pem):
rsakey_dict['keyid'] = keyid
rsakey_dict['keyval'] = key_value

# Add "keyid_hash_algorithms" so equal ecdsa keys with
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "so that equal RSA keys ..."

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