Sign and verify #2

PradyumnaKrishna · 2022-06-22T03:53:54Z

Fixes: #

Description of the changes being introduced by the pull request:

This pull request is related to DSSE implementation and adds methods to create and verify DSSE signatures.

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

PradyumnaKrishna

I have few doubts regarding the implemented classes and methods.

securesystemslib/key.py

securesystemslib/metadata.py

securesystemslib/signer.py

lukpueh

Great work, @PradyumnaKrishna! I will take a closer look tomorrow. Left some replies to your comments inline...

securesystemslib/metadata.py

securesystemslib/signer.py

securesystemslib/key.py

PradyumnaKrishna

I have added comments about some points I noticed. I think now this PR is ready for review.

securesystemslib/metadata.py

MVrachev

@PradyumnaKrishna first of all, I want to say I am really happy that you are helping us spend so much time writing and thinking about this!

I have lots of comments which shouldn't discourage you as many of those can be considered nitpicks (especially those about the final .).
Most of them are stylish changes.

Additional comment about commit messages: focus on explaining why you made the change and not so much on what you changed.

securesystemslib/key.py

securesystemslib/metadata.py

tests/test_metadata.py

PradyumnaKrishna

@MVrachev, I tried to address almost all of your comments, thank you for your feedback.

securesystemslib/key.py

securesystemslib/metadata.py

PradyumnaKrishna · 2022-06-28T02:46:32Z

securesystemslib/metadata.py

+            raise TypeError(f"expected type 'Signer', got {type(signer).__name__}")
+
+        signature = signer.sign(self.pae)
+        self.signatures.append(signature)


Not sure about it, according to current signature wrappers:
in-toto: signatures is a list and appends the signatures.
python-tuf: signatures is a dict and appends with keyid.

Also, in DSSE keyid is optional. But what I can do is:

Suggested change

self.signatures.append(signature)

if signature.keyid:

for s in self.signatures:

if s.keyid == signature.keyid:

self.signatures.remove(s)

break

self.signatures.append(signature)

tests/test_key.py

securesystemslib/key.py

tests/test_metadata.py

securesystemslib/key.py

securesystemslib/signer.py

securesystemslib/key.py

MVrachev · 2022-06-28T12:48:30Z

securesystemslib/metadata.py

+            raise TypeError(f"expected type 'Signer', got {type(signer).__name__}")
+
+        signature = signer.sign(self.pae)
+        self.signatures.append(signature)


in-toto: signatures is a list and appends the signatures.
Doesn't in-toto make a check if a signature already exists?

In python-tuf to add a new signature you should use sign: https://github.com/theupdateframework/python-tuf/blob/8a03abfdeb5a6b6bc892859f65425be0834893a8/tuf/api/metadata.py#L342.
In sign there is an append option which if used will add an additional signature.
If append is True and a key is already used to sign a file, then the signature will exist in the dictionary will be replaced by a new Signature instance with the same content: https://github.com/theupdateframework/python-tuf/blob/8a03abfdeb5a6b6bc892859f65425be0834893a8/tuf/api/metadata.py#L389.
This removes the possibility of two existing Signature instances with the same content.

The question is do we want to allow duplicating signatures?
I think duplicating signatures could be a problem because they artificially increase the numbers of signatures used and thus threshold loses its importance.
I don't think an attacker can utilize that as for that he/she will need the private key used for signing (and if he has that he can break the system on multiple levels), so maybe I shouldn't worry.

@lukpueh any thoughts?

PS: If you want to remove an already existing signature you can directly use == and not rely on keyid existing in a signature as a __eq__ implementation exist for the Signature class:

securesystemslib/securesystemslib/signer.py

Line 40 in d3f4888

def __eq__(self, other: Any) -> bool:

MVrachev

@PradyumnaKrishna much better with your latest changes, thanks.

A few additional things that I noticed.

PS: My comments which I mark with 👍 you can mark as resolved.

securesystemslib/metadata.py

securesystemslib/exceptions.py

tests/test_key.py

tests/test_metadata.py

PradyumnaKrishna · 2022-06-29T13:52:06Z

