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

Change data structure for signatures to use key IDs as dictionary keys #155

Open
davidstrauss opened this issue Apr 15, 2021 · 4 comments
Open

Comments

@davidstrauss
Copy link

davidstrauss commented Apr 15, 2021

Because signatures (at least in the preferred ed25519 mode) are deterministic products of the key and payload materials, it doesn't seem like there's a use case for more than one signature from the same key ID.

Meanwhile -- and more concerning -- we've now had two implementations that have mistakenly used vulnerable signature threshold checking that could have been avoided by using a more robust data format that intrinsically prevents this attack.

I'm not sure how to make this change in a way that's both clean and backwards-compatible, but I'd even be satisfied getting this change into the hopper for TUF 2.0, even if that's a ways off.

Old:

 "signatures": [
  {
   "keyid": "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3",
   "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
           f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
  }...

New:

 "signatures": [
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3":
  {
   "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
           f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
  }...

I'm assuming there may be future signatures that aren't contained in a single value and therefore can't simply have this structure:

 "signatures": {
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3": 
    "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
     f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
   ...
@mnm678
Copy link
Collaborator

mnm678 commented Apr 15, 2021

This is a good idea, it's always nice to make implementation mistakes harder. It might also be worth cross-posting to the sigining-spec project, which is exploring some options for the signature wrapper more generally (cc @adityasaky)

Unfortunately, as you mention it would break backwards compatibility. There are a few TAPs in draft stage right now that would also require a major version change, so maybe we can prioritize some of them for a 2.0.0 release? At the very least I think this warrants some discussion (and maybe a community meeting).

@adityasaky
Copy link

adityasaky commented Apr 16, 2021

Thanks Marina!

Here's how the signing-spec currently defines key IDs.

https://github.com/secure-systems-lab/signing-spec/blob/master/protocol.md#signature-definition

KEYID: Optional, unauthenticated hint indicating what key and algorithm was used to sign the message. As with Sign(), details are agreed upon out-of-band by the signer and verifier. It MUST NOT be used for security decisions; it may only be used to narrow the selection of possible keys to try.

As I understand it, it puts the onus on implementations to handle this correctly. I may be mistaken though.

I'm assuming there may be future signatures that aren't contained in a single value and therefore can't simply have this structure

GPG signatures from securesystemslib currently include an other_headers field.

At the very least I think this warrants some discussion (and maybe a community meeting).

Is there one scheduled? Could we also get some thoughts on the current version of signing-spec from folks there? 😄

@jku
Copy link
Member

jku commented Jun 2, 2021

I agree with this (as you can see in python-tuf I'd like to internally handle signatures as a dict for same reasons).

New:

 "signatures": [
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3":
  {
   "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaed
           f4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
  }...

Just checking: I believe you mean for the outermost object to be a dictionary:

New:

"signatures": {
    "1bf1c6e3cdd3d3a8420b19199e27511999850f4b376c4547b2f32fba7e80fca3": {
        "sig": "90d2a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaedf4df84891d5aa37ace3ae3736a698e082e12c300dfe5aee92ea33a8f461f02"
    }, ...
}

@davidstrauss
Copy link
Author

Just checking: I believe you mean for the outermost object to be a dictionary

Correct. I may have made an error in how I edited the structure by hand.

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

No branches or pull requests

4 participants