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

doc: create notation threat model #242

Merged
merged 21 commits into from
Jun 29, 2023

Conversation

yizha1
Copy link
Contributor

@yizha1 yizha1 commented Feb 21, 2023

Updates:

  • Create notation threat model

Signed-off-by: Yi Zha [email protected]

@yizha1 yizha1 changed the title doc: add notation threat model doc: create notation threat model Feb 21, 2023
@toddysm
Copy link
Contributor

toddysm commented Feb 22, 2023

Looking at the threat model, I believe we need to update it with the following information:

  • How is the artifact to be signed produced?
    For that we can use illustrate few examples: container image, SBOM, or another artifact but those two will illustrate the scenarios.
  • How is the descriptor to be signed obtained?
    This one is important to cover because the descriptor is fetched remotely.
  • On the validation side, how is the cert pulled?
    In general I think we need to be more detailed on the different "stores": cert store, policy store, config store, plugin store. They all may be on the file system but each one may have different access control settings.
  • Adding some details about the external stores (KMS, Registry) would be good
  • Do we need to create a thread model for signing with local keys? This is still a scenario although we can document that is not recommended.

@toddysm
Copy link
Contributor

toddysm commented Feb 24, 2023

Some more thoughts about the threat model...

We should split the File System into separate storages: Cert Store, Configuration Store, Policy Store, and Plugin Store. Each one of those can be exploited separately depending on the level of File System access of the user (because those are in different directories).

@yizha1
Copy link
Contributor Author

yizha1 commented Feb 25, 2023

Some more thoughts about the threat model...

We should split the File System into separate storages: Cert Store, Configuration Store, Policy Store, and Plugin Store. Each one of those can be exploited separately depending on the level of File System access of the user (because those are in different directories).

I will update it to make it clear. There are different directories, but they are owned by the same user. We don't support system level configuration yet.

@yizha1
Copy link
Contributor Author

yizha1 commented Feb 26, 2023

@toddysm PTAL the latest updates. I haven't split the file system yet and will do it later.

Copy link

@byronchien byronchien left a comment

Choose a reason for hiding this comment

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

  • media/notation-verify-local.png is missing a Step 2?
  • should there also be a remote verify example with a plugin?

@yizha1
Copy link
Contributor Author

yizha1 commented Mar 2, 2023

  • media/notation-verify-local.png is missing a Step 2?
    I will fix it.
  • should there also be a remote verify example with a plugin?
    I will try to add an example for it, but I didn't try any verify plugin yet, do you have any document that I can refer to?

@byronchien
Copy link

  • should there also be a remote verify example with a plugin?
    I will try to add an example for it, but I didn't try any verify plugin yet, do you have any document that I can refer to?

the example plugin here has a verify-signature option, but don't have a different plugin to point you to

@yizha1
Copy link
Contributor Author

yizha1 commented Mar 4, 2023

@priteshbandi @byronchien @JeyJeyGao Please check whether the flow diagram of "verifying remote artifacts using plugin" is correct. I am not sure whether plugin should read trust store instead of notation.

@JeyJeyGao
Copy link
Contributor

@priteshbandi @byronchien @JeyJeyGao Please check whether the flow diagram of "verifying remote artifacts using plugin" is correct. I am not sure whether plugin should read trust store instead of notation.

In fact, notation verification plugin can verifiy both local and remote artifacts. Also notation plugin doesn't need to read notation trust store, but there is no limitation that plugin is forbidden to read it, that is to say, whether to read the notation trust store depends on the plugin implementation.

Copy link

@byronchien byronchien left a comment

Choose a reason for hiding this comment

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

In media/notation-sign-local.png there are two step 6s ("Read certificate bundle file" and "Return signature and certificates").

i'm also not 100% clear on what happens in the "Read certificate bundle file" step. if the plugin is retrieving keys/certs for signing, should the source be specified as certificate/key storage rather than just file system?

@yizha1
Copy link
Contributor Author

yizha1 commented Apr 24, 2023

In media/notation-sign-local.png there are two step 6s ("Read certificate bundle file" and "Return signature and certificates").

i'm also not 100% clear on what happens in the "Read certificate bundle file" step. if the plugin is retrieving keys/certs for signing, should the source be specified as certificate/key storage rather than just file system?

I will fix the duplicated number 6. Currently signing certificate is stored in KMS, but the root/intermediate CA may not be able to fetch from KMS, since root/intermediate CA is from CA vendor. Currently it can be fetched via files stored in filesystem. That is the purpose of "Read certificate bundle file. " Do you have any concrete examples on how the CA certificates are handled?

yizha1 added 4 commits June 12, 2023 16:55
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
yizha1 added 2 commits June 13, 2023 17:36
@yizha1
Copy link
Contributor Author

yizha1 commented Jun 13, 2023

@priteshbandi resolved all the comments and added new threats except the following one:

I still didn't get the point, since it is expected behavior as the normal expire/revoked, right?

Compromised(expire or revoked) CA in certificate chain
Impact:

  • Expiry
    An expired cert in cert chain won’t lead to failure of expiry validation if signature is timestamped or is from a signingAuthority. Ref: trust-store-trust-policy.md#signature-verification(point 6)
    If signature is not timestamped and is not from signingAuthority, expired CA will result in failure of expiry validation.
  • Revocation
    Revocation of any CA in certificate chain will lead to failure of revocation validation. Ref: trust-store-trust-policy.md#signature-verification(point 7)
    Mitigation: Already mitigated(see impact)

@Two-Hearts
Copy link
Contributor

nit:
Since we are adding this file at threatmodels/notation-threatmodel.md, shall we deprecate/remove the file threatmodel.md?

threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
@yizha1 yizha1 requested review from shizhMSFT and byronchien June 19, 2023 12:09
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

Added some comments.

threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
threatmodels/notation-threatmodel.md Outdated Show resolved Hide resolved
Signed-off-by: Yi Zha <[email protected]>
@yizha1 yizha1 requested review from priteshbandi and shizhMSFT June 27, 2023 03:40
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@toddysm toddysm left a comment

Choose a reason for hiding this comment

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

LGTM

@priteshbandi priteshbandi merged commit 1366015 into notaryproject:main Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants