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

[release/9.0.1xx] AuthHandshakeMessageHandler: sync behavior with other clients. #43942

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 7, 2024

Backport of #43491 to release/9.0.1xx

Description / Customer Impact

Customers attempting to authenticate against OpenShift registries experience authentication failures because of an interesting spec quirk around the use of Identity Tokens. This PR introduces a fallback mechanism at a lower priority than our current scheme to try a quirked behavior for such registries. This logic is based on analysis of other widely-used containers tooling like Podman, Docker, and the regclient library for Go.

Regression

No - non-identity token logins work for such registries, this is a support gap.

Risk

Low - the fallback only happens if the identity token pathway is not valid for a particular domain. There is a scenario that looks from the code that we might regress, but in real world scenarios that pathway doesn't ever present itself.

Testing

A broad set of authentication mocks have been created based on test suites and registries that we support today that lets us cover the entire matrix of auth - this is a huge amount of additional testing that is a great foundation for future validation.

In addition, I tested the original version of this PR against a broad set of registries over at baronfel/sdk-container-demo and there were no regressions. I did not have an OpenShift registry to test against, but @tmds has done this.

Workaround

There is no workaround for users using our tooling exclusively, the main workaround would be to include external tooling like Docker and have the SDK push to those locations before using Docker et al to push to the final destination.

/cc @baronfel @tmds

tmds added 3 commits October 7, 2024 17:24
- 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.
@github-actions github-actions bot requested a review from a team as a code owner October 7, 2024 17:24
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Oct 7, 2024
@baronfel
Copy link
Member

baronfel commented Oct 9, 2024

@dotnet/source-build the sdk-source-build leg has failed repeatedly here when initializing the VMR - is there something we can to do to in this PR to fix this, or is there some other solution?

@MichaelSimons
Copy link
Member

MichaelSimons commented Oct 9, 2024

@dotnet/source-build the sdk-source-build leg has failed repeatedly here when initializing the VMR - is there something we can to do to in this PR to fix this, or is there some other solution?

There were conflicting changes made a few days ago. This has been addressed so a simple update from the target branch should address the issue. I updated the branch for you.

@baronfel
Copy link
Member

baronfel commented Oct 9, 2024

So simple :) thanks @MichaelSimons!

@baronfel
Copy link
Member

baronfel commented Oct 9, 2024

Good to go, pending a discussion about change scope and risk management for the backport.

@marcpopMSFT
Copy link
Member

@baronfel checking in on 9.0.1xx backport PRs to see if they are needed for 9.0.1xx GA and ready to go.

@baronfel
Copy link
Member

Yeah, this is good to go but we're nailing down a bit of scope. Give us another day?

@baronfel baronfel merged commit 1e14cc2 into release/9.0.1xx Oct 14, 2024
31 checks passed
@baronfel baronfel deleted the backport/pr-43491-to-release/9.0.1xx branch October 14, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants