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

Exclude Windows Server 2016 from WAM support #629

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

mjcheetham
Copy link
Collaborator

Windows Server 2016 does not support WAM, so we should not try to enable the broker unless we're on Server 2019 and later.

Also update the Windows (client; non-server) version checks to be tighter. Previously any major version of 10 or greater was considered "supported", but this is wrong. Windows 10 build 15063 was the first to support WAM.

Fixes #487

Windows Server 2016 does not support WAM, so we should not try to enable
the broker unless we're on Server 2019 and later.

Also update the Windows (client; non-server) version checks to be
tighter. Previously any major version of 10 or greater was considered
"supported", but this is wrong. Windows 10 build 15063 was the first to
support WAM.
@mjcheetham mjcheetham added bug A bug in Git Credential Manager platform:windows Specific to the Windows platform windows-broker Related to the Windows "Web Account Manager" authentication broker labels Mar 1, 2022
Copy link

@jeffhostetler jeffhostetler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand enough of the issues or APIs to have an an
informed opinion, but LGTM.

@mjcheetham
Copy link
Collaborator Author

cc @bgavrilMS

@bgavrilMS
Copy link

Not sure you need to go to all this trouble, we fixed this in MSAL itself.

@mjcheetham
Copy link
Collaborator Author

mjcheetham commented Mar 1, 2022

@bgavrilMS

Not sure you need to go to all this trouble, we fixed this in MSAL itself.

The problem is that IsBrokerAvailable() is an instance method on PublicClientApplicationBuilder, which means we need to load MSAL and create a PCAB (with client ID etc) in Main() for every GCM invocation in order to set the COM security if needed. We don't always know the client ID at this stage, and the extra cost required isn't worth it IMO.

@mjcheetham mjcheetham merged commit 2a077a8 into git-ecosystem:main Mar 2, 2022
@mjcheetham mjcheetham deleted the wam-server2016 branch March 2, 2022 11:41
@ldennington ldennington mentioned this pull request Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in Git Credential Manager platform:windows Specific to the Windows platform windows-broker Related to the Windows "Web Account Manager" authentication broker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when using WAM on Windows Server 2016
4 participants