@MVrachev updated the code, please review it.

MVrachev

I don't see any other points that need modification.
Great work, @PradyumnaKrishna

securesystemslib/metadata.py

adityasaky · 2022-06-30T19:10:03Z

Left some comments, @PradyumnaKrishna, I think this PR is in great shape!

adityasaky

I think this is good to go, bar the error message here: https://github.com/in-toto/securesystemslib/pull/2/files#r912183095

securesystemslib/metadata.py

MVrachev · 2022-07-02T13:01:04Z

securesystemslib/metadata.py

+            raise TypeError(f"expected type 'Signer', got {type(signer).__name__}")
+
+        signature = signer.sign(self.pae)
+        self.signatures.append(signature)


I still think that in order to escape possible problems regarding threshold computation it's better if we don't allow duplicating signatures. They just don't make sense to exist.
@adityasaky maybe before merging, we can wait for @lukpueh input here?

lukpueh · 2022-07-04T16:41:43Z

Thanks for the great work, @PradyumnaKrishna, and all your comments, @MVrachev! I'll try to follow up with a proper review tomorrow.

Regarding signature thresholds: Any API-imposed constraints about modifying the unauthenticated signature list have no effect on security. An attacker could easily bypass it by hand-crafting the metadata. So the duplicates/threshold check really needs to happen at verification time. That said, disallowing duplicates in sign could prevent some footguns. I'm fine either way.

lukpueh

Great work as always, @PradyumnaKrishna! Here are some thoughts about metadata.py only. I'll follow-up with more comments about the other changed modules shortly.

securesystemslib/metadata.py

lukpueh · 2022-07-05T09:25:53Z

securesystemslib/metadata.py

+
+        # checks for threshold value.
+        if len(keys) < threshold or threshold <= 0:
+            raise ValueError(f"Threshold must be between 0 and {len(keys)}")


Minor nit: I'd make two separate checks -- 1. threshold <= 0, 2. len(keys) < threshold -- with separate error messages here, because you are checking two different arguments.

lukpueh · 2022-07-05T10:38:00Z

securesystemslib/metadata.py

+                if key.verify(signature, pae):
+                    if key.keyid:
+                        # If keyid has already been verified, skip.
+                        if key.keyid in used_keyids:


Let's not use the keyid to de-duplicate keys. The dsse spec makes it explicit that keyids must not be used for security decisions.

The way I read the spec ...

A (t, n)-ENVELOPE is valid if the enclosed signatures pass the verification against at least t of n unique trusted public keys.

... we need to trust the caller to pass a list of "unique trusted" keys. So I would say you can remove the used_keyids list and just check if the name is already in recognized_signers (btw. using a set might make this a bit faster). What do you think?

Might be a good idea to check with the dsse spec authors, if this assumption is correct. Mind creating an issue in the spec repo?

Either way we should definitely mention this expectation regarding the passed key list in the docstring.

I took it from dsse golang implementation, this is related to gpg subkeys.

I understand where it comes from, but, reading the dsse spec, I don't think it shouldn't be part of this verify method.

Can you see why?

Ok, I will remove this piece of code.

Can you see why?

GPG implementation is not done yet, and thats why I am not sure about this.

Okay, I'm trying to say that the question of trust, realized via keyids, is not in the scope of DSSE. This is something that the caller should deal with.

If I understand the spec correctly, DSSE just receives a list of public keys for a set of signatures, then it iterates over the signatures and tries to verify each signature, with each key, unless the key has been used before. If a threshold of signatures has been verified, we return success.

DSSE itself does not need to check if the keys are really distinct or if they are really trusted. It also does not need to check if the signatures are really distinct. This should happen implicitly when using distinct keys and using each key only once.

securesystemslib/metadata.py

lukpueh

Some thoughts about the optional keyid in the Signature class. It's not so easy to figure out a solution that is compatible for both the traditional tuf/in-toto signature wrapper and the new dsse spec, and also has an intuitive API.

securesystemslib/signer.py

PradyumnaKrishna · 2022-07-20T09:54:09Z

