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

Revocation check control for timestamp verification #303

Open
Two-Hearts opened this issue Jun 20, 2024 · 13 comments
Open

Revocation check control for timestamp verification #303

Two-Hearts opened this issue Jun 20, 2024 · 13 comments
Milestone

Comments

@Two-Hearts
Copy link
Contributor

          > **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 #290 (comment)

@Two-Hearts
Copy link
Contributor Author

Two-Hearts commented Jun 20, 2024

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
Copy link
Contributor

@gokarnm Do you have any better idea?

@Two-Hearts
Copy link
Contributor Author

Two-Hearts commented Jun 27, 2024

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
Copy link
Contributor

gokarnm commented Jul 1, 2024

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
Copy link
Contributor Author

Two-Hearts commented Jul 2, 2024

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

@shizhMSFT @toddysm @priteshbandi @FeynmanZhou

@toddysm
Copy link
Contributor

toddysm commented Jul 3, 2024

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
Copy link
Contributor Author

Two-Hearts commented Jul 3, 2024

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 #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
Copy link
Member

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

@yizha1
Copy link
Contributor

yizha1 commented Jul 8, 2024

@gokarnm I would suggest we go for the original proposal by Patrick #303 (comment) (as implemented in PR #305). The reasons are:

  • The new timestampRevocation is just one parameter for timestamping, there are others such as: expiry, authenticity, integrity. It is a bit weird that only revocation surfaced to the first level and keep other properties under authenticTimestamp.
  • The new timestampRevocation is not independent, which is not the case for other signature validations (Authenticity, Integrity and so on). for example, in certain case, the value depends on authenticTimestamp as Patrick described at comment Revocation check control for timestamp verification #303 (comment)
  • We discussed previously at the time for introducing field verifyTimestamp. We can consider trust policy 2.0 for major changes and keep 1.0 as simple and backward capability, since it requires more time to bake a good trust policy 2.0.

Hope it helps.

@gokarnm
Copy link
Contributor

gokarnm commented Jul 8, 2024

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 #303 (comment) 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
Copy link
Contributor

gokarnm commented Jul 8, 2024

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

Yes that is correct, something like

"signatureVerification": {
              "level" : "strict",
              "override": {
                "timestampRevocationCheck": "skip" 
              },
            }

@toddysm
Copy link
Contributor

toddysm commented Jul 9, 2024

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?

  • integrity validation cannot be overriden, and therefore cannot be specified as a key.
  • authenticity - Supported values are enforce and log.
  • authenticTimestamp - Supported values are enforce and log.
  • expiry - Supported values are enforce and log.
  • revocation - Supported values are enforce, log, and skip.
  • timestamp revocation - Supported values are enforce, log, and skip.

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.

@yizha1 yizha1 modified the milestones: 1.1.0, Future Jul 9, 2024
@Two-Hearts
Copy link
Contributor Author

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

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

Successfully merging a pull request may close this issue.

6 participants