Support clean up the source key and certificate generated by Notation

[FeynmanZhou]

What is the areas you would like to add the new feature to?

Notation CLI

Is your feature request related to a problem?

notation key delete can only remove the key from the signing key list and notation cert delete can only remove the self-signed certificate from the trust store. This is by design since Notation doesn't support signing with local keys and managing local keys.

Per discussion in #606 (comment) and another issue #604 , users want to delete the source key and certificate generated by notation cert generate-test in a convenient way.

What solution do you propose?

Providing a flag --cleanup to notation cert generate-test to allow users to delete the specified source key and certificate generated by notation cert generate-test. This flag is only used to delete the test key and self-signed certificate. The keys and certificates that are not generated by Notation will not be able to be deleted with this flag.

For example, delete a source key and cert generated by notation cert generate-test "wabbit-networks.io":

$ notation cert generate-test --cleanup wabbit-networks.io
Deleted <key_name> and <cert_name> 

What alternatives have you considered?

N/A

Any additional context?

No response

[patrickzheng200]

Will take this one. @yizha1 @FeynmanZhou Do we want it in 1.0 or after 1.0?

[patrickzheng200]

@FeynmanZhou By reading through your issue description, I have the following question:
One could run notation cert generate-test for multiple times and each time with a different key name. Should notation cert generate-test --cleanup delete all of those keys/certs? Or notation cert generate-test --cleanup <name> requires a name argument so that each time only one key/cert specified by the user is deleted?

[FeynmanZhou]

@patrickzheng200 It's in v1.0.0. This cleanup flag is used to delete the specified source key/cert. We need to add an argument <name> when using this flag. I will update the Notation CLI Spec with this new flag.

[patrickzheng200]

@patrickzheng200 This cleanup flag is used to delete the specified source key/cert. We need to add an argument <name> when using this flag. I will update the Notation CLI Spec with this new flag.

I see. So, each time we run with flag --cleanup, we only delete 1 key/cert. Then my follow up question would be:
Since signingkeys.json is just a file, the user can manually add contents to it, for example:

{
    "default": "alpine2",
    "keys": [
        {
            "name": "alpine",                     <-------- a test key created by `notation cert generate-test`
            "keyPath": "notation/localkeys/alpine.key",
            "certPath": "notation/localkeys/alpine.crt"
        },
        {
            "name": "alpine2",                  <-------- a test key manually inserted into the file by the user
            "keyPath": "notation/localkeys/alpine2.key",
            "certPath": "notation/localkeys/alpine2.crt"
        }
    ]
}

Given the file above, what is our expected result after running notation cert generate-test --cleanup alpine2?

[FeynmanZhou]

IMO, adding key and cert to signingkey.json manually is not a regular manipulation although technically it is allowed. Given the case above, I think notation cert generate-test --cleanup alpine2 should be able to remove the key alpine2 from signingkeys.json and delete the source key and cert locally. Is it complicated to implement this alpine2 case in v1.0.0?

[patrickzheng200]

IMO, adding key and cert to signingkey.json manually is not a regular manipulation although technically it is allowed. Given the case above, I think notation cert generate-test --cleanup alpine2 should be able to remove the key alpine2 from signingkeys.json and delete the source key and cert locally. Is it complicated to implement this alpine2 case in v1.0.0?

Yeah, that makes sense. To confirm, the command notation cert generate-test --cleanup <name> is able to delete any key/cert as long as it's not an external key. The command would go to the paths (keyPath, certPath) defined in the signingkeys.json and delete them from the paths. Also, it would delete the corresponding certificate from the trust store.

[yizha1]

Just to confirm:

notation cert generate-test <name> will do

• create a local key file and certificate file
  • add entry in signingkeys.json
  • create files under {NOTATION_CONFIG}/localkeys
• add key to signing key list
• add cert to trust store

notation cert generate-test --cleanup <name> will clean up everything created in the above steps, right?

[FeynmanZhou]

@yizha1 Right.

[toddysm]

The following command notation cert generate-test --cleanup <name> does not make too much sense for me. The command is to "generate" something but we are adding switch to "cleanup". This is a very confusing concept. Either the command should be cleanup or we should add a switch to the key delete command for "cleanup"

[patrickzheng200]

The following command notation cert generate-test --cleanup <name> does not make too much sense for me. The command is to "generate" something but we are adding switch to "cleanup". This is a very confusing concept. Either the command should be cleanup or we should add a switch to the key delete command for "cleanup"

Thanks @toddysm, I agree with your concern here. How about this: notation cert cleanup-test <name>? I think it's reasonable to still keep it under notation cert command because the user used notation cert generate-test to create the testing elements. It might be more convenient for them to call another command under notation cert to clean it up. @FeynmanZhou what do you think?

[toddysm]

@patrickzheng200 your proposal makes more sense. Is the expectation that only "test" pairs are cleaned up? Although unlikely, there may be some odd usage where the key and the cert are both on the same machine (device - I can come up with a hypothetical scenario for IIoT :) ). Shouldn't we just have the command as cleanup and avoid the -test part?

