-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add Key rotation policy. #194
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have a new end to end testing framework. This is perfect for this. Have a look to e.g.
test_keyReleasePolicy.py
You could change policies and test the key expiry case
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
@@ -39,6 +39,9 @@ export class KeyGeneration { | |||
|
|||
// We will get an untrusted timestamp from the host. Is this a threat? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is useful to log the timestamp in the ledger even if we are getting from the host. It allows auditors to detect misbehavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timestamp and expiry are in the KeyItem stored on the ledger
src/policies/KeyRotationPolicy.ts
Outdated
*/ | ||
public static defaultKeyRotationPolicy(): IKeyRotationPolicy { | ||
return { | ||
rotation_interval_seconds: 300, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very low for default. One day with a grace period of a few hours e.g., 6 hours sounds more reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is currently only used in tests. If the consortium does not apply the policy, no key rotation policy will be active as at this moment.
If we want to make this more as a policy example, shouldn't we use something like six months. We should of course use a lower value for testing first.
@@ -13,6 +13,7 @@ enableEndpoint(); | |||
// Define the interface for storing keys | |||
export interface IKeyItem extends JsonWebKeyEdDSAPublic { | |||
timestamp?: number; | |||
expiry?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still calculate it on the fly. Expiry can be used by clients to remove the key from their cache and go back to kms.
It is also very useful for testing the expiry feature in our E2E tests.
/** | ||
* | ||
* @param keyRotationPolicyMap - The map containing the key rotation policy. | ||
* @param keyItem - The key item to check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyItem is not a parameter
Key Rotation Policy
Definitions
New Properties
expiry
. This is an indication for clients and can be used to:Enforcement
unwrapKey
.unwrapKey
.unwrapKey
where is needs to be done anyway.TODO