Enable better certificate importing for Key Vault #2754
Conversation
| 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.") |
There was a problem hiding this comment.
The help text shouldn't make reference to "base64EncodedCertificate"
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed, but I'm hesitant to take that up right now.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| except ValueError: | ||
| pass | ||
| except crypto.Error: | ||
| pass |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
damn... C# snuck into my head
devigned
force-pushed
the
bug/kv-cert-import
branch
from
afd01d5
to
8edf47f
Compare
|
@tjprescott and @derekbekoe you guys good with this pr? |
|
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. |
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 importwas 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.