use designated domains in tests (RFC2606)

[thaJeztah]

• depends on rewrite tests to use sub-tests, improve error-handling in tests, and use t.Cleanup() #275

Update domains used in tests to used domains that are designated for this
purpose as described in RFC2606, section 3

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.72%. Comparing base (6b9df3e) to head (b737c5a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   55.28%   50.72%   -4.57%     
==========================================
  Files           9        8       -1     
  Lines         624      483     -141     
==========================================
- Hits          345      245     -100     
+ Misses        234      201      -33     
+ Partials       45       37       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thaJeztah]

oh! found some more; hold on a minute

hm.. also interested why CI fails; need to check

[crazy-max]

oh! found some more; hold on a minute

hm.. also interested why CI fails; need to check

Hum yes it seems path query is not taken into account anymore https://github.com/docker/docker-credential-helpers/actions/runs/5098464489/jobs/9165620051?pr=274#step:7:66

=== RUN   TestWinCredHelperStoreRetrieve
    wincred_windows_test.go:230: Error: expected username user-7, got username user-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar
    wincred_windows_test.go:233: Error: expected secret secret-7, got secret secret-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar

[crazy-max]

It should exact match properly though

docker-credential-helpers/wincred/wincred_windows.go

Line 110 in 11e6d37

return serverURL.String() == target.String()

[thaJeztah]

Yeah; want to give it a look indeed.

Also change these tests into subtests probably

[thaJeztah]

(only addressed the "found some more" - not looked at the failures yet)

[thaJeztah]

I rebased this PR on top of the other PR that adds subtests;

=== RUN   TestWinCredHelperStoreRetrieve/https://foobar.example.com:2376/some/other/path?foo=bar
    wincred_windows_test.go:274: Error: expected username user-7, got username user-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar
    wincred_windows_test.go:277: Error: expected secret secret-7, got secret secret-6 for URL: https://foobar.example.com:2376/some/other/path?foo=bar

The test that's failing uses the same domain and path as the test before it, but differs in query parameters

docker-credential-helpers/wincred/wincred_windows_test.go

Lines 195 to 196 in f09e79d

	{url: "https://foobar.docker.io:2376/some/other/path"},
	{url: "https://foobar.docker.io:2376/some/other/path?foo=bar"},

And the test is using a test-table, which adds the credentials as part of the table for each test, but does not delete the credentials after testing;

docker-credential-helpers/wincred/wincred_windows_test.go

Lines 213 to 235 in f09e79d

	for i, te := range tests {
	c := &credentials.Credentials{
	ServerURL: te.url,
	Username: fmt.Sprintf("user-%d", i),
	Secret: fmt.Sprintf("secret-%d", i),
	}
	if err := helper.Add(c); err != nil {
	t.Errorf("Error: failed to store secret for URL: %s: %s", te.url, err)
	continue
	}
	user, secret, err := helper.Get(te.url)
	if err != nil {
	t.Errorf("Error: failed to read secret for URL %q: %s", te.url, err)
	continue
	}
	if user != c.Username {
	t.Errorf("Error: expected username %s, got username %s for URL: %s", c.Username, user, te.url)
	}
	if secret != c.Secret {
	t.Errorf("Error: expected secret %s, got secret %s for URL: %s", c.Secret, secret, te.url)
	}
	}

I wonder if the issue is if

• wincred.Add() possibly update secrets if it already found an existing one for the given host / URL?
• and if it considers https://foobar.docker.io:2376/some/other/path to be the same as https://foobar.docker.io:2376/some/other/path?foo=bar
• ^^ and because of that, doesn't update the credentials

The .Add() is designed to effectively be an "Upsert" (create if not exists, otherwise update). For the macOS keychain helper, I noticed that there's an explicit Delete() at the start;

docker-credential-helpers/osxkeychain/osxkeychain_darwin.go

Lines 35 to 37 in f09e79d

	// Add adds new credentials to the keychain.
	func (h Osxkeychain) Add(creds *credentials.Credentials) error {
	_ = h.Delete(creds.ServerURL) // ignore errors as existing credential may not exist.

The wincred.Add() inmplementation does not have that;

docker-credential-helpers/wincred/wincred_windows.go

Lines 16 to 26 in f09e79d

	// Add adds new credentials to the windows credentials manager.
	func (h Wincred) Add(creds *credentials.Credentials) error {
	credsLabels := []byte(credentials.CredsLabel)
	g := winc.NewGenericCredential(creds.ServerURL)
	g.UserName = creds.Username
	g.CredentialBlob = []byte(creds.Secret)
	g.Persist = winc.PersistLocalMachine
	g.Attributes = []winc.CredentialAttribute{{Keyword: "label", Value: credsLabels}}
	return g.Write()
	}

Some of the other tests call a Delete() as part of the test, e.g. TestWinCredHelperRetrieveAliases;

docker-credential-helpers/wincred/wincred_windows_test.go

Line 128 in f09e79d

helper.Delete(te.storeURL)

So, I suspect what happens is that;

• the https://foobar.docker.io:2376/some/other/path test runs (succesfully), which adds credentials for that URL
• the https://foobar.docker.io:2376/some/other/path?foo=bar test runs; does an Add(), but the credentials store already has an entry for https://foobar.docker.io:2376/some/other/path, and doesn't actually update?
• question is: why does it fail NOW ? All I changed in this PR is the domain (https://foobar.docker.io:2376 -> https://foobar.example.com:2376), which should not matter for this test.

[thaJeztah]

Right, so skipping one of the tests makes CI pass, so it's indeed due to state left behind from the previous test in the test-table, and it looks like that's a bug somewhere (see my comment)

[crazy-max]

Can you try with foo.com instead of example.com?

[thaJeztah]

Can try when I'm back at my keyboard.

You're thinking it may be because docker.io sorts before example.com or something?

[crazy-max]

I was looking at the API for the Credential structure: https://learn.microsoft.com/en-us/windows/win32/api/wincred/ns-wincred-credentiala and go-winres conversion https://github.com/danieljoos/wincred/blob/c3ce29ad62ec1f6a3ef4a35c83b8d52346779c15/conversion.go#L74-L116 and smth looks off but not sure what atm. Was also comparing with wincred handling in Git https://github.com/git/git/blob/79bdd48716a4c455bdc8ffd91d57a18d5cd55baa/contrib/credential/wincred/git-credential-wincred.c#L157-L187 but need to take a closer look.