notaryproject / specifications

Cross tooling and interoperability specifications
https://notaryproject.dev/
Apache License 2.0
155 stars 44 forks source link

Revocation check control for timestamp verification #303

Open Two-Hearts opened 2 months ago

Two-Hearts commented 2 months ago
          > **Revocation check** : Guarantees that the TSA identity is still trusted at verification time. Events such as key or system compromise can cause a previously trusted TSA identity to become untrusted. It is always enforced when timestamp countersignature verification is triggered.

Shouldn't this be governed by revocation configuration in trust policy ?

_Originally posted by @priteshbandi in https://github.com/notaryproject/specifications/pull/290#discussion_r1646655747_

Two-Hearts commented 2 months ago

As suggested by @priteshbandi, it is desired that the revocation check in timestamp verification be configurable by the user. Because a user may not want any network roundtrip during verification. I think we have a couple of options:

  1. Like signature verification, we could introduce Override and Level to timestamp verification as well. But since timestamp is already controlled by AuthenticTimestamp overall, this may make our trust policy really complicated and extremely hard to understand.

  2. An alternative might be adding a new field under signatureVerification (similar to field verifyTimestamp), we can call it skipTimestampRevocationCheck and default to false. When set to true, timestamp verification would not include tsa cert chain revocation check. For example,

    {
    "version": "1.0",
    "trustPolicies": [
        {
            "name": "wabbit-networks-images",
            "registryScopes": [ "*" ],
            "signatureVerification": {
              "level" : "strict",
              "verifyTimestamp": "afterCertExpiry"
              "skipTimestampRevocationCheck": true
            },
            "trustStores": ["ca:acme-rockets", "tsa:trusted-tsa"],
            "trustedIdentities": [
              "x509.subject: C=US, ST=WA, L=Seattle, O=acme-rockets.io, OU=Finance, CN=SecureBuilder"
            ]
        }
    ]
    }