securesystemslib/metadata.py

+            recognized_signers: A set of recognized public keys.
+        """
+
+        recognized_signers = set()


As protocol is updated, and recognized_signers are unique ACCEPTED_KEYS. So, I decided to make it a set, and set requires a hashable type. Hence, I created __hash__ method for Key.
The __hash__ method can be found in the respective SSlibKey and GPGKey. I decided to not take optional fields like expires, created_at and keyid in hash computation.

Review the method and provide your thought about any field can be added or removed in hash computation.

This is a bit tricky. In the case of SSlibKey.__hash__ the "opaque" key content is now required to be hashable, which is probably okay, but it makes it less "opaque". And in the case of GPGKey.__hash__, you are using non-canonical dict/json, which is not guaranteed to always yield the same data for the same key.

I suggest to use keyid as unique identifier.

I am not sure about keyid at this point because its somewhere between optional and required fields..

The other solution to hashable is, we can create a container for ACCEPTED_KEYS (KeyList). which will have an add method and we can write an optimized algorithm to add unique keys to it.

I really think keyid should be mandatory for Key objects. See #2 (comment).

More details about why keyids should be mandatory in a Key object:
secure-systems-lab#310 (comment)

PradyumnaKrishna · 2022-07-20T10:04:39Z

@lukpueh, If I tries to create a test case for GPG RSA and DSSE envelope similar to:

securesystemslib/tests/test_signer.py

Lines 132 to 149 in 6a918dc

    
           def test_gpg_sign_and_verify_object_with_default_key(self): 
        
               """Create and verify a signature using the default key on the keyring.""" 
        
               # Create a signature. 
        
               signer = GPGSigner(homedir=self.gnupg_home) 
        
               signature = signer.sign(self.test_data) 
        
               # Generate Key from gnupg keyring. 
        
               key = GPGKey.from_keyring(self.default_keyid, self.gnupg_home) 
        
               self.assertTrue(key.verify(signature, self.test_data)) 
        
               self.assertFalse(key.verify(signature, self.wrong_data)) 
        
               # Generate Key from dict. 
        
               key = GPGKey.from_dict(self.default_key_dict) 
        
               self.assertTrue(key.verify(signature, self.test_data)) 
        
               self.assertFalse(key.verify(signature, self.wrong_data))

It fails because GPGKey keyid is different than keyid present in the signature created and due to the fact that protocol filters keys and signatures according to keyid.

Thats not a case for GPG DSA and GPG EdDSA according to my testing.

lukpueh · 2022-07-20T11:27:52Z

It fails because GPGKey keyid is different than keyid present in the signature created and due to the fact that protocol filters keys and signatures according to keyid.

How do the keyids differ? They should both be self.default_keyid in lower case, on the signature and on the key. Can you point me to the failing test? Or show me a dump of the two objects (in dict representation)?

PradyumnaKrishna · 2022-07-20T11:40:30Z

How do the keyids differ?

GPGRSA has multiple keyids, signature stores signing subkey keyid and public key has default keyid for some reason.

securesystemslib/tests/test_gpg.py

Lines 487 to 494 in 6a918dc

    
           class TestGPGRSA(unittest.TestCase): 
        
             """Test signature creation, verification and key export from the gpg 
        
             module""" 
        
             default_keyid = "8465A1E2E0FB2B40ADB2478E18FB3F537E0C8A17" 
        
             signing_subkey_keyid = "C5A0ABE6EC19D0D65F85E2C39BE9DF5131D924E9" 
        
             encryption_subkey_keyid = "6A112FD3390B2E53AFC2E57F8FC8E12099AECEEA" 
        
             unsupported_subkey_keyid = "611A9B648E16F54E8A7FAD5DA51E8CDF3B06524F" 
        
             expired_key_keyid = "E8AC80C924116DABB51D4B987CB07D6D2C199C7C"

GPGSignature dict

{
    "keyid": "c5a0abe6ec19d0d65f85e2c39be9df5131d924e9",
    "signature": "2476dc5e357812ea382f7681d634f5510a51b5acd4f01e38b5f18c0d21960d32854eb27a6e824c89761731f24dd39361de757e3bdf15e4f4bace98c6f5b3f75842b9afaf098c5913e6d088c0a6f147d739d1977b12363e33cfaa462ddbebb504103d42732d0acd39a1a27933b395c8fc5de19aab3a1bfbcd88b58b0da0c33f2fc7e72f3da6ac99bbf80c3739df8d842b66bf52ce618b11b3c73c17b34d6f6aa9b723cdd4653fac525aa1d06e07fb362ba8b70e031366a143692f7a5ef30334e03ffd4123148250d6b97e9b1c93d8f27dad34e45792b2af99df8715335902c22a13143ca48a447cd206fbec96b3d481da6a7485ca4dcfdb30cb5f19df0ab6ccff",
    "other_headers": "04000108001d162104c5a0abe6ec19d0d65f85e2c39be9df5131d924e9050262d7e935"
}

GPGKey dict

{
    "method": "pgp+rsa-pkcsv1.5",
    "type": "rsa",
    "hashes": [
        "pgp+SHA2"
    ],
    "keyid": "8465a1e2e0fb2b40adb2478e18fb3f537e0c8a17",
    "keyval": {
        "private": "",
        "public": {
            "e": "010001",
            "n": "da59409e6ede307a52f6851954a7bd4b9e309bd40a390f8c0de9722b6310110ef0b095bf1c473e33db97150edae05c63dda70c03902701b15f3c5c308947e1b06675b4f1112030f1145be84ae1562e9120c2d429b20d5056337cbc97fc8b5db5704a21db635d00b2157ed68a403c793e9958b77e00163f99b01809e08ee9099b99b117c086501e79eb947f760a0715bead0024c48d81f9000671c4306a93725965f3ff2dc9806eaf081357f0268cab8ba7582d2e95e51225a9dc7ed31a9568c45568d7917b05e7c954d561cd084291e77a7bdd69e3ac2f9091de55fe3f4e730147e880e2fc044c5f7c04c75ce33a3c0b52380f4d60309708c56185f3bce6703b"
        }
    },
    "creation_time": 1510870143,
    "subkeys": {
        "6a112fd3390b2e53afc2e57f8fc8e12099aeceea": {
            "method": "pgp+rsa-pkcsv1.5",
            "type": "rsa",
            "hashes": [
                "pgp+SHA2"
            ],
            "keyid": "6a112fd3390b2e53afc2e57f8fc8e12099aeceea",
            "keyval": {
                "private": "",
                "public": {
                    "e": "010001",
                    "n": "a40806286bfeb2bee9f36d9953ef2f83265f2b802024981775ce8498f6f08a6d7ea14dcecc1b82013b71e7d93d6ecb294f8880ee703e01a73cb45d8740221671a04f4b6cdf79f44438c7b8b8b4963d4308e0f40dbe18aee79e48fdf9fecb02661b76da7ba17a5dffa88afc8984743d3a033879a26384cce12350e25068ec38d4e3d7075176b6020882ca81a12d174df864dc59f11e19f98eec5e5e22d0ab6cc32bf1dd70cf4c2ca7c6aa1be48da0f94dcebc2f3c425d0d1e536e01f01ce5dfdc655f2994ccadd120469d55e2ad2f1e9c3483abcfefcf959d600a2ebb7990b46841b15d25537d8f1fbeda9347ffa466ccfcceca4567f9a7d84dd767fdf037fe97"
                }
            },
            "creation_time": 1510870143
        },
        "c5a0abe6ec19d0d65f85e2c39be9df5131d924e9": {
            "method": "pgp+rsa-pkcsv1.5",
            "type": "rsa",
            "hashes": [
                "pgp+SHA2"
            ],
            "keyid": "c5a0abe6ec19d0d65f85e2c39be9df5131d924e9",
            "keyval": {
                "private": "",
                "public": {
                    "e": "010001",
                    "n": "c152fc1f1535a6d3c1e8c0dece7f0a1d09324466e10e4ea51d5d7223ab125c1743393eebca73ccb1022d44c379fae30ef63b263d0a793882a7332ef06f28a4b9ae777f5d2d8d289167e86c162df1b9a9e127acb26803688556ecb08492d071f06caf88cea95571354349d8ef131eff03b0d259fae30ebf8dac9ab5acd6f26f4770fe2f30fcd0a3c54f03463a3094aa6524e39027a625108f04e12475da248fb3b536df61b0f6e2954739b8828c61171f66f8e176823e1c887e65fa0aec081013b2a50ed60515f7e3b3291ca443e1222b9b625005dba045a7208188fb88d436d473f6340348953e891354c7a5734bf64e6274e196db3074a7ce3607960baacb1b"
                }
            },
            "creation_time": 1519661780
        }
    }
}

lukpueh · 2022-07-20T11:49:09Z

GPGRSA has multiple keyids, signature stores signing subkey keyid and public key has default keyid for some reason.

What do you think how we should deal with subkeys here?

PradyumnaKrishna · 2022-07-20T11:56:41Z

What do you think how we should deal with subkeys here?

Create a function to return a list of all keyids of a Key and check if keyid is present in the list on verify.
Thats how gpg verify function selects keyid

securesystemslib/securesystemslib/gpg/functions.py

Lines 221 to 226 in 6a918dc

    
           verification_key = pubkey_info 
        
           # If the keyid on the signature matches a subkey of the passed key, 
        
           # we use that subkey for verification instead of the master key. 
        
           if sig_keyid in list(pubkey_info.get("subkeys", {}).keys()): 
        
             verification_key = pubkey_info["subkeys"][sig_keyid]

lukpueh · 2022-07-20T12:58:12Z

Create a function to return a list of all keyids of a Key and check if keyid is present in the list on verify.

In GPGKey.verify or in Envelope.verify?

PradyumnaKrishna · 2022-07-20T14:07:47Z

In GPGKey.verify or in Envelope.verify?

https://github.com/in-toto/securesystemslib/pull/2/files#diff-2864d1ba8b39b6e64cba34ae3125c3b2ae03d57aa7c4efae44e8381cf11632bfR138-R143

Here, we can use

keyids = key.get_keyids()
if signature.keyid not in keyids:
    continue

lukpueh · 2022-07-20T15:00:55Z

In GPGKey.verify or in Envelope.verify?

https://github.com/in-toto/securesystemslib/pull/2/files#diff-2864d1ba8b39b6e64cba34ae3125c3b2ae03d57aa7c4efae44e8381cf11632bfR138-R143

Here, we can use
keyids = key.get_keyids()
if signature.keyid not in keyids:
    continue

Good idea. Although get_keyids wouldn't make much sense on Key implementations that don't have subkeys. Maybe it could be a method that takes the signature and returns True if the keyids match, which can be implemented in different ways in the different Key implementations.

lukpueh · 2022-07-21T07:57:18Z

securesystemslib/metadata.py

+                    continue
+
+                # If a key verifies the signature, we exit and use the result.
+                if key.verify(signature, pae):


FYI and because you asked about how to deal with exceptions that might be raised in Key.verify:

theupdateframework/python-tuf@414dfc8 discusses returning boolean vs. raising exception in Key.verify and opts for the latter.

I suggest we change our Key implementations to do the same thing, i.e. return None if signature verification passes and raise an exceptions (or rather different exceptions with a common base) in all other cases. Then the caller does not have to case handle True/False/Error but only error type.

I also suggest to do it in a separate PR.

I suggest we change our Key implementations to do the same thing, i.e. return None if signature verification passes and raise an exceptions (or rather different exceptions with a common base) in all other cases.

@PradyumnaKrishna, would you mind creating an issue for this?

match_keyid method checks and match a keyid with the keyids present in the Key instance. This method will be used to filter the key based on signature's keyid. Signed-off-by: Pradyumna Krishna <[email protected]>

* Added missing types in Envelope constructor. * Remove property tag from pre-auth-encoding. * Modify doc string of DSSE Envelope. Signed-off-by: Pradyumna Krishna <[email protected]>

lukpueh

Awesome work! The match_keyid solves the gpg subkey issue really elegantly without adding extra coupling.

Except for two minor nits this is good to go.

lukpueh · 2022-07-22T11:24:28Z

securesystemslib/metadata.py

+
+        for signature in self.signatures:
+            for key in keys:
+                # If Signature keyid doesn't matches with Key, skip.


Suggested change

# If Signature keyid doesn't matches with Key, skip.

# If Signature keyid doesn't match with Key, skip.

lukpueh · 2022-07-22T11:34:54Z

securesystemslib/metadata.py

+            raise ValueError("Threshold must be greater than 0")
+
+        if threshold > len(keys):
+            raise ValueError("Amount of keys must be greater than threshold")


The error message is still not accurate. The number of passed keys can also be equal to the threshold.

Here's what I meant by logical operator and error message should align

if len(keys) < threshold: raise ValueError("Number of keys can't be less than threshold")

Maybe it's just me, but I would argue that one sees immediately that condition and error message mean the same thing.

sign and verify methods are DSSE protocols used to create and verify DSSE signatures. sign uses a Signer to sign the payload and create DSSE a signature and verify method uses a Key List to verify all DSSE signatures with the payload present in the envelope. Signed-off-by: Pradyumna Krishna <[email protected]>

Tests to ensure integrity of creation and verification of DSSE signatures. Signed-off-by: Pradyumna Krishna <[email protected]>

Tests creation and verification of different types of GPGSignatures for a DSSE envelope. Signed-off-by: Pradyumna Krishna <[email protected]>

lukpueh · 2022-08-01T10:44:28Z

securesystemslib/metadata.py

+                    continue
+
+                # If a key verifies the signature, we exit and use the result.
+                if key.verify(signature, pae):


I suggest we change our Key implementations to do the same thing, i.e. return None if signature verification passes and raise an exceptions (or rather different exceptions with a common base) in all other cases.

@PradyumnaKrishna, would you mind creating an issue for this?

lukpueh · 2022-08-01T11:03:15Z

securesystemslib/metadata.py

+
+        Raises:
+            ValueError: If "threshold" is not valid.
+            SignatureVerificationError: If the amount of "recognized_signers" is less


Suggested change

SignatureVerificationError: If the amount of "recognized_signers" is less

SignatureVerificationError: If the amount of "accepted_keys" is less

Actually, accepted_keys is an implementation detail and does not need to be mentioned in the docstring. Maybe you can rephrase the sentence without using the variable name.

lukpueh · 2022-08-01T11:06:19Z

securesystemslib/metadata.py

+            keys: A list of "Key" class instance that is a public key to verify
+                the signatures.


Minor nit:

Suggested change

keys: A list of "Key" class instance that is a public key to verify

the signatures.

keys: A list of public keys to verify the signatures.

Should be good enough, right? The data type is shown by the type hint. 🤷

lukpueh · 2022-08-01T11:16:13Z

securesystemslib/metadata.py

+            for key in keys:
+                # If Signature keyid doesn't match with Key, skip.
+                if not key.match_keyid(signature.keyid):
+                    continue


Is it worth adding a TODO comment either here or in the docstring above that what we do here, i.e. mandating keyids in signatures in order to consider them for verification, is not dsse spec compliant?

see secure-systems-lab#416 (comment)

lukpueh · 2022-08-02T08:00:54Z

securesystemslib/metadata.py

+
+        Note:
+            DSSE signatures created have mandatory keyid due to underlying sign
+            methods (Issue #416) and is not according to DSSE specification.


Suggested change

Note:

DSSE signatures created have mandatory keyid due to underlying sign

methods (Issue #416) and is not according to DSSE specification.

As I said in secure-systems-lab#416 (comment):

The DSSE spec does not require us to support keyid-less signing. This means it should be okay if all Signature objects returned with e.g. Signature.sign and serialized into a DSSE envelope with e.g. Signature.to_dict include a keyid.

So I think we can do without the note here.

lukpueh · 2022-08-02T08:09:07Z

securesystemslib/metadata.py

+            in order to consider them for verification, is not a DSSE spec
+            complaint (Issue #416).


complaint != compliant

Suggested change

in order to consider them for verification, is not a DSSE spec

complaint (Issue #416).

in order to consider them for verification, is not DSSE spec

compliant (Issue #416).

"compliant" is an adjective, hence my suggestion to also remove the article "a"

Signed-off-by: Pradyumna Krishna <[email protected]>

lukpueh · 2022-08-02T13:33:16Z

💯

PradyumnaKrishna force-pushed the sign_and_verify branch from 5eb47a2 to f1f84c3 Compare June 22, 2022 04:10

PradyumnaKrishna commented Jun 22, 2022

View reviewed changes

securesystemslib/key.py Outdated Show resolved Hide resolved

securesystemslib/metadata.py Outdated Show resolved Hide resolved

securesystemslib/metadata.py Outdated Show resolved Hide resolved

securesystemslib/signer.py Outdated Show resolved Hide resolved

lukpueh reviewed Jun 22, 2022

View reviewed changes

securesystemslib/metadata.py Outdated Show resolved Hide resolved

securesystemslib/metadata.py Outdated Show resolved Hide resolved

securesystemslib/signer.py Outdated Show resolved Hide resolved

securesystemslib/key.py Outdated Show resolved Hide resolved

PradyumnaKrishna force-pushed the sign_and_verify branch 3 times, most recently from 81bb2ee to 71878e9 Compare June 27, 2022 05:55

PradyumnaKrishna commented Jun 27, 2022

View reviewed changes

securesystemslib/metadata.py Outdated Show resolved Hide resolved

securesystemslib/metadata.py Outdated Show resolved Hide resolved

PradyumnaKrishna marked this pull request as ready for review June 27, 2022 10:35

adityasaky self-requested a review June 27, 2022 13:52

MVrachev suggested changes Jun 27, 2022

View reviewed changes

PradyumnaKrishna force-pushed the sign_and_verify branch 2 times, most recently from 65f812f to 3b0d53a Compare June 28, 2022 02:56

PradyumnaKrishna commented Jun 28, 2022

View reviewed changes

MVrachev reviewed Jun 28, 2022

View reviewed changes

MVrachev suggested changes Jun 28, 2022

View reviewed changes

PradyumnaKrishna force-pushed the sign_and_verify branch 3 times, most recently from 2da2b05 to 2905b27 Compare June 29, 2022 13:49

MVrachev approved these changes Jun 29, 2022

View reviewed changes

adityasaky reviewed Jun 30, 2022

View reviewed changes

securesystemslib/metadata.py Outdated Show resolved Hide resolved

PradyumnaKrishna mentioned this pull request Jul 1, 2022

keyid is not Optional in securesystemslib Signature secure-systems-lab/securesystemslib#416

Open

adityasaky approved these changes Jul 1, 2022

View reviewed changes

PradyumnaKrishna force-pushed the sign_and_verify branch 2 times, most recently from 737a536 to 36bf4fb Compare July 2, 2022 11:23

MVrachev reviewed Jul 2, 2022

View reviewed changes

lukpueh requested changes Jul 5, 2022

View reviewed changes

lukpueh reviewed Jul 5, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

securesystemslib/signer.py Outdated Show resolved Hide resolved

securesystemslib/signer.py Outdated Show resolved Hide resolved

PradyumnaKrishna force-pushed the sign_and_verify branch from 766da3c to 68e76ab Compare July 20, 2022 09:42

PradyumnaKrishna commented Jul 20, 2022

View reviewed changes

lukpueh reviewed Jul 21, 2022

View reviewed changes

PradyumnaKrishna added 2 commits July 21, 2022 14:13

Add match_keyid method in Key

a4b920d

match_keyid method checks and match a keyid with the keyids present in the Key instance. This method will be used to filter the key based on signature's keyid. Signed-off-by: Pradyumna Krishna <[email protected]>

Changes related to DSSE envelope

3129f96

* Added missing types in Envelope constructor. * Remove property tag from pre-auth-encoding. * Modify doc string of DSSE Envelope. Signed-off-by: Pradyumna Krishna <[email protected]>

PradyumnaKrishna force-pushed the sign_and_verify branch from 68e76ab to 56d8102 Compare July 21, 2022 08:46

PradyumnaKrishna requested a review from lukpueh July 21, 2022 10:57

lukpueh requested changes Jul 22, 2022

View reviewed changes

PradyumnaKrishna added 3 commits July 22, 2022 17:21

Add tests for sign and verify methods of Envelope

5a06521

Tests to ensure integrity of creation and verification of DSSE signatures. Signed-off-by: Pradyumna Krishna <[email protected]>

Add testcase to sign and verify DSSE GPGSignatures

a290b94

Tests creation and verification of different types of GPGSignatures for a DSSE envelope. Signed-off-by: Pradyumna Krishna <[email protected]>

PradyumnaKrishna force-pushed the sign_and_verify branch from 8fa108a to 517539d Compare July 22, 2022 12:16

lukpueh reviewed Aug 1, 2022

View reviewed changes

PradyumnaKrishna mentioned this pull request Aug 2, 2022

Exception handling on Key.verify for incompatible type of signature #8

Closed

PradyumnaKrishna force-pushed the sign_and_verify branch 2 times, most recently from 62f488e to b39e58d Compare August 2, 2022 03:32

lukpueh requested changes Aug 2, 2022

View reviewed changes

PradyumnaKrishna force-pushed the sign_and_verify branch from b39e58d to a290b94 Compare August 2, 2022 08:52

Fix grammatical mistake in doc-string

d7ec553

Signed-off-by: Pradyumna Krishna <[email protected]>

lukpueh approved these changes Aug 2, 2022

View reviewed changes

lukpueh merged commit 3bb6b67 into in-toto:master Aug 2, 2022

-        self.signatures.append(signature)
+        if signature.keyid:
+            for s in self.signatures:
+                if s.keyid == signature.keyid:
+                    self.signatures.remove(s)
+                    break
+        self.signatures.append(signature)

	# If Signature keyid doesn't matches with Key, skip.
	# If Signature keyid doesn't match with Key, skip.

	SignatureVerificationError: If the amount of "recognized_signers" is less
	SignatureVerificationError: If the amount of "accepted_keys" is less

		keys: A list of "Key" class instance that is a public key to verify
		the signatures.

	keys: A list of "Key" class instance that is a public key to verify
	the signatures.
	keys: A list of public keys to verify the signatures.

		in order to consider them for verification, is not a DSSE spec
		complaint (Issue #416).

Sign and verify #2

Sign and verify #2

Conversation

PradyumnaKrishna commented Jun 22, 2022 • edited Loading

Description of the changes being introduced by the pull request:

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

PradyumnaKrishna left a comment

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

PradyumnaKrishna left a comment

Choose a reason for hiding this comment

MVrachev left a comment

Choose a reason for hiding this comment

PradyumnaKrishna left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

MVrachev left a comment • edited Loading

Choose a reason for hiding this comment

PradyumnaKrishna commented Jun 29, 2022

MVrachev left a comment

Choose a reason for hiding this comment

adityasaky commented Jun 30, 2022

adityasaky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Jul 4, 2022

lukpueh 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

PradyumnaKrishna Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh 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

PradyumnaKrishna commented Jul 20, 2022

lukpueh commented Jul 20, 2022

PradyumnaKrishna commented Jul 20, 2022

lukpueh commented Jul 20, 2022

PradyumnaKrishna commented Jul 20, 2022 • edited Loading

lukpueh commented Jul 20, 2022

PradyumnaKrishna commented Jul 20, 2022

lukpueh commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh 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

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

lukpueh commented Aug 2, 2022

PradyumnaKrishna commented Jun 22, 2022 •

edited

Loading

PradyumnaKrishna left a comment •

edited

Loading

MVrachev Jun 28, 2022 •

edited

Loading

MVrachev left a comment •

edited

Loading

PradyumnaKrishna Jul 5, 2022 •

edited

Loading

PradyumnaKrishna commented Jul 20, 2022 •

edited

Loading