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 --secrets for VM and VMSS #2212
Conversation
short-summary: Get a default policy for a self-signed certificate | ||
long-summary: > | ||
This default policy can be used in conjunction with `az keyvault create` to create a self-signed certificate. | ||
The policy can also be used as a base to manipulate to create other permutations of Key Vault certificates. |
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 policy can also be used as a base to manipulate to create other
permutations of Key Vault certificates.
Does 'to manipulate to create' make sense?
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.
nope -- will change
@@ -176,6 +177,17 @@ def b64encode(s): | |||
return encoded.decode('latin-1') | |||
|
|||
|
|||
def b64_to_hex(s): |
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.
IMO we should avoid adding such util methods to core unless they will really be used by a lot of other modules.
As this PR is, we'd have to release both 'azure-cli-keyvault' and 'azure-cli-core' for this change.
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.
Well, presumably any command module that can integrate with KeyVault will need these methods, so I see the logic of putting this in core.
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.
@johanste would you like to provide a point of view?
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's fine in core, especially if it'll be used by multiple modules.
|
||
def register(application): | ||
application.register(application.TRANSFORM_RESULT, _resource_group_transform) | ||
application.register(application.TRANSFORM_RESULT, _x509_from_base64_to_hex_transform) |
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.
Does this belong in core?
Is it used only by keyvault or the vm module as well?
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.
I'm not sure it belongs in core. As far as I can tell, it will only be used in Key Vault.
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.
@derekbekoe I've looked into extension registration, but it's not completely apparent to me how a command module would register an extension outside of core. Is there an example of a module doing this?
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.
Having it in core is fine as looking at the code, it doesn't look like we support it outside of core.
1e6bac4
to
87b8e21
Compare
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.
Requesting some changes per our discussion about --scaffold. Also, it seems like "vm-format" should either be generic enough to work across all RPs OR that the logic of "vm_format" should be inside the VM/VMSS template_builder.py file.
type: command | ||
short-summary: Get a default policy for a self-signed certificate | ||
long-summary: > | ||
This default policy can be used in conjunction with `az keyvault create` to create a self-signed certificate. |
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 default policy is for self-signed certs--so if I wanted to make a non-self-signed cert, would I actually be able to use the output of this command as a basis, or would there be "pieces" that wouldn't get included that I would pretty much have no idea how to specify? In other words, does this essentially output a complete scaffolding of the certificate policy object?
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.
Just added --scaffold to the command.
$ az keyvault certificate get-default-policy -h
Command
az keyvault certificate get-default-policy: Get a default policy for a self-signed certificate.
This default policy can be used in conjunction with `az keyvault create` to create a self-
signed certificate. The default policy can also be used as a starting point to create
derivative policies.
Arguments
--scaffold : Create a fully formed empty policy structure.
Global Arguments
--debug : Increase logging verbosity to show all debug logs.
--help -h : Show this help message and exit.
--output -o: Output format. Allowed values: json, jsonc, table, tsv. Default: json.
--query : JMESPath query string. See http://jmespath.org/ for more information and examples.
--verbose : Increase logging verbosity. Use --debug for full debug logs.
Examples
Create a self-signed certificate with a the default policy
az keyvault create -g group-name -n vaultname -l westus --enabled-for-deployment true \
--enabled-for-template-deployment true
az keyvault certificate create --vault-name vaultname -n cert1 \
-p "$(az keyvault certificate get-default-policy)"
$ az keyvault certificate get-default-policy --scaffold
{
"attributes": {
"created": null,
"enabled": null,
"expires": null,
"notBefore": null,
"updated": null
},
"id": null,
"issuerParameters": {
"certificateType": null,
"name": null
},
"keyProperties": {
"exportable": null,
"keySize": null,
"keyType": null,
"reuseKey": null
},
"lifetimeActions": [
{
"action": null,
"trigger": null
}
],
"secretProperties": {
"contentType": null
},
"x509CertificateProperties": {
"ekus": null,
"keyUsage": [],
"subject": null,
"subjectAlternativeNames": null,
"validityInMonths": null
}
}
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.
Closer, but I would argue that for simple types we should include a sample value (string, int, datetime, etc). For things like lifetimeActions (which I believe there are only two types) include an example of each. The idea would be that someone could use this as a basis for editing and not have to go look things up somewhere (like the tests!) where it may or may not exist.
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.
should be updated with the following:
{
"attributes": {
"enabled": true,
"expires": null,
"notBefore": null
},
"issuerParameters": {
"certificateType": "(optional) DigiCert, GlobalSign or WoSign",
"name": "Unknown, Self, or {IssuerName}"
},
"keyProperties": {
"exportable": true,
"keySize": 2048,
"keyType": "(optional) RSA or RSA-HSM (default RSA)",
"reuseKey": true
},
"lifetimeActions": [
{
"action": {
"actionType": "AutoRenew"
},
"trigger": {
"daysBeforeExpiry": 90,
"lifetimePercentage": null
}
}
],
"secretProperties": {
"contentType": "application/x-pkcs12 or application/x-pem-file"
},
"x509CertificateProperties": {
"ekus": [
"1.3.6.1.5.5.7.3.1"
],
"keyUsage": [
"cRLSign",
"dataEncipherment",
"digitalSignature",
"keyEncipherment",
"keyAgreement",
"keyCertSign"
],
"subject": "C=US, ST=WA, L=Redmond, O=Contoso, OU=Contoso HR, CN=www.contoso.com",
"subjectAlternativeNames": {
"dnsNames": [
"hr.contoso.com",
"m.contoso.com"
],
"emails": [
"hello@contoso.com"
],
"upns": []
},
"validityInMonths": 24
}
}
az keyvault certificate create --vault-name vaultname -n cert1 \\ | ||
-p "$(az keyvault certificate get-default-policy)" | ||
|
||
az vm create -g group-name -n vm-name --admin-username deploy --image debian --secrets \'{secrets}\'' |
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.
What is '{secrets}' here?
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.
good catch -- will change
helps['keyvault secret vm-format'] = """ | ||
type: command | ||
long-summary: > | ||
Create a Key Vault certificate. Certificates can also be used as a secrets in provisioned virtual machines. |
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 long summary seems... inaccurate? Also, where's the short summary?
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.
copy pasta...
az vm create -g group-name -n vm-name --admin-username deploy --image debian --secrets \'{secrets}\'' | ||
""" | ||
|
||
helps['keyvault secret vm-format'] = """ |
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.
I have reservations about exposing such a specific command as this. Is this an ARM format, or it is VM specific? Ideally a consistent way of making KeyVault references would be used across resource providers (so compute and appservice would reference KeyVault entities the same) so this could simply be a keyvault secret get-reference
command or something NOT specific to VM.
Otherwise we'll end up with appservice-format
, sql-format
etc commands which is not the kind of surface area we want to expose.
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.
Agreed, but I don't think we know what those formats will look like as of now.
There is a portion of the format that is specific to VMs and that is the certificate store. Outside of that, we could simply take a list of secret ids and then add the formatting logic to VM and VMSS.
@johanste do you have any thoughts here?
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.
Based on our discussion yesterday, this is definitely a VM (Windows VM) specific thing. However, I think we should strongly consider having this command be something like: az vm/vmss get-keyvault-reference
or something similar. While the command logic would remain in the KeyVault module, the "natural" place for these types of commands to live in the command tree is in the module they pertain to (vm, appservice, sql, etc). These commands would only appear in these branches if the KeyVault module were installed (which, by default, it is).
@@ -152,24 +175,25 @@ def validate_policy_permissions(ns): | |||
if p not in cert_allowed: | |||
raise ValueError("unrecognized cert permission '{}'".format(p)) | |||
|
|||
|
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.
PEP8 loves you.
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.
sure does
# --secrets in vm and vmss create require. | ||
# | ||
# Also, vm-format should only produce JSON as it's the only format that makes sense for this cmd | ||
print(json.dumps(formatted, sort_keys=True, indent=2)) |
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.
Should we output a warning if the output is set to something else to alert the user that their output choice is being ignored?
"keyAgreement", | ||
"keyCertSign" | ||
], | ||
"subject": "C=US, ST=WA, L=Redmon, O=Test Noodle, OU=TestNugget, CN=www.mytestdomain.com", |
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.
You might want better defaults here. :)
"not_before": None | ||
}, | ||
"issuer_parameters": { | ||
"name": "Self" |
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.
Like we discussed, if a user is not doing a self-signed certificate, the command needs to show them what are the rest of the valid "issuer_parameters". --scaffold would be one option to expose an unusable (as-is) template that would give them all the tools they will need.
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.
will add
@@ -214,6 +214,7 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin | |||
register_cli_argument(scope, 'public_ip_address', help='Name of the public IP address when creating one (default) or referencing an existing one. Can also reference an existing public IP by ID or specify "" for None.', arg_group='Network') | |||
register_cli_argument(scope, 'public_ip_address_allocation', help=None, arg_group='Network', **enum_choice_list(['dynamic', 'static'])) | |||
register_cli_argument(scope, 'public_ip_address_dns_name', help='Globally unique DNS name for a newly created Public IP.', arg_group='Network') | |||
register_cli_argument(scope, 'secrets', help='Key Vault secrets as a JSON string or file \'[{ "sourceVault": { "id": "value" }, "vaultCertificates": [{ "certificateUrl": "value", "certificateStore": "cert store name (only on windows)"}] }]\'', completer=FilesCompleter(), type=file_type) |
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 seems weird to me to have a JSON string fed into a command to make yet another JSON string. Having a primitive (such as a list) be converted to a JSON string seems to be the limit of what makes sense. Why does this need to be a dictionary?
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 JSON for secrets is simply passed through to the rest api. One could not simply take the input of az keyvault secret list
and send that into --secrets. There is additional information needed such as certificateStore (on Windows).
@@ -573,6 +576,9 @@ def build_vmss_resource(name, naming_prefix, location, tags, overprovision, upgr | |||
if custom_data: | |||
os_profile['customData'] = b64encode(custom_data) | |||
|
|||
if secrets: | |||
os_profile['secrets'] = secrets |
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.
Rather than have a "vm-format" command, why can you not just do the formatting here through a helper function or within the template_builder.py file for VM/VMSS? There are things such as "build_storage_profile" and so on that essentially fill in those missing pieces.
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.
One could, but we'd have to introduce --certificate-store
on the VM command. I'm not entirely against this, but it would limit the certificate stores the secrets are added to.
Beyond just certificate stores, if another property is added to the vaultCertificates array objects such as fileName
or something else, then it would cause us to need to add something like --file-names [array of file names]
to the VM / VMSS create commands. I think it's less scary to extend vm-format
to produce:
[{
"sourceVault": {
"id": "value"
},
"vaultCertificates": [{
"certificateUrl": "value",
"certificateStore": "cert store name (only on windows)",
"fileName": "somecert"
}]
}]
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.
You already answered this one. :)
a41db3a
to
f3539f9
Compare
cli_keyvault_data_plane_command('keyvault certificate get-default-policy', custom_path.format('get_default_policy')) | ||
|
||
# secret vm formatter | ||
cli_command(__name__, 'keyvault secret vm-format', custom_path.format('get_vm_format_secret'), factory) |
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 still seems more appropriate as part of the VM "command tree" than here in keyvault.
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.
@johanste you and @tjprescott get to talk this over. I don't feel strongly where this lives, but I do feel that this will make secrets easier for users of the CLI.
3ad4b6c
to
a959a7b
Compare
@tjprescott I've update the command and after Travis is back to life, I think we should be all set. Let me know if you have any concerns. |
helps['vm format-secret'] = """ | ||
type: command | ||
long-summary: > | ||
Transform secrets into a form consumed by VMs and VMSS create via --secrets. |
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.
Should we specify that this is only applicable to Windows VMs?
vm_secrets=$(az vm format-secret -s "$secrets") \n | ||
|
||
az vm create -g group-name -n vm-name --admin-username deploy \\ | ||
--image debian --secrets "$vm_secrets" |
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.
Why do I need to run this command to use secrets for Debian if this command only exists because of the Windows cert store?
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 is non-trivial work to be done to format a key for use in vm secrets with or without cert store. Yes, we could accept only secret ids and fetch the vault resource id within az vm create
, but that would change the input format for --secret
when we need to input certificate store with a secret in az vm create
.
I opt'd for a single format for input for --secret
which matches the API paired with a transformation command which takes as input a list of secrets and optionally a certificate store. I think balances the tradeoffs between ease of use and flexibility to change the output of az vm format-secret
if need arises due to API changes.
Do you have a different design in mind?
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.
To recap our f2f:
- Make
--secrets
on VM create supportnargs='+', type=json.loads
and have VM create do the work of merging the different outputs fromaz vm format-secrets
. Keep the output offormat-secrets
as that required by VM create (not a simplified intermediate one). - Have the
vm format-secrets --secrets
arg take a list of secret URIs rather than the JSON output of the list command (or, have it accept both). You'd have to use query and TSV output to feed this in, but that's consistent with our current--ids
usage elsewhere in the CLI.
a959a7b
to
17ea71f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2212 +/- ##
=========================================
+ Coverage 72.2% 72.3% +0.09%
=========================================
Files 323 323
Lines 18098 18224 +126
Branches 2654 2689 +35
=========================================
+ Hits 13068 13176 +108
- Misses 4206 4218 +12
- Partials 824 830 +6
Continue to review full report at Codecov.
|
17ea71f
to
bb66387
Compare
@tjprescott I think this pull request reflects all of our conversations and should be ready for inclusion. |
@@ -214,7 +214,7 @@ def get_vm_size_completion_list(prefix, action, parsed_args, **kwargs): # pylin | |||
register_cli_argument(scope, 'public_ip_address', help='Name of the public IP address when creating one (default) or referencing an existing one. Can also reference an existing public IP by ID or specify "" for None.', arg_group='Network') | |||
register_cli_argument(scope, 'public_ip_address_allocation', help=None, arg_group='Network', **enum_choice_list(['dynamic', 'static'])) | |||
register_cli_argument(scope, 'public_ip_address_dns_name', help='Globally unique DNS name for a newly created Public IP.', arg_group='Network') | |||
register_cli_argument(scope, 'secrets', help='Key Vault secrets as a JSON string or file \'[{ "sourceVault": { "id": "value" }, "vaultCertificates": [{ "certificateUrl": "value", "certificateStore": "cert store name (only on windows)"}] }]\'', completer=FilesCompleter(), type=file_type) | |||
register_cli_argument(scope, 'secrets', multi_ids_type, help='One or many Key Vault secrets as JSON strings or files via \'@<file path>\' containing \'[{ "sourceVault": { "id": "value" }, "vaultCertificates": [{ "certificateUrl": "value", "certificateStore": "cert store name (only on windows)"}] }]\'', type=file_type, completer=FilesCompleter()) |
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.
Is "multi_ids_type" just a synonym for nargs='+'? If so, I would recommend just using that since this isn't really accepting multiple ids at all--it is taking multiple JSON parameters.
az keyvault certificate get-default-policy
az vm format-secret
Usage of:
az keyvault certificate get-default-policy
Usage of
az vm format-secret