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

docs: nVersionCount support for KMP design doc #1831

Merged
merged 7 commits into from
Oct 22, 2024
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions docs/design/kmp-nversions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# nVersionCount support for Key Management Provider

Author: Josh Duffney (@duffney)

Tracked issues in scope:

- https://github.com/ratify-project/ratify/issues/1751

Proposal ref:

- https://github.com/ratify-project/ratify/blob/dev/docs/proposals/Automated-Certificate-and-Key-Updates.md

## Problem Statement

In version 1.3.0 and earlier, Ratify does not support the nVersionCount parameter for Key Management Provider (KMP) resources. This means that when a certificate or key is rotated, Ratify updates the cache with the new version and removes the previous one, which may not suit all use cases.

For instance, if a user needs to retain the last three versions of a certificate or key in the cache, Ratify cannot meet this requirement without manually adjusting the KMP resource for each new version.

By supporting nVersionCount, Ratify would allow users to specify how many versions of a certificate or key should be kept in the cache, eliminating the need for manual updates to the KMP resource.

## Proposed Solution

To address this challenge, this proposal suggests adding support for the `maxVersionCount` parameter to the KMP resource in Ratify. This parameter will allow users to specify the number of versions of a certificate or key that should be retained in the cache.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion to include a CR example with maxVersionCount parameter


When a new version of a certificate or key is created, Ratify will check the `maxVersionCount` parameter to determine how many versions should be retained in the cache. If the number of versions exceeds the specified count, Ratify will remove the oldest version from the cache.
susanshi marked this conversation as resolved.
Show resolved Hide resolved
susanshi marked this conversation as resolved.
Show resolved Hide resolved

If a version is disabled, Ratify will remove it from the cache. This ensures that disabled versions are not retained in the cache, reducing the risk of using compromised keys or certificates being passed to the verifiers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion to add example of the output of "describe" command. Customer should be able to see the versions fetched. If a version is disabled, I m thinking we could:

  • log a warning that cert was skipped due to cert disabled state
  • agreed we should not cache disabled version


Example: AKV KMP resource with `maxVersionCount` parameter

```yaml
apiVersion: config.ratify.deislabs.io/v1beta1
kind: KeyManagementProvider
metadata:
name: keymanagementprovider-akv
spec:
type: azurekeyvault
refreshInterval: 1m
parameters:
vaultURI: https://yourkeyvault.vault.azure.net/
certificates:
- name: yourCertName
maxVersionCount: 3
tenantID:
clientID:
```

Example: AKV KMP resource status with multiple versions retained in the cache

```yaml
Status:
Issuccess: true
Lastfetchedtime: 2024-10-02T14:58:54Z
Properties:
Certificates:
Last Refreshed: 2024-10-02T14:58:54Z
Name: yourCertName
Version: a1b2c3d4e5f67890abcdef1234567890
Status: Enabled
Last Refreshed: 2024-10-02T14:58:54Z
Name: yourCertName
Version: 0ff373a9259c4578a247cfd7861a8805
Status: Disabled
```

## Implementation Details

- Modify the KMP data structure to include the status of the version.
```go
type KMPMapKey struct {
Name string
Version string
Status string // Enabled or Disabled
susanshi marked this conversation as resolved.
Show resolved Hide resolved
Created time.Time // Time the version was created used for determining the oldest version
}
```
- Add the `maxVersionCount` parameter to the KMP resource in Ratify.
- ensure the value cannot be less than 1 or a negative number
akashsinghal marked this conversation as resolved.
Show resolved Hide resolved
- default to 0 if not specified
binbin-li marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

The parameter name maxVersionCount seems not intuitive. For example, the default value 0 means max version count is 0 , however, we will cache one version, the latest one, right? If the value is 1, we cached two versions. Maybe using maxPreviousVersionCount or previousVersionCount is better? Additonally, using max is also a bit confusing for me, for example, if the value is 2, does that mean at maximum, two versions are cached, but caching one version is still OK?

Since it means the cached versions, how about using the parameter name cachedVersionCount or cachedPreviousVersionCount?

Copy link
Collaborator

Choose a reason for hiding this comment

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

discussed during community meeting. we want to avoid 'cache' as this is implementation detail. We are proposing , "syncVersionCount", and given @duffney 's research that 3 is a common default. We propose to stay with 3 as the default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not a strong opinion, just posting a var name recommended by Chatgpt, versionsFetchLimit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

secret store uses objectVersionHistory, so perhaps a good name for us would be certificateVersionHistory? or simply versionHistory?

Copy link
Collaborator

Choose a reason for hiding this comment

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

discussed during community meeting, we would go with "versionHistory"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Vote for versionHistory

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for versionHistory

- maximum value should be (TBD)
- specify the value at the object level within the parameters of the KMP resource.
- Changes to `azurekeyvault` provider:
- support for the `maxVersionCount` parameter.
- allowing retrieval of multiple versions of certificates or keys.
- remove the oldest version from the cache when the number of versions exceeds the `maxVersionCount` parameter.
- update disabled certs status in the cache & remove the certData from the cache.
- Log when the status of a version changes.
- Log when a conflict between the `maxVersionCount` and the number of specified certificate versions occurs.

## Dev Work Items

## Open Questions

- If a version is disabled, should it be removed from the cache or retained based on the nVersionCount and marked as inactive\disabled?
- [x] Keep the disabled version in the cache and mark it as disabled.
- If a version is disabled, does that count towards the nVersionCount? For example, if nVersionCount is set to 3 and one of the versions is disabled, should Ratify retain the last three active versions or the last three versions, regardless of their status?
- [x] Yes, disabled versions should count towards the nVersionCount. The reason for this is that disabled versions may be re-enabled in the future, and it is important to retain them in the cache.
- Should the existing KMP data structure be changed to group versions by key or certificate name?
- [x] No, a flat list of versions is sufficient. At this time, there is no need to group versions by key or certificate name because the verifiers do not need to know the history of the versions.
- Should the KMP status return a flat list of versions?
- [x] Yes, the status should return a flat list of versions.
- What should the maximum value for nVersionCount be?
- [ ] TBD
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can start with 2, which means we will cache the latest version and one previous version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does break away from the pattern secret store driver has, but I'm not opposed.

It does bring up an interesting point, what exactly is the int counting for the value of the history? Is it the total number of the versions stored in the KMP or latest +n? for example the secret store, always stores latest and then if you specify to store one more than latest you would pass 1 to the objectVersionHistory option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

discussed during community meeting, versionHistory defaults to 2 ( keeping 3 version)

Copy link
Collaborator

@yizha1 yizha1 Oct 17, 2024

Choose a reason for hiding this comment

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

discussed during community meeting, versionHistory defaults to 2 ( keeping 3 version)

LGTM. One current version, 2 history versions



## Future Considerations
Loading