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

🧹 Improving the keyVault resources to cover automatic key rotation policy #4624

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

HRouhani
Copy link
Contributor

@HRouhani HRouhani commented Sep 2, 2024

  • Ensure Automatic Key Rotation is Enabled Within Azure Key Vault for the Supported Services

Screenshot from 2024-09-02 15-57-47

Copy link
Contributor

github-actions bot commented Sep 2, 2024

Test Results

3 098 tests  ±0   3 097 ✅ ±0   1m 20s ⏱️ ±0s
  370 suites ±0       1 💤 ±0 
   28 files   ±0       0 ❌ ±0 

Results for commit 1ae7e0d. ± Comparison against base commit 342ea94.

♻️ This comment has been updated with latest results.

@@ -1538,6 +1538,8 @@ private azure.subscription.keyVaultService.key @defaults("kid keyName") {
version() string
// List of key versions
versions() []azure.subscription.keyVaultService.key
// Auto-rotation enabled status
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make this an optional field and only call the api if needed? (add the parens here and move the call to a function)

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do keep it like it is, please change the description to:

Suggested change
// Auto-rotation enabled status
// Whether auto-rotation is enabled

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vjeffrey I did as you requested, however I still could not solve the uniqueness of the keys:

Screenshot from 2024-09-04 14-30-09

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, is there maybe an id function missing? i haven't re-checked the code, i'll look when i get online in a while, but usually that's an id thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks VJ, I already added id function as here:

func (a *mqlAzureSubscriptionKeyVaultServiceKeyAutorotation) keyName() (string, error) {
	id := a.Kid.Data
	kvid, err := parseKeyVaultId(id)
	if err != nil {
		return "", err
	}

	return kvid.Name, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

the function should be named id() for this to work. alternatively you can just pass in the id directly in the args as __id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@preslavgerchev awesome, Thanks. It works now. Appreciate you help.

@HRouhani HRouhani force-pushed the hossein/azure-keyvault-keyrotation branch from 53e91ea to 52d69ab Compare September 4, 2024 12:28
return "", err
}

return kvid.Name, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

is the name guaranteed unique? if not, could just use the id itself here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is always unique!

@@ -1510,6 +1510,16 @@ private azure.subscription.keyVaultService.vault @defaults("vaultName type vault
secrets() []azure.subscription.keyVaultService.secret
// Vault diagnostic settings
diagnosticSettings() []azure.subscription.monitorService.diagnosticsetting
// Auto-rotation enabled status for all keys
autorotation() []azure.subscription.keyVaultService.key.autorotation
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be under the key resource and not the vault resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy you raised that, it was our plan, but in this way we get following error:

# go.mondoo.com/cnquery/v11/providers/azure/resources
resources/azure.lr.go:14246:12: c.autorotation undefined (type *mqlAzureSubscriptionKeyVaultServiceKey has no field or method autorotation, but does have field Autorotation)
make[1]: *** [Makefile:339: providers/build/azure] Error 1

if we do:

// Azure Key Vault key
private azure.subscription.keyVaultService.key @defaults("kid keyName") {
  // Key ID
  kid string
  // Key tags
  tags map[string]string
  // Whether the key is managed
  managed bool
  // Whether the key is enabled
  enabled bool
  // Date the key begins to be usable
  notBefore time
  // Date the key expires
  expires time
  // Key creation time
  created time
  // Key last update time
  updated time
  // Key recovery level
  recoveryLevel string
  // Key name
  keyName() string
  // Key version
  version() string
  // List of key versions
  versions() []azure.subscription.keyVaultService.key
  // Auto-rotation enabled status for all keys
  autorotation() []azure.subscription.keyVaultService.key.autorotation
}
``

…licy

Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
@HRouhani HRouhani force-pushed the hossein/azure-keyvault-keyrotation branch from 52d69ab to 1ae7e0d Compare September 9, 2024 14:11
Signed-off-by: Hossein Rouhani <h_rouhani@hotmail.com>
@HRouhani HRouhani merged commit 6faf374 into main Sep 9, 2024
7 checks passed
@HRouhani HRouhani deleted the hossein/azure-keyvault-keyrotation branch September 9, 2024 14:28
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants