NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
16.54k stars 13.02k forks source link

.NET Core tooling stops working due to `ñ` in release codename and lack of HTTP headers sanitization #315574

Open nazarewk opened 1 month ago

nazarewk commented 1 month ago

EDIT1: Series of issues filed in ends up with this one https://github.com/Azure/azure-sdk-for-net/issues/44300

EDIT2: there are more issues across .NET than just Azure SDK (eg: NuGet has it's own faulty implementation), see the discussion for details.

Describe the bug

Yesterday evening I ran update to latest nixos-unstable, today when using msgraph-cli I'm running across Request headers must contain only ASCII characters. errors.

Running it with debug I have noticed following header:

User-Agent:azsdk-net-Identity/1.10.4 (.NET 8.0.5; NixOS 24.11 (Vicuña))

Steps To Reproduce

Steps to reproduce the behavior:

  1. run (probably) anything doing HTTP requests using .NET SDK/azsdk on latest nixos-unstable
  2. get the error

Expected behavior

I'm quite sure this is improper escaping on the side of .NET SDK, but still using non-latin characters in names NixOS might yield such errors here and there.

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

Add any other context about the problem here.

Notify maintainers

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
output here

Add a :+1: reaction to issues you find important.

nazarewk commented 1 month ago

seems to be a .NET Core thing https://stackoverflow.com/questions/49879502/is-there-built-in-support-for-automatically-escaping-invalid-http-header-charact

nazarewk commented 1 month ago

Actually I can't fix it myself:

(lib.mkIf (lib.trivial.codeName == "Vicuña") { system.nixos.codeName = lib.mkForce "Vicuna"; })

results in:

       error: The option `system.nixos.codeName' is read-only, but it's set multiple times. Definition values:
       - In `/nix/store/s94zxf55wpvq06d80vjp1s6bzxz32qsl-source/nixos/modules/misc/version.nix': "Vicuña"
       - In `/nix/store/19sqqhhf11l3v5458f52d4npifn8pc24-source/modules/profile/machine/baseline/default.nix': "Vicuna"
nazarewk commented 1 month ago

I have finally got it to work, you can find code and explanation comments at https://github.com/nazarewk-iac/nix-configs/commit/5a76297893bbb171c440da90fb5e417bf9993bfe#diff-9bd1903895edc8ec69ce6debf7823905ba99fa41c35d4482d0464ab7f47aa627 :

/* Last reviewied: 2024-05-29

  fixes issues with lack of HTTP header sanitization in .NET Core, see:
  - https://github.com/NixOS/nixpkgs/issues/315574
  - https://github.com/microsoftgraph/msgraph-cli/issues/477
*/
{ lib, options, ... }: {
  /*
    using just `readOnly` because it can contain neither of: default, example, description, apply, type
    see https://github.com/NixOS/nixpkgs/blob/aae38d0d557d2f0e65b2ea8e1b92219f2c0ea8f9/lib/modules.nix#L752-L756
   */
  options.system.nixos.codeName = lib.mkOption { readOnly = false; };
  config.system.nixos.codeName =
    let
      codeName = options.system.nixos.codeName.default;
      renames."Vicuña" = "Vicuna";
    in
      renames."${codeName}" or (throw "Unknown `codeName`: ${codeName}, please add it to `renames` in `ascii-workaround.nix`");
}
l0b0 commented 1 month ago

Ping @NixOS/dotnet

l0b0 commented 1 month ago

Whoops, sorry. I thought I was assigning a reviewer! :facepalm:

GGG-KILLER commented 1 month ago

I believe there's nothing we can do here ourselves, instead it seems to be a problem with the upstream tool not accounting for this.

In this case I think it'd be be better to open an issue on the msgraph-cli repo so they can work on it.

eclairevoyant commented 1 month ago

There already is such an issue: https://github.com/microsoftgraph/msgraph-cli/issues/477

nazarewk commented 1 month ago

There already is such an issue: microsoftgraph/msgraph-cli#477

Actually this is probably THE one affecting multiple tools https://github.com/Azure/azure-sdk-for-net/issues/44300

purepani commented 1 month ago

As per https://github.com/Azure/azure-sdk-for-net/issues/44300#issuecomment-2145495387, it looks like setting the environment variable AZURE_TELEMETRY_DISABLED=1 fixes the issue(at least when using the dotnet cli). Might be worth putting that into the deriviation, or patching it with the setting also mentioned in that comment.

GGG-KILLER commented 1 month ago

Upstream fix (Azure/azure-sdk-for-net#44386) has been merged, now we have to wait for consumers of the SDK package to update to it.

m-redding commented 1 month ago

This has been released in Azure.Core - https://www.nuget.org/packages/Azure.Core/1.40.0 . On the next Azure.Identity release the package reference to Azure.Core will be upgraded. Until then, you could opt to take a direct reference on Azure.Core 1.40 to hoist up your version.

qubitnano commented 1 month ago

What exactly needs to be updated on NixOS? Are we waiting for a new dotnet sdk/runtime version with this fix?

nazarewk commented 1 month ago

What exactly needs to be updated on NixOS? Are we waiting for a new dotnet sdk/runtime version with this fix?

as far as I understand it is the issue with Azure SDK and all affected tools need to update it by themselves.

The only thing NixOS can do as a workaround is:

As per Azure/azure-sdk-for-net#44300 (comment), it looks like setting the environment variable AZURE_TELEMETRY_DISABLED=1 fixes the issue(at least when using the dotnet cli). Might be worth putting that into the deriviation, or patching it with the setting also mentioned in that comment.

SuperSandro2000 commented 1 month ago

As per Azure/azure-sdk-for-net#44300 (comment), it looks like setting the environment variable AZURE_TELEMETRY_DISABLED=1 fixes the issue(at least when using the dotnet cli). Might be worth putting that into the deriviation, or patching it with the setting also mentioned in that comment.

At least for the dotnet update scripts, that isn't working and still failing with the rather misleading error error NU1301: Unable to load the service index for source https://api.nuget.org/v3/index.json unless I change the release name.

purepani commented 1 month ago

The fix is no longer working for me, so I have no clue what is happening...

purepani commented 1 month ago

How hard would it be to override the azure-sdk version to require 1.40 for any package that is broken by this?

SuperSandro2000 commented 1 month ago

I've just now set system.nixos.codeName = "Vicuna"; since the linked PR to change it, went no where.

purepani commented 1 month ago

Actually, are we sure that this solution propagating would fix the dotnet-cli issues? The original issue was about msgraph, and the fix was implemented in the azure sdk, and at least on a surface level, I'm not quite sure why the dotnet-sdk would depend on the azure-sdk

SuperSandro2000 commented 1 month ago

Hmmm 🤔 I didn't find it either in the generated sources

I am not sure if this is a duplicate of #316374 or maybe it is a separate issue. Nevertheless the root cause is the same.

anpin commented 1 month ago

@purepani

The fix is no longer working for me, so I have no clue what is happening...

Some packages might have been cached, so the CLI wouldn’t make any requests. That’s what I have seen on my machine at least. Only renaming the distro worked for me, setting aforementioned variable did nothing.

purepani commented 1 month ago

@purepani

The fix is no longer working for me, so I have no clue what is happening...

Some packages might have been cached, so the CLI wouldn’t make any requests. That’s what I have seen on my machine at least. Only renaming the distro worked for me, setting aforementioned variable did nothing.

I am reasonably certain I attempted to build using dotnet, it failed, and I then set the environment variable and it worked; though it was a week ago, so I may be misremembering.

SuperSandro2000 commented 1 month ago

I think we also need to patch dotnet/dotnet but I didn't fully verify it and it could be that I missed something. Leaving the breadcrumbs here anyway, wo that other people get the chance to verify my initial thought without having to search for the traces first.

In the first link PRETTY_NAME gets read and in the last one it is added to some user agent which might be the one that is used to fetch from nugget (I did not verify this further)

m-redding commented 1 month ago

The fix in Azure.Core will fix tools that were failing with "Request headers must contain only ASCII characters" coming from packages that use Azure.Identity or other Azure SDK libraries.

It doesn't seem like the "error NU1301: Unable to load the service index for source https://api.nuget.org/v3/index.json" is related to the Azure Identity SDK error, especially since setting the AZURE_TELEMETRY_DISABLED=1 doesn't work. The Azure.Core code-in-question is not run if that is set, which is true in all versions of Azure.Core.

@SuperSandro2000 I took a quick look and agree it seems likely that is also an issue. I would open an issue in their repository. dotnet restore uses nuget but IDK that it uses any Azure packages (although I could be wrong about that)

GGG-KILLER commented 1 month ago

For the .NET CLI I've opened the bug NuGet/Home#13531 and once this is fixed we'll need to update the .NET binaries to fix this.

Patching isn't possible outside of source builds and the source builds currently don't have the latest SDK version unless for .NET 9 which is still in preview.

purepani commented 4 weeks ago

Is it possible to modify nuget-to-nix to run inside the nix sandbox somehow? The sandbox does not expose /etc/os-release AFAIK so that should fix it.

SuperSandro2000 commented 4 weeks ago

No, it needs network access. We could hack around it with bwrap or libredirect but that maybe also breaks the next thing.

infinisil commented 3 weeks ago

This is now merged:

purepani commented 3 weeks ago

Is there a tag to add to this in order to put this on the task list for the 24.11 release? Given the temporary fix being a name change, and the slightly controversial discussion in #318511 it should probably come up for discussion again in the unlikely event that there's not a fix or a patch by the next release.