AuthHandshakeMessageHandler: sync behavior with other clients.

[tmds]

Fixes #43319

• Removes the special handling of the '<token>' username. This value is used as a sentinel from credential-helper scripts/programs. It is already handled by the docker-creds-provider .NET library.

• When there is no identity token, use the username/password with Basic auth.

[tmds]

@baronfel ptal. This is my understanding of the desired behavior after reading through some of the other clients code.

I am going to do some validation. I wanted to make a PR already to get early feedback.

I will include some tests when we settled on the behavior.

[tmds]

I've added a test that mocks the server behavior.

Compared to the main branch, there are two functional changes:

• There is no longer any special handling of the <token> username. What happens in https://github.com/mthalman/docker-creds-provider/blob/e4dd7658b589a4771b3ceb1c8b1b0c3de343f476/src/Valleysoft.DockerCredsProvider/NativeStore.cs#L60-L64 should be enough. <token> only has a special meaning when returned from a credential helper script/program.

• When we have a username/password for Bearer, previously those would be used with OAuth password_grant and then fall back to use Basic auth. This matches the behavior of regclient. I have swapped the order because I see both podman and docker do Basic auth.

[baronfel]

THANK YOU for the tests here - that's making it much easier to debug and reason about.

[baronfel]

I can't test this against baronfel/sdk-container-demo because this is targeted at main and the package produced has assembly load issues on a 9 SDK.

[tmds]

I can't test this against baronfel/sdk-container-demo because this is targeted at main and the package produced has assembly load issues on a 9 SDK.

I can target the 9.0 branch, if that helps for verification?

[baronfel]

That would be incredibly helpful. If testing passed we were going to try to backport the fix to 9 anyway.

[tmds]

The PR is now targeting the 9.0 branch.

[tmds]

I've changed the target branch back to main.

Sorry for code view requests, it is out of my control.

[baronfel]

The one test failure here is the crypto 'no more endpoints' known build error, not relevant to this PR.

[baronfel]

@tmds it looks like the full-framework test leg is consistently failing, can you take a look?

[tmds]

@baronfel I don't see anything other than the mentioned "There are no more endpoints available from the endpoint mapper." in the Azure CI. Do you see something different in some log/artifact?

I haven't tried to reproduce the issue by running the full-framework tests because I don't have a Windows machine.

[baronfel]

Oh, in that case that's fine. This error has been endemic recently :(

[tmds]

This error has been endemic recently :(

I skimmed through dotnet/dnceng#3844 and it may not be fixed soon.

Does it affect many tests besides RegistryTests.InsecureRegistry?

If you need to continuously manually filter these out, it may be worth either skipping tests (under a limited set of conditions), or catching the exception and allowing the test to pass at that point.

[tmds]

@baronfel is this good to merge? Or are we waiting for additional reviewers?

Once merged, can you initiate the backport to 9.0?

[baronfel]

Just awaiting review - we can trigger the backport for sure!

[baronfel]

@dotnet/product-construction had to rerun this PR due to the sdk-unified-build leg, I was told to ping you as this happens.

[baronfel]

/azp run sdk-unified-build

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[baronfel]

/backport to release/9.0.1xx

[github-actions]

Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/11220724421