(The similar may apply to authenticity, but it's a different situation, where user indeed must configure their tsa trust store during verification)

/cc: @yizha1 as also involved in the discussion

priteshbandi commented 2 months ago

@gokarnm Do you have any better idea?

Two-Hearts commented 2 months ago

Based on discussion in the 6/24/24 community meeting, option 2, adding an extra field skipTimestampRevocationCheck to trust policy is better than adding a new column to the validation action table. We had the same discussion when adding the verifyTimestamp field to trust policy. Therefore, I'm going to pick option 2 for now, and we can update in future iterations if better idea comes up. @shizhMSFT @yizha1 @priteshbandi @gokarnm

gokarnm commented 2 months ago

Let's surface "Timestamp Revocation check" as a new top level validation similar to "Revocation check", and they can be overridden using the "timestampRevocation" with values as "enforce", "log", "skip".

Two-Hearts commented 2 months ago

Based on 7/1/24 community meeting discussion, created two PRs to compare the possible solutions: https://github.com/notaryproject/specifications/pull/305 -- adding a new skipTimestampRevocationCheck sub-field https://github.com/notaryproject/specifications/pull/306 -- adding a new Timestamp revocation check column

@shizhMSFT @toddysm @priteshbandi @FeynmanZhou

toddysm commented 2 months ago

Can we have an example for the first proposal also posted? It is hard to imagine how the trust policy will look for #305 if there is no example in this issue or in the PR.

Two-Hearts commented 2 months ago

Can we have an example for the first proposal also posted? It is hard to imagine how the trust policy will look for #305 if there is no example in this issue or in the PR.

@toddysm Do you mean an example for https://github.com/notaryproject/specifications/pull/306? The example for 305 is shown above:

{
    "version": "1.0",
    "trustPolicies": [
        {
            "name": "wabbit-networks-images",
            "registryScopes": [ "*" ],
            "signatureVerification": {
              "level" : "strict",
              "verifyTimestamp": "afterCertExpiry"
              "skipTimestampRevocationCheck": true
            },
            "trustStores": ["ca:acme-rockets", "tsa:trusted-tsa"],
            "trustedIdentities": [
              "x509.subject: C=US, ST=WA, L=Seattle, O=acme-rockets.io, OU=Finance, CN=SecureBuilder"
            ]
        }
    ]
}

An example for 306 would be:

{
    "version": "1.0",
    "trustPolicies": [
        {
            "name": "wabbit-networks-images",
            "registryScopes": [ "*" ],
            "signatureVerification": {
              "level" : "strict",
              "override": {
                "timestampRevocationCheck": "skip"  // Skip timestamp revocation check during timestamp verification
              },
              "verifyTimestamp": "afterCertExpiry"  // Only verify any timestamp countersignature when codesigning certificate chain has expired
            },
            "trustStores": ["ca:acme-rockets", "tsa:trusted-tsa"], // The trust store type `tsa` MUST be configured to enable timestamp verification.
            "trustedIdentities": [
              "x509.subject: C=US, ST=WA, L=Seattle, O=acme-rockets.io, OU=Finance, CN=SecureBuilder"
            ]
        }
    ]
}

Based on 7/1/24 community meeting, the following is an invalid trust policy example of 306, which is a breaking change to trust policy version 1.0. Users already having the following trust policy would find verification failed by upgrading Notation:

{
    "version": "1.0",
    "trustPolicies": [
        {
            "name": "wabbit-networks-images",
            "registryScopes": [ "*" ],
            "signatureVerification": {
              "level" : "strict",
              "override": {
                "authenticTimestamp": "log"  // This actually invalidates the trust policy, because the default value of `timestampRevocationCheck` is `enforced`.
              },
            },
            "trustStores": ["ca:acme-rockets"]
            "trustedIdentities": [
              "x509.subject: C=US, ST=WA, L=Seattle, O=acme-rockets.io, OU=Finance, CN=SecureBuilder"
            ]
        }
    ]
}
FeynmanZhou commented 2 months ago

@gokarnm Are you expecting a new property "override" under "signatureVerification"?

yizha1 commented 2 months ago

@gokarnm I would suggest we go for the original proposal by Patrick https://github.com/notaryproject/specifications/issues/303#issuecomment-2192882551 (as implemented in PR https://github.com/notaryproject/specifications/pull/305). The reasons are:

Hope it helps.

gokarnm commented 2 months ago

Timestamp Revocation Check is related to Authentic Timestamp but can be available as a top level validation. This is similar to how Authenticity and RevocationCheck are related but two distinct validations. The goal of signature verification level is to provide preset values that dictates all other behavior. This simplifies the customer experience with reasonable presets for most cases, and users can override them for special cases. I'm not in favor of introducing newer keys under signatureVerification similar to verifyTimestamp, unless there is a strong case.

The comment about invalid policy https://github.com/notaryproject/specifications/issues/303#issuecomment-2205098156 has merit, the same is applicable to authenticity and revocation as well in the following policy, so we should address this edge case in both these situations. One option is, the same override level would apply to the related revocation check as well.

"signatureVerification": {
   "level" : "strict",
   "override": {
        "authenticity": "log"  // Should this imply that `revocation` is `log` as well?
     },
 }
gokarnm commented 2 months ago

@gokarnm Are you expecting a new property "override" under "signatureVerification"?

Yes that is correct, something like

"signatureVerification": {
              "level" : "strict",
              "override": {
                "timestampRevocationCheck": "skip" 
              },
            }
toddysm commented 2 months ago

Can we have all the possible combinations of the following fields for override map (from https://github.com/notaryproject/specifications/blob/14209c5a2c88f34701a885176001513877df2070/specs/trust-store-trust-policy.md#timestamp-countersignature-verification-details) in combination with each supported level and describe what the behavior is?

My concern is that this will result in a long list of permitations and some of the combinations will be incompatible. It will be easy for user to create an incompatible combination and hard to troubleshoot (which will inherently reduce the security).

I would suggest that we start with the use-cases (descfribe them well) and then try to figure out what would be the appropriate fields (columns) and values for them.

Two-Hearts commented 2 months ago

Based on 7/8/24 community meeting discussion, we've moved this issue to Future milestone.