-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[KeyVault] Onboard KeyVault Round 1 #1037
Conversation
@@ -74,6 +81,7 @@ def __init__(self, storage=None, auth_ctx_factory=None): | |||
env = get_env() | |||
self._management_resource_uri = env[ENDPOINT_URLS.MANAGEMENT] | |||
self._graph_resource_uri = env[ENDPOINT_URLS.ACTIVE_DIRECTORY_GRAPH_RESOURCE_ID] | |||
self._key_vault_resource_uri = env[ENDPOINT_URLS.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.
i don't think we will need _graph_resource_uri
and _key_vault_resource_uri
, just remove
The rest change on Profile LGTM
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.
Fixed.
keyvault group
keyvault set-policy
keyvault key group
keyvault key create
keyvault key set-attributes
keyvault secret group
keyvault secret set
keyvault secret set-attributes
|
Issue #1029 is preventing this from passing the CI. Please still review. 😅 |
cc/ @pomortaz for review and feedback (authentication will be incorporated in a later PR) |
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.
Added a few different comments.
@@ -0,0 +1,34 @@ | |||
-----BEGIN 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.
This file is in the root directory.
Think it should be moved to src/command_modules/azure-cli-keyvault/tests or something like that if it's needed for a keyvault test.
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 will eventually be moved there. Right now I'm using it for development of the key import command.
@@ -0,0 +1,31 @@ | |||
-----BEGIN 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.
Same with this: think it should move from root project directory.
@@ -0,0 +1,42 @@ | |||
{ |
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.
Same with this: think it should move from root project directory.
@@ -22,10 +22,10 @@ def exec_command(command): | |||
|
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.
My PR that just merged has this change so we should remove this change (and rebase) to prevent a conflict.
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.
Done
|
||
_environments = { | ||
ENV_DEFAULT: { | ||
ENDPOINT_URLS.MANAGEMENT: 'https://management.core.windows.net/', | ||
ENDPOINT_URLS.ACTIVE_DIRECTORY_AUTHORITY : 'https://login.microsoftonline.com', | ||
ENDPOINT_URLS.ACTIVE_DIRECTORY_GRAPH_RESOURCE_ID: 'https://graph.windows.net/' | ||
ENDPOINT_URLS.ACTIVE_DIRECTORY_GRAPH_RESOURCE_ID: 'https://graph.windows.net/', | ||
ENDPOINT_URLS.KEY_VAULT: 'https://vault.azure.net' |
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.
@yugangw-msft Are you okay with having service endpoints defined in the azure.cli.core package like this?
You're more familiar with the endpoint urls so want to get your input.
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 was where they already were defined. I added the endpoint for KeyVault. But this will need to change once the challenge piece is in the convenience layer. Also, I'm definitely not sure about the values I put in for the other hard-coded clouds (which really don't seem like they should be hard coded).
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.
Yes, i am okay to have the service endpoints to defined here (in one single file), the same pattern used by xplat. When our profile design consolidated, we can do any appropriate change we like
raise ValueError('URL cannot be None') | ||
|
||
if not challenge: | ||
raise ValueError('challenge cannot be None') |
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.
really nitpicky but capital C for challenge like everywhere else?
|
||
class KeyVaultClient(object): | ||
|
||
def __init__(self, credentials, api_version='2015-06-01', accept_language='en-US', long_running_operation_retry_timeout=30, generate_client_request_id=True, filepath=None): |
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 the client work with multiple API versions? (I'm thinking of the multi-cloud story)
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.
Ultimately, this will be packaged with the data plane SDK, so it will be revved and versioned along with the SDK.
from . import httpbearerchallenge | ||
from . import httpbearerchallengecache as challengecache | ||
|
||
class keyvaultcredential(authentication): |
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.
KeyVaultCredential? - https://www.python.org/dev/peps/pep-0008/#class-names
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 looks like I must have pressed a funky shortcut in VS because it seems to have made this file all lowercase. Currently it's not used by anything (though it may be used in the convenience layer auth later on).
|
||
def signed_session(self): | ||
session = super(keyvaultcredential, self).signed_session() | ||
session.headers['authorization'] = 'bearer {}'.format(self._token or '') |
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.
Whilst HTTP headers are case-insensitive, I'd recommend 'Authorization' and 'Bearer'.
It could/potentially/maybe cause an obscure issue in the future.
Here they use the capitalized form: https://msdn.microsoft.com/en-us/library/azure/dn903615.aspx
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.
Fixed. 'Twas the lowercase shortcut key.
def signed_session(self): | ||
session = super(keyvaultcredential, self).signed_session() | ||
session.headers['authorization'] = 'bearer {}'.format(self._token or '') | ||
print('signed session w/ auth: {}'.format(session.headers['authorization'])) |
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.
Shouldn't print to stdout here.
Do you need to print the Authorization header? If so, consider using logging.
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.
No, this was part of debugging. I'll remove it.
ENDPOINT_URLS.ACTIVE_DIRECTORY_GRAPH_RESOURCE_ID: 'https://graph.chinacloudapi.cn/' | ||
ENDPOINT_URLS.ACTIVE_DIRECTORY_GRAPH_RESOURCE_ID: 'https://graph.chinacloudapi.cn/', | ||
# TODO: Verify KeyVault endpoint | ||
ENDPOINT_URLS.KEY_VAULT: 'https://vault.chinacloudapi.cn' |
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.
@yugangw-msft how do I verify this URL?
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 cross check from xplat environment.js where keyvault team put in the endpoints themselves. If still missing, please ask @karlaugsten
@tjprescott the endpoint for KeyVault in china is https://vault.azure.cn |
…re-cli into KeyVaultDataPlane
@tjprescott: I have worked out why your recent build failed - https://travis-ci.org/Azure/azure-cli/jobs/165952645. In src/command_modules/azure-cli-keyvault/setup.py, you should list the packages you want to be included in the When I list
See this other setup.py file as an example - https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-network/setup.py#L51. |
@derekbekoe you're a genius! Thank you 😄 |
…re-cli into KeyVaultDataPlane # Conflicts: # src/command_modules/azure-cli-keyvault/setup.py
This is the first in a series of PRs which will onboard the KeyVault data plane commands. This PR:
Note that the client was generated in autorest and will be removed in favor of direct import once the Python SDK is approved and released.
@yugangw-msft for the changes to profile and @derekbekoe for the changes to the management plane commands. Anyone feel free to drop feedback on the commands themselves. 😄