-
Notifications
You must be signed in to change notification settings - Fork 19
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
Proposed TAP #9 - Mandatory metadata signing schemes #21
Conversation
tap9.md
Outdated
{ | ||
"keyid": "588f02cf6907c3f199b201e845f5a458285e9ae956680e0e2fdcc7987306ef7c", | ||
"method": "rsassa-pss", | ||
"signature": "fce1a97b55076065c7dc7c78fb0206d5dd87db817ccc7bcf83c3cdf..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "signature" entry should be "sig"
See section 4.2 of the specification and example metadata.
} | ||
``` | ||
|
||
Multiple security researchers have pointed out that the `signatures` section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide a reference to one of these audits, if possible? It will be helpful to the
reader.
I know of one audit that is available online for review: https://github.com/docker/notary/blob/master/docs/resources/ncc_docker_notary_audit_2015_07_31.pdf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll ask tomorrow in the office. I'm not sure what of our report we can share.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't release the doc at the moment, but the relevant excerpt was placed into the TAP.
tap9.md
Outdated
|
||
# Motivation | ||
|
||
Moving the `methods` field into the signed portion of the metadata removes one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "methods" should be "method."
} | ||
``` | ||
|
||
When a signature is made, the `signature` object would still need to have a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the following example, assume that a developer has access to the same RSA key.
What if the "keys" entry was formatted as follows?
"keys": {
"0bde74244f238cd6d4e3839d35a5b248d6cde5759b760ebdbee45a386109a41a": {
"keytype": "rsa",
"scheme": "rsa-pkcs1-1.5",
"keyval": {
"public": "-----BEGIN PUBLIC KEY-----<etc>"
},
{
"349838deff9a9a9...": {
"keytype": "rsa",
"scheme": "rsassa-pss-sha256",
"keyval": {
"public": "-----BEGIN PUBLIC KEY-----<etc>"
},
},
...
And the "signatures" entry could list multiple signing keys like so:
{
"signatures": [
{
"keyid": "349838deff9a9a9...",
"sig": "fce1a97b55076065c7dc7c78fb0206d5dd87db817ccc7bcf83c3cdf..."
},
{
"keyid": "0bde74244f238cd6d4e3839d35a5b248d6cde5759b760ebdbee45a386109a41a",
"sig": "5764debfaaaacde343..."
}
],
"signed": {
...
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I edited my previous comment to change the "sig" listed by the "0bde7..." key, which should differ from the one provided by the "3498..." key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed this in the latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stick with the original solution. The RSA keys themselves are likely to be bulk of the data inside a root.json so not duplicating them seems beneficial.
4f9646a
to
4eaed96
Compare
|
||
This itself would be insufficient because when checking a threshold, a client | ||
would have have to check that each key ID calculated in the above way points to | ||
exactly one real key. We would have a calculate a second key ID that is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I'm in favor of this view, but I suppose a key in this case would be the KEYID entries in "keys." In other words, there are two unique keys in the example above: one rsa key + 2 different signing schemes.
However, I admit that this can potentially be a source of confusion to users, who are likely to view a " real key" by its keytype, and not keytype + signing scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would still need the check against the "real key ID" because the whole point of thresholds is that N entities sign the key, and allowing a single key to do multiple signatures means that an entity could provide all the signatures needed. For example:
Key with key ID sha256(cjson(encoded)) = 123
has been placed in the metadata for the targets role with signing schemes A
, B
, and C
. The new key IDs done via sha256(cjson(scheme || encoded))
would be 456
, abc
, and def
. If the targets role has threshold = 3
, then the single key 123
could sign it 3 times using "keys" 456
, abc
, and def
. To avoid this, we'd have to check that the sha256(cjson(endcoded))
of each signature is unique which seems like an overly complicated step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. This point was raised in issue #425 and I was under the impression that @JustinCappos views "keys" 456
, abc
, and def
as unique, even though they all use the same 123
key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to discussion:
theupdateframework/python-tuf#425 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support the removal of a "method" entry, but might need further tweaks or comment on how we specify keys and their signing methods. Merging TAP 9 now that it has a TAP number!
TAPs that have been assigned a TAP number will now be merged into the "master" branch. This does not mean that the merged TAP has been accepted, only that the proposed change (with potential tweaks) is likely to be incorporated into TUF. We'll likely continue to comment on the pull request after it's been closed, so that we can easily comment on specific lines of the TAP. Thanks for submitting TAP 9! |
On Fri, May 19, 2017 at 11:22 AM, Vladimir Diaz ***@***.***> wrote:
TAPs that have been assigned a TAP number will now be merged into the
"master" branch. This does not mean that the merged TAP has been accepted,
only that the proposed change (with potential tweaks) is likely to be
incorporated into TUF.
I would say instead that merging a TAP means it is worth having a permanent
/ fixed TAP number to point to this discussion. Moving from 'draft' to
'accepted' status is what means it is likely to be merged with tweaks.
|
@endophage: I'd love to have your thoughts on this TAP. |
@JustinCappos I'd like to think that every client matches the signing scheme against the key type regardless of where those pieces of data are located so the premise for this issue seems to be "in a poorly implemented client ..." which isn't the strongest of reasons. That said, I have no issues with the change and agree that it increases this aspect of security in those poorly implemented clients. |
Okay, does anyone else have a thought / concern? Any objections with moving this to accepted? |
Addresses issues covered in the conversation here: theupdateframework/python-tuf#425