osxkeychain: list: return full server URIs

[akerouanton]

- What I did

Commit 4cdcdc2 changed the format of list output. Before that commit, the json keys were containing full URIs (scheme://host/path[:port]), but afterward, the keys were only containing the path component.

With this commit, the list operation now returns full URIs (fixing the regression), and also fixes the malformed URIs issue when a port is specified (introduced by 19ec1c3, and affecting >=v0.4.2,<v0.9.0).

- How to verify it

TestOSXKeychainHelper has been improved to check which ServerURLs are returned by list.

CI should be green.

- Description for the changelog

- osxkeychain: list: return full URIs (regression introduced in v0.9.0)
- osxkeychain: list: fix malformed URIs when a `ServerURL` is stored with a port specified (introduced in v0.4.2)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.28%. Comparing base (833d2c3) to head (d8e34f8).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #364      +/-   ##
==========================================
+ Coverage   50.15%   51.28%   +1.13%     
==========================================
  Files          13       13              
  Lines         650      661      +11     
==========================================
+ Hits          326      339      +13     
+ Misses        279      278       -1     
+ Partials       45       44       -1     

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

[thaJeztah]

Should we used net.JoinHostPort here, or was this on purpose to keep the old format?

If that's the case, perhaps we should have a comment here to describe that we did so on purpose.

[akerouanton]

Nope, that's an oversight. I'm not trying to preserve the old behavior because it seems buggy to me:

• You store a credential for https://registry.example.com:5000/v1/
• And you end up with https://registry.example.com/v1/:5000 in a subsequent list

Unless someone starts complaining because they're depending on that specific bug, I don't think we should preserve the old behavior. If that's the case, and we want to add an escape hatch, I think we can introduce an env var. Downside: integrations need to use client.NewShellProgramFunc to inherit the calling process' env vars, or NewShellProgramFuncWithEnv with that escape hatch specified.

[thaJeztah]

And you end up with https://registry.example.com/v1/:5000 in a subsequent list

And now looking at that example, I suspect some of that is related to the assumption that we store creds per host, but I suspect the handling of that lived (or still lives) elsewhere.

I.e., the credentials helper would not show anything other than registry.example.com or registry.example.com:5000, and where needed for the credentials-store used, make that into a proper URL (with scheme); https://registry.example.com:5000.

That obviously fails if data was not normalised? And possibly part of that may have been because the "special case" for https://index.docker.io/v1/

Also see;

• Inconsistent auth keys on macOS, Windows, Linux #256
• Protocol should not be stripped out of the registry URL on login cli#190

[thaJeztah]

Do we know if Path is always cleaned? Mostly because URL.String() doesn't do any cleaning up; https://go.dev/play/p/azv5LBXYlZo

package main

import (
	"fmt"
	"net/url"
)

func main() {
	u := url.URL{
		Scheme: "https",
		Host:   "::1:8080",
		Path:   "//////location/../../hello",
	}

	fmt.Println(u.String())
}

Will print https://::1:8080//////location/../../hello

[akerouanton]

Do we know if Path is always cleaned?

It's not, and never was. (I wrote a test checking if uncleaned paths are preserved, and tried to run it on this branch and on v0.8.2 -- it passes on both.)

I'm not sure if that's correct tbh, but at least this PR doesn't introduce yet another backward-incompatible change, so it should be okay from that standpoint.

[thaJeztah]

LGTM, thanks!