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

Send User-Agent header to service #182

Merged
merged 7 commits into from
Mar 26, 2024

Conversation

arthuraraujo-msft
Copy link
Contributor

@arthuraraujo-msft arthuraraujo-msft commented Mar 25, 2024

Related Issues

Why is this change being made?

Service will capture and use the User-Agent header for their own telemetry to know which calls are coming from the Client library. This is a "best-effort" model and there is no service enforcement.

What is being changed?

  • Sending "User-Agent" header on every request.
  • Format is "Microsoft-SFSClient/<version> (<OS Version>; <Machine Info>)"
    • Examples:
      • Microsoft-SFSClient/1.0.0 (Windows NT 10.0; x64)
      • Microsoft-SFSClient/1.0.0 (Ubuntu 22.04.3 LTS; x86_64)
    • In Windows there is no longer an API to retrieve the version. We must rely on whatever the application that is using the library has registered in a manifest file. At least the machine info should be trustworthy.
    • In Linux it also changes depending on the distribution. The most common place to find distro + version is the /etc/os-release file. After that, the uname() function returns some generic info.

How was the change tested?

  • New tests are being added: added lines to functional CurlConnectionTests and SFSClientImplTests to expect the same header in the mock web server.

@arthuraraujo-msft arthuraraujo-msft requested a review from a team as a code owner March 25, 2024 13:25
@arthuraraujo-msft arthuraraujo-msft changed the title Send User-Agent header Send User-Agent header to service for telemetry Mar 25, 2024
@arthuraraujo-msft arthuraraujo-msft changed the title Send User-Agent header to service for telemetry Send User-Agent header to service Mar 25, 2024
Copy link

@JeffMill JeffMill left a comment

Choose a reason for hiding this comment

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

approved with comments

client/src/details/connection/CurlConnection.cpp Outdated Show resolved Hide resolved
client/src/details/connection/CurlConnection.cpp Outdated Show resolved Hide resolved
client/src/details/connection/CurlConnection.cpp Outdated Show resolved Hide resolved
client/src/details/connection/HttpHeader.cpp Show resolved Hide resolved
@arthuraraujo-msft arthuraraujo-msft merged commit eafcfa6 into main Mar 26, 2024
3 checks passed
@arthuraraujo-msft arthuraraujo-msft deleted the user/arthuraraujo/send-user-agent branch March 26, 2024 11:27
// In Windows, the API below only works if the application has a manifest file with indications to the supported OS
// version (https://learn.microsoft.com/en-us/windows/win32/sysinfo/targeting-your-application-at-windows-8-1).
// If the caller application does not have a manifest file, this function will just return "Windows"
return IsWindows10OrGreater() ? "Windows NT 10.0" : "Windows";

Choose a reason for hiding this comment

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

this is already incorrect as it's reporting Windows 10 (and there's no such thing as "Windows NT 10") on Windows 11.

WU must need this info as it has OS version specific applicability. I'm guessing it uses GetVersionEx() [ignore the MSDN warnings that have been there since windows 8] or perhaps does a registry read -- see

Get-ItemProperty 'HKLM:\Software\Microsoft\Windows NT\CurrentVersion'

If you can't find a definitive source from WU folks, LMK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see reports on the web that GetVersionEx() function returns unexpected values
Like https://stackoverflow.com/questions/58294262/getversionex-windows-10-detected-as-windows-8

The current suggestion from MSDN is to use these helper functions IsWindows10OrGreater()
That's probably going to be the best guess we can make at this point.
Windows 11 can no longer be detected programmatically and the recommendation is instead to check for capabilities available on the system.

Copy link

@JeffMill JeffMill Mar 26, 2024

Choose a reason for hiding this comment

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

as i mention in another comment, while it's undocumented, it's been there as long as I've been at Windows which is a LONG time....for build info it's probably fine for us to use some variant of:

$cv = Get-ItemProperty 'HKLM:\Software\Microsoft\Windows NT\CurrentVersion'
'Version {0} (OS Build {1}.{2})' -f $cv.DisplayVersion, $cv.CurrentBuildNumber,$cv.UBR

which gives the same value as winver.exe, "Version 23H2 (OS Build 22631.3296)" on my dev box.

likely service team just needs a BuildNumber as that's unique across all windows builds ever, e.g.

  • The first Windows 10 build number is 10240
  • The last Windows 8.1 build number is 9600
  • The last Windows XP build number is 3790

so maybe just send up BuildNumber and architecture ID ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool!
Created #185

client/src/details/OSInfo.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send User-Agent in the request header
3 participants