Namespaced credentials key enhancements #1373
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 11, 2021
mtrmac
force-pushed
the
getcredentials-keys
branch
from
c514b7c to
3a3b7b7
Compare
|
@saschagrunert PTAL |
|
@giuseppe PTAL |
mtrmac
added a commit
to mtrmac/common
that referenced
this pull request
Sep 11, 2021
containers/image#1373 > go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@getcredentials-keys Signed-off-by: Miloslav Trmač <[email protected]>
This was referenced Sep 11, 2021
|
LGTM |
mtrmac
added a commit
to mtrmac/common
that referenced
this pull request
Sep 13, 2021
containers/image#1373 > go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@getcredentials-keys Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
getcredentials-keys
branch
from
3a3b7b7 to
2136228
Compare
|
@mtrmac any update on this? |
mtrmac
force-pushed
the
getcredentials-keys
branch
from
2136228 to
7631634
Compare
mtrmac
added a commit
to mtrmac/common
that referenced
this pull request
Oct 21, 2021
containers/image#1373 > go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@getcredentials-keys Signed-off-by: Miloslav Trmač <[email protected]>
|
Rebased; based on containers/podman#11538, I think this is ready for review/merge. |
mtrmac
force-pushed
the
getcredentials-keys
branch
from
7631634 to
5c3ba87
Compare
|
LGTM, But why is skopeo test failing? |
mtrmac
force-pushed
the
getcredentials-keys
branch
from
5c3ba87 to
38ad787
Compare
mtrmac
added a commit
to mtrmac/common
that referenced
this pull request
Nov 29, 2021
containers/image#1373 > go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@getcredentials-keys Signed-off-by: Miloslav Trmač <[email protected]>
The values are not that wide, and having more test cases fit on the screen makes it easier to understand how they relate together. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Use common code for that instead of open-coding the check. Signed-off-by: Miloslav Trmač <[email protected]>
Make the check fully table-driven now. Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Just turn the existing test case into an array, and re-create the test file from scratch in each iteration. Also ensure the temporary file is removed eventually. Signed-off-by: Miloslav Trmač <[email protected]>
We will add tests for key normalization, so build the infrastructure for this. Signed-off-by: Miloslav Trmač <[email protected]>
Also use require.NoError where semantically warranted. Signed-off-by: Miloslav Trmač <[email protected]>
Fully exercise the (lack of) treating docker.io/library specially. Signed-off-by: Miloslav Trmač <[email protected]>
... and not a namespaced key. Signed-off-by: Miloslav Trmač <[email protected]>
Just tell the compiler to compare a structure for equality instead
of prescribing how. (The parentheses around (types.DockerAuthConfig{})
seem to be a workaround for a bug in the language parser; otherwise the
inner value is a valid value.)
Also, just return in an if instead of if/continue/return at the end of
a loop
Signed-off-by: Miloslav Trmač <[email protected]>
This matches a string key in SetCredentials etc. For now, use it only for validation, and more accurate debug logging. For now, we accept invalid key values, because GetAllCredentials (incorrectly) depends on that. We'll fix that later. Signed-off-by: Miloslav Trmač <[email protected]>
…Named A reference.Named cannot represent a "docker.io/vendor" namespace in its _string format_; it can actually be created as a Go value, but such a value violates our expectations (converting to a string and back would result in "docker.io/library/vendor"). The "login" implementation needs to read the credentials for an arbitrary key, possibly including such a "docker.io/vendor" key; we plan to support that using the text GetCredentials function. So, build the underlying support for that by modifying the findAuthentication implementation to work with a string key. For now, GetCredentials still assumes the input is a registry, and does not work with namespaced keys. Also rename findAuthentication to findCredentialsInFile, continuing to eradicate the "Authentication" naming for credentials. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
As described previously, the login code needs to look up values scoped to docker.io/vendor, which is problematic to represent as a reference.Named; so, accept namespaced string keys. Also consolidate/update the API documentation. The tests have been only minimally updated for consistency, and will be consolidated later. Signed-off-by: Miloslav Trmač <[email protected]>
It's now only used to drive the file format to create. Signed-off-by: Miloslav Trmač <[email protected]>
Before this set of commits, GetAllCredentials was incorrectly passing namespaced values and URLs to GetCredentials, which was silently broken. Within this set of commits, GetCredentials started accepting namespaced values, but refusing URL keys; so, update the code to use the "keys" naming, and to normalize URL keys. This is a behavior change. Signed-off-by: Miloslav Trmač <[email protected]>
GetCredentials (and GetAuthentication) now validate the key, and reject URLs. So, update a test case that provided an invalid input that used to silently be accepted. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Don't involve a reference.Named now that it isn't required. Signed-off-by: Miloslav Trmač <[email protected]>
Now that GetAuthentication also accepts namespaced keys, we don't really need that boolean. Signed-off-by: Miloslav Trmač <[email protected]>
... to make it clearer what the test entry means. Signed-off-by: Miloslav Trmač <[email protected]>
We are already only using one of the values, so simplify the code. Signed-off-by: Miloslav Trmač <[email protected]>
This test was not possible to express when a reference.Named was involved; so add it only now. Signed-off-by: Miloslav Trmač <[email protected]>
Pedantically this avoids testing isNamespaced on invalid keys, where it is undefined; more importantly it allows making the tests more readable, as we add more tests, without a sea of misaligned true/false values. Signed-off-by: Miloslav Trmač <[email protected]>
For now, reject things that look like tags or digests, but still accept clearly invalid characters. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
getcredentials-keys
branch
from
38ad787 to
11809de
Compare
|
containers/podman#11538 and containers/common#763 pass; is this good to go? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Three major themes:
GetCredentials, as strings (as opposed toGetCredentialsForRef. This is necessary becausereference.Namedcan’t represent thedocker.io/vendorparent namespace, see Fix login/logout for docker.io/user common#757 . It also simplifies the c/common/pkg/auth caller, it won’t need two different code paths.GetAllCredentials, which was passing arbitrary JSON keys (URLs, namespaces) toGetCredentials. That sort of worked, but it doesn’t any more, with stricterGetCredentialsformat validation, and anyway it was doing the wrong thing if equivalent credential keys existed in multiple locations.GetCredentialsnow uses new-style normalized keys for registries; that could indirectly help Normalize auth key before callingSetAuthenticationpodman#11430 by changing what clients send, but note that that code should probably still be doing its own normalization to handle old Docker clients.See individual commit messages for details. Most of the churn is in extending, updating, and simplifying tests (and in the column with changes done by
gofmt; ignoring white space changes is recommended).Warning: Only unit-tested; it might be worth prototyping the full set of changes with containers/common#757 / a fixed containers/podman#11430 against the Podman test suite.