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

[Feature Request] deprecate data out of kwargs and make SSHCertificate functionality public. #755

Open
jiasli opened this issue Oct 10, 2024 · 6 comments

Comments

@jiasli
Copy link
Contributor

jiasli commented Oct 10, 2024

MSAL client type

Public, Confidential

Problem Statement

data has been used for a long time for getting SSH certificates, but it is still not publicly documented and is part of kwargs:

Azure CLI currently passes kwargs received by get_token() directly to MSAL. Since SDK is adding more keyword arguments, such as enable_cae (Azure/azure-sdk-for-python#37358), this will cause failure as enable_cae is not recognized by MSAL.

Azure CLI is considering making data an official keyword argument of get_token() so that only known arguments (scopes, claims_challenge, data) are passed to MSAL and kwargs received by get_token() is no longer directly passed to MSAL.

Proposed solution

Move data out of kwargs and make it a public keyword argument.

@ashok672 ashok672 changed the title [Feature Request] Move data out of kwargs and make it public [Feature Request] deprecate data out of kwargs and make SSHCertificate functionality public. Oct 16, 2024
@jiasli
Copy link
Contributor Author

jiasli commented Oct 16, 2024

Consider it another way: Since MSAL.NET has made getting SSH certificate a public interface via WithSSHCertificateAuthenticationScheme, MSAL.PY should do the same and have a similar public interface for getting SSH certificate.

3 years ago, when Azure CLI was migrated to MSAL, we had the assumption that kwargs received by get_token() was dedicated to MSAL and that's why we pass kwargs directly to MSAL. However, SDK keeps adding more arguments which are also received by kwargs in get_token(), thus breaking our assumption.

If MSAL exposed data as a public interface, we could have avoided the kwargs assumption from the very beginning.

@rayluo
Copy link
Collaborator

rayluo commented Oct 31, 2024

Proposed solution

Move data out of kwargs and make it a public keyword argument.

To summarize, Azure CLI is not blocked by the proposal above, and now the PR in Azure CLI has been merged.

I would suggest close this feature request as Won't-fix. We can revisit the topic of exposing the SSH Cert feature (when/if other apps also needs it), but its official api will unlikely be a generic data.

@ashok672
Copy link

@localden - We need to triage this tomorrow.

@bgavrilMS
Copy link
Member

Consider it another way: Since MSAL.NET has made getting SSH certificate a public interface via WithSSHCertificateAuthenticationScheme, MSAL.PY should do the same and have a similar public interface for getting SSH certificate.

3 years ago, when Azure CLI was migrated to MSAL, we had the assumption that kwargs received by get_token() was dedicated to MSAL and that's why we pass kwargs directly to MSAL. However, SDK keeps adding more arguments which are also received by kwargs in get_token(), thus breaking our assumption.

If MSAL exposed data as a public interface, we could have avoided the kwargs assumption from the very beginning.

I am not aware of a single 3p customer using this. Only 1-2 internal teams use this for very specific sceanrios, i.e. provision Linux VMs. As such I do not think it qualifies for a fully fledged API. I am sure there is server telemetry for this.

In hindsight, it was a mistake to create an explicit public API in MSAL.NET without a good E2E scenario & docs & motivation.

@rayluo
Copy link
Collaborator

rayluo commented Nov 13, 2024

I am not aware of a single 3p customer using this. Only 1-2 internal teams use this for very specific sceanrios, i.e. provision Linux VMs. As such I do not think it qualifies for a fully fledged API.

Fully agreed. Also, in the case of SSH Certificate, there is only one internal team using it, and that is the Azure CLI team. The current undocumented API was designed for that, will continue to work, and I don't think it is worth becoming a documented public API.

@ashok672 ashok672 added the ado tracked in ado label Dec 4, 2024
@ashok672
Copy link

ashok672 commented Dec 4, 2024

This is being tracked as a feature in ADO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants