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

resolver: set buildkit own user-agent #2593

Merged
merged 1 commit into from Feb 11, 2022
Merged

Conversation

tonistiigi
Copy link
Member

Instead of using containerd defaults set our own user-agent on registry requests.

For dev versions I added some pattern matching to avoid putting sha in the requests as it would be useless for any kind of statistics.

image

(The double value comes from some tracing artifact, there is no actual double header).

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

version = matchs[0][1] + "-dev"
}

return "buildkit/" + version
Copy link
Member

@crazy-max crazy-max Feb 2, 2022

Choose a reason for hiding this comment

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

Could we add the runtime? That could be useful:

return fmt.Sprintf("buildkit/%s go/%s %s/%s", version, runtime.Version()[2:], runtime.GOOS, runtime.GOARCH)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. Go version can be implied from the main version. OS/Arch can be useful but we need to draw the line somewhere. The previous containerd string used the same formatting with only version.

@AkihiroSuda
Copy link
Member

Generally I'd prefer not to leak so much info in the User-Agent, which can be used for tracking anonymous users.

@tonistiigi
Copy link
Member Author

@AkihiroSuda You mean you prefer adding platform value or what do you mean by "so much info"?

@AkihiroSuda
Copy link
Member

@AkihiroSuda You mean you prefer adding platform value or what do you mean by "so much info"?

I felt leaking the version number might decrease anonymity, and potentially helpful for malicious registry operators to find hosts running vulnerable versions of BuildKit. (relevant: containerd/containerd#6474 containerd/nerdctl#704 (review))

But maybe acceptable, if the patch version (Z of vX.Y.Z) and the commit hash are omitted.

@tonistiigi
Copy link
Member Author

I already took out the commit hash. For the regular version, I'm not sure if there is any risk. We only support secured connections by default anyway.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

I've updated so it is only a major version on releases and the default 0.0.0 version for anything that doesn't is not associated with a release.

@tonistiigi tonistiigi merged commit 196efb9 into moby:master Feb 11, 2022
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.

None yet

3 participants