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

Add nerdctl version to User-Agent header #704

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ningziwen
Copy link
Contributor

Right now, nerdctl reports its user-agent string as "containerd/${containerd_version}`, so container registries can't distinguish nerdctl pulls from containerd pulls. It would be useful to re-set the user-agent as "nerdctl/${nerdctl_version} containerd/${containerd_version}".

Signed-off-by: Ziwen Ning [email protected]

@AkihiroSuda
Copy link
Member

container registries can't distinguish nerdctl pulls from containerd pulls

Why do you need to distinguish?

@samuelkarp
Copy link
Member

Hey @AkihiroSuda,

Our apologies for not explaining this super clearly; we’ll be sure to open issues for changes like this first next time.

We’d like to see nerdctl have a distinct User-Agent for the same reasons we’d like to see other clients have distinct User-Agent strings: (a) if our customers use nerdctl and run into problems, a distinct User-Agent string helps us isolate relevant logs so we can more effectively troubleshoot and reproduce any behavior, and (b) understanding usage of different clients (like nerdctl) helps us understand if there are additional features we can develop for the registry or if there are opportunities for us to help out in the clients themselves.

Thanks!
Sam

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Sorry, NOTLGTM.

I don't think we should leak unnecessary information that can be used for identifying users (and also for finding vulnerable versions of clients, potentially) without the user's consent.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Jan 25, 2022

Maybe we can extend hosts.toml to support specifying custom user agents (or any HTTP header), and Amazon can extend docker-credential-ecr-login to automatically configure hosts.toml (with notice to users, perhaps).

EDIT: hosts.toml seems already supporting custom HTTP headers https://github.com/containerd/containerd/blob/0def98e462706286e6eaeff4a90be22fda75e761/docs/hosts.md#header-fields-in-the-toml-table-format

@samuelkarp
Copy link
Member

Further discussion is on containerd/containerd#6474. I'd still like to see this change made as I think that having nerdctl version information is valuable to the ecosystem at large.

@tompittson
Copy link

Another issue I have encountered that this will solve is that the User-Agent header is the default "Go-http-client" when executing a nerdctl login command.

@apostasie
Copy link
Contributor

Curious what is our thinking overall on this now, and if it has evolved over the last 3 years.

Personal 2 cents:

  • agreed with @samuelkarp that a distinct UA string is not a threat to anonymity and that it has definite benefits for registry operators
  • I also do not think there is a realistic scenario where an attacker would derive actual value from a UA string (can elaborate on that if needed)

Now:

  • I do not see a reason to customize containerd UA string - nerdctl pulls are containerd pulls, right? - I do not see what kind of issues could arise with pull that would be nerdctl specific...
  • I am more interested in the login command, which IS done client-side, and which IS fully-identifying the user already (making the anonymity argument moot), and in that case, I do believe we should have a default nerdctl/vx.y.z UA (that users can still override from hosts.toml if they wish) - and that one would certainly help with point b from Add nerdctl version to User-Agent header #704 (comment)

Where do you folks stand?

@AkihiroSuda
Copy link
Member

At least we should hide the git commit hash

Not just for anonymity...
kubernetes/registry.k8s.io#286 (comment)

@apostasie
Copy link
Contributor

At least we should hide the git commit hash

Not just for anonymity... kubernetes/registry.k8s.io#286 (comment)

Yep. nerdctl/vx.y.z is absolutely enough (and again, only for login).

I must say I am ambivalent on this. I do definitely support using a specific UA for point b. mentioned above as I said - but I do think server-side behavioral adaptation based on UA sniffing is just abhorrent (there is a reason the frontend world moved to feature testing rather than UA sniffing, and that any operator worth their salt are instead looking at request urls and overall behaviors rather than UA).

Someone in the kube discussion you provided is saying just as much - waf rules on UA are idiotic and very easy to workaround.

@apostasie
Copy link
Contributor

Integrating defaultNerdUserAgent = fmt.Sprintf("nerdctl/%s (os: %s)", version.GetVersion(), runtime.GOOS) in the (WIP) login refactor at #3249 (we can continue the discussion on this specific aspect over there when this PR will be ready).

That will make nerdctl login send the above UA, unless there is one user-defined in hosts.toml.

@samuelkarp would that change on nerdctl login satisfy your needs?

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

Successfully merging this pull request may close these issues.

5 participants