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

Enable better certificate importing for Key Vault #2754

Merged
merged 1 commit into from Apr 6, 2017

Conversation

Projects
None yet
5 participants
@devigned
Copy link
Member

commented Apr 4, 2017

This pull request provides better documentation for Key Vault certificate importing showing the path for creating a service principal, importing the cert into key vault, and using it to provision a vm via secrets.

This pull request also fixes an issue where we az keyvault certificate import was not sniffing the type of certificate and properly specifying the content type. Now, we sniff out either PEM or PFX and properly encode the payload for the server.

@devigned devigned requested a review from tjprescott Apr 4, 2017

register_attributes_argument('keyvault certificate set-attributes', 'certificate', CertificateAttributes)
register_cli_argument('keyvault certificate set-attributes', 'expires', ignore_type)
register_cli_argument('keyvault certificate set-attributes', 'not_before', ignore_type)

for item in ['create', 'set-attributes', 'import']:
register_cli_argument('keyvault certificate {}'.format(item), 'certificate_policy', options_list=('--policy', '-p'), help='JSON encoded policy defintion. Use @{file} to load from a file.', type=get_json_object)

register_cli_argument('keyvault certificate import', 'base64_encoded_certificate', options_list=('--file', '-f'), completer=FilesCompleter(), help='PKCS12 file or PEM file containing the certificate and private key.', type=base64_encoded_certificate_type)
register_cli_argument('keyvault certificate import', 'certificate_data', options_list=('--file', '-f'), completer=FilesCompleter(), help='PKCS12 file or PEM file containing the certificate and private key.', type=certificate_type)
register_cli_argument('keyvault certificate import', 'password', help="If the private key in base64EncodedCertificate is encrypted, the password used for encryption.")

This comment has been minimized.

Copy link
@tjprescott

tjprescott Apr 4, 2017

Member

The help text shouldn't make reference to "base64EncodedCertificate"

This comment has been minimized.

Copy link
@devigned

devigned Apr 5, 2017

Author Member

good catch -- I'll clean that up.

return dateutil.parser.parse(asn1_date.decode('utf-8'))


def import_certificate(client, vault_base_url, certificate_name, certificate_data,

This comment has been minimized.

Copy link
@tjprescott

tjprescott Apr 4, 2017

Member

It seems to me that this logic should be moved to azure.keyvault.key_vault_client.py (the convenience wrapper) so that it benefits all Python developers and not just CLI users.

This comment has been minimized.

Copy link
@devigned

devigned Apr 5, 2017

Author Member

Agreed, but I'm hesitant to take that up right now.

@codecov-io

This comment has been minimized.

Copy link

commented Apr 4, 2017

Codecov Report

Merging #2754 into master will increase coverage by <.01%.
The diff coverage is 74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2754      +/-   ##
==========================================
+ Coverage   62.83%   62.84%   +<.01%     
==========================================
  Files         480      480              
  Lines       25783    25824      +41     
  Branches     3904     3911       +7     
==========================================
+ Hits        16201    16229      +28     
- Misses       8574     8582       +8     
- Partials     1008     1013       +5
Impacted Files Coverage Δ
...ault/azure/cli/command_modules/keyvault/_params.py 85.93% <100%> (+0.11%) ⬆️
.../azure/cli/command_modules/keyvault/_validators.py 73.33% <100%> (-0.53%) ⬇️
...yvault/azure/cli/command_modules/keyvault/_help.py 100% <100%> (ø) ⬆️
...ult/azure/cli/command_modules/keyvault/commands.py 100% <100%> (ø) ⬆️
...vault/azure/cli/command_modules/keyvault/custom.py 67.37% <69.04%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dda58c...8edf47f. Read the comment docs.

@derekbekoe derekbekoe added the KeyVault label Apr 5, 2017

except ValueError:
pass
except crypto.Error:
pass

This comment has been minimized.

Copy link
@derekbekoe

derekbekoe Apr 5, 2017

Member

This can be

except (ValueError, crypto.Error):
    pass

I believe.


if certificate_policy:
secret_props = certificate_policy.get('secret_properties')
if secret_props is SecretProperties:

This comment has been minimized.

Copy link
@derekbekoe

derekbekoe Apr 5, 2017

Member

Should this be if isinstance(secret_props, SecretProperties)?
http://stackoverflow.com/questions/2987958/how-is-the-is-keyword-implemented-in-python
Unless you really do intend to check the object identity.

This comment has been minimized.

Copy link
@derekbekoe

derekbekoe Apr 5, 2017

Member

Same below with the checks for is dict it looks like.

$ python
Python 2.7.6 (default, Jun 22 2015, 17:58:13) 
>>> a = {}
>>> a is dict
False
>>> b = {'hi': 1}
>>> b is dict
False
>>> isinstance(b, dict)
True
>>> isinstance(a, dict)
True

This comment has been minimized.

Copy link
@devigned

devigned Apr 5, 2017

Author Member

damn... C# snuck into my head

@devigned devigned force-pushed the devigned:bug/kv-cert-import branch from afd01d5 to 8edf47f Apr 5, 2017

@devigned

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

@tjprescott and @derekbekoe you guys good with this pr?

@tjprescott

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

I'm okay with the logic--but I really think this should be elevated to the SDK or, even better, the service. I wouldn't block the PR for that though.

@devigned devigned merged commit e0ca649 into Azure:master Apr 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@devigned devigned deleted the devigned:bug/kv-cert-import branch Apr 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.