-
Notifications
You must be signed in to change notification settings - Fork 3.3k
{Core} support get msal accesstoken with adal refresh token for vm ssh feature #12999
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
Conversation
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.
seems account not used, how about remove this wrapper?
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 a method which defined in the parent class. We'd better not to change the method signature in subclasses.
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 this be different under different cloud env?
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.
Currently there is only one link. This was described in this document. The document says it may change to sshservice.azure.net in the future.
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 feature currently seems only supported in public cloud https://docs.microsoft.com/en-us/azure/active-directory/devices/howto-vm-sign-in-azure-ad-windows#supported-azure-regions-and-windows-distributions, but we should add this endpoint in the cloud.py and throw error for non-public cloud
In reply to: 412720221 [](ancestors = 412720221)
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 think you should let the client pass scopes and data to get token to make this method more generic, this method contain too much implementation detail for SSHcert
In reply to: 413537394 [](ancestors = 413537394,412720221)
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 think you should let the client pass scopes and data to get token to make this method more generic, this method contain too much implementation detail for SSHcert
In reply to: 413537394 [](ancestors = 413537394,412720221)
Agreed. I will change it.
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 shouldn't be using the built-in hash because the value changes between runs. Here's the code that I've modified it to locally to make it generate the same hash each run to use the same SSH cert out of the token cache.
key_hash = hashlib.sha256()
key_hash.update(modulus.encode('utf-8'))
key_hash.update(exponent.encode('utf-8'))
key_id = key_hash.hexdigest()
jwk = {
"kty": "RSA",
"n": modulus,
"e": exponent,
"kid": key_id
}
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.
Thanks @rlrossiter . Updated.
|
vm ssh |
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.
can you add test for this api
|
vm ssh |
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.
ssh_certificate [](start = 13, length = 15)
please just implement a method like get_msal_for_resource and put the SSH related logic in vm module
qianwens
left a comment
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.
![]()
Description
This PR is try to provide an workaround solution for vmssh on adal.
As vmssh feature can only be implemented based on a msal access token. After investigation, we find it's possible to get msal access token with adal refresh token. Thus we can unblock vmssh before we adapt to msal completely.
These codes may be deprecated after the msal adoption finished.
Testing Guide
It should be tested through
sshextension which is in developing.History Notes
[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.