Skip to content

[WIP] untangling auth-related code #5925

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

Really (REALLY) work in progress; slowly untangling some of the auth-code which was wrapper-upon-wrapper-upon-wrapper; often because types like registry.IndexInfo or registry.RepositoryInfo were part of the signature.

Docker Content Trust added yet-another layer of abstraction on top of that, with trust.ImageRefAndAuth, which is a wrapper on its own to wrap all those bits.

In most cases, all we need is;

  • either the name of the registry, or an image-ref from which we can deduct the name
  • we DONT need to know about Mirrors, because the client doesn't configure those
  • for most situations we don't even need to know about "insecure registries", but we can deduct "defaults" there from the hostname (default is loopbacks are insecure, everything else isn't)

And of course, there's the "special cases" for docker hub;

  • docker.io or index.docker.io PREFIX means "docker hub registry" (actual registry is registry-1.docker.io (but there's other domains possible ⚠️ we still need to normalise those)
  • we currently use https://index.docker.io/v1/ as KEY to store credentials for those
  • ☝️ also something we should consider changing, because for other registries, we use hostname without scheme / path

But there's more to untangle, such as creds-helpers/stores converting "to hostname", but other paths don't, and likely corner-cases, where (e.g.) a trailing / is missing in https://index.docker.io/v1/, etc etc.

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

FWIW, more untangling also happening in #5876 and #5921

I should probably look at basing this one on that, but wanted to see things before that

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 25.45455% with 41 lines in your changes missing coverage. Please review.

Project coverage is 58.86%. Comparing base (4d8c241) to head (76452e7).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5925      +/-   ##
==========================================
- Coverage   58.89%   58.86%   -0.04%     
==========================================
  Files         358      358              
  Lines       29962    29979      +17     
==========================================
- Hits        17647    17646       -1     
- Misses      11334    11351      +17     
- Partials      981      982       +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thaJeztah thaJeztah force-pushed the simplify_auth branch 3 times, most recently from df692c7 to 8a4f9b4 Compare March 19, 2025 14:37
@thaJeztah thaJeztah force-pushed the simplify_auth branch 4 times, most recently from 074b5a7 to c784802 Compare March 25, 2025 12:25
@thaJeztah thaJeztah force-pushed the simplify_auth branch 3 times, most recently from a3f954e to 26845e4 Compare April 4, 2025 19:52
@thaJeztah thaJeztah force-pushed the simplify_auth branch 2 times, most recently from 59e6e83 to 199969d Compare April 11, 2025 14:12
Lots to do here; too many wrappers everywhere, which may become easier
when content trust is removed (which adds another level of abstraction)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
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.

2 participants