[patrickzheng200]

@patrickzheng200 your proposal makes more sense. Is the expectation that only "test" pairs are cleaned up? Although unlikely, there may be some odd usage where the key and the cert are both on the same machine (device - I can come up with a hypothetical scenario for IIoT :) ). Shouldn't we just have the command as cleanup and avoid the -test part?

@toddysm Yeah, the original purpose was to only clean up those test pairs created by notation cert generate-test. However, as you mentioned, there may be those odd usages by users, and the cleanup-test command actually can clean up any pairs on the machine. So yes, we could remove the -test part. Then the command becomes notation cert cleanup <name>, which looks good to me.

[yizha1]

We still need to triage this issue, and understand what the solution will be.

[yizha1]

The function of cleanup is similar to delete command. The difference is that the testing key need to be deleted as well besides the testing certs. Since we may introduce signing using local key in the future, we maynot use notation cert delete to delete local key. So, to avoid confusion, I would suggest using notation cert cleanup-test.

[Two-Hearts]

The function of cleanup is similar to delete command. The difference is that the testing key need to be deleted as well besides the testing certs. Since we may introduce signing using local key in the future, we maynot use notation cert delete to delete local key. So, to avoid confusion, I would suggest using notation cert cleanup-test.

@toddysm I just completed OCI spec related works for rc.5, switching my focus to this issue again. I think @yizha1's concern is valid. If we call it notation cert cleanup, it might bring in ambiguity since we already have another command called notation cert delete. So perhaps notation cert cleanup-test is a better option here. And IMO, we should put this command behind experimental to avoid breaking changes. Because we may support local key signing in the future, then cleanup-test is subject to change. What do you think? @toddysm @yizha1 @FeynmanZhou

[yizha1]

@toddysm I just completed OCI spec related works for rc.5, switching my focus to this issue again. I think @yizha1's concern is valid. If we call it notation cert cleanup, it might bring in ambiguity since we already have another command called notation cert delete. So perhaps notation cert cleanup-test is a better option here. And IMO, we should put this command behind experimental to avoid breaking changes. Because we may support local key signing in the future, then cleanup-test is subject to change. What do you think? @toddysm @yizha1 @FeynmanZhou

If it is experimental flag, then it is not that useful for users.

[Two-Hearts]

@toddysm I just completed OCI spec related works for rc.5, switching my focus to this issue again. I think @yizha1's concern is valid. If we call it notation cert cleanup, it might bring in ambiguity since we already have another command called notation cert delete. So perhaps notation cert cleanup-test is a better option here. And IMO, we should put this command behind experimental to avoid breaking changes. Because we may support local key signing in the future, then cleanup-test is subject to change. What do you think? @toddysm @yizha1 @FeynmanZhou

If it is experimental flag, then it is not that useful for users.

@yizha1 I see. I'm okay with both options. Let's wait for others' suggestions.

[Two-Hearts]

Had some discussions with @shizhMSFT today:

1. We should move notation cert generate-test behind experimental, because it relies on self-signed cert, and hence, not designed for production use. (we should still keep the command as it's the base of our quick start.)
2. Given above, we have two options to clean up the key-cert pairs created by notation cert generate-test:
   a. The current proposed notation cert cleanup-test, behind experimental as well.
   b. Instead of introducing a new subcommand, create a new experimental flag under notation cert delete, for example, notation cert delete --test (flag name TBD)

Waiting for others' suggestions on this. /cc: @priteshbandi @toddysm @sajayantony @yizha1 @FeynmanZhou @JeyJeyGao

[FeynmanZhou]

notation cert generate-test is not recommended for production use so it's reasonable to mark it as an experimental feature.

IMO, adding a new sub-command notation cert cleanup-test <name> to clean up the source key and certificate generated by notation cert generate-test seems more natural for a quick start experience.

[yizha1]

No matter what solution will be, suggest updating the description for generate-test command:

generate-test Generate a test RSA key and a corresponding self-signed certificate. Use it only for testing purposes

So my proposal is

• update the description text of generate-test command as above suggestion.
• add a new sub-command notation cert cleanup-test <name> to clean up testing keys only. (If we cannot agree on this for v1 release, at least we should document it properly. Tracked by Add a shortcut note for cleaning up testing keys in quick start notaryproject.dev#221)
  • cleanup-test Clean up the test key and corresponding certificate. Use it only for testing purposes

[iamsamirzon]

I like the proposal for a new command "notation cert cleanup-test" and modifying the description test of generate-test command as @yizha1 said above. However, I don't think we need to move this under the "EXPERIMENTAL" feature flag, because we intend to support this option for people to test notation with the least amount of friction

[yizha1]

As discussed in the community meeting on 5/23/2023, this issue is not critical for v1 release, so let's scope it out and triage it after v1 release.