feat: introduce ReadOnlyFileStore

[programmer04]

What this PR does / why we need it:

Currently, struct Config that represents a docker configuration file can be populated only with the function

Load(configPath string) (*Config, error)

that expects to have a file available on the disk and read it. Sometimes, this is a redundant step and a security risk when file content is available in memory. Here is a real-world example

• Do not create temporary file when dealing with container registry credentials Kong/gateway-operator#521

Hence, to overcome the aforementioned problems, this PR introduces a new type of a store - ReadOnlyFileStore that can be populated from io.Reader and supports only Get(...) method according to the proposal from @Wwwsylvia described in the #850 (comment)

[Wwwsylvia]

Hi @programmer04, thank you for the PR!

From the original issue Kong/gateway-operator#521, it looks like your scenario involves reading plaintext credentials from regcred without needing to update or delete them. Is that correct?

If so, rather than modifying the original credentials file store implementation, we could introduce something like ReadOnlyFileStore, which allows creating a store from a reader and does not support write operations on the credentials.

To get unblocked, you can consider implementing the ReadOnlyFileStore type in your code base. Since the plan is to support read operations only on the auth entry, the implementation would be much more straightforward than the current implementation in oras-go (no need to worry about concurrent writes, accidental overwrites, etc.).

If we feel like this functionality is necessary to be in oras-go, we can open an issue to track the feature request. What do you think?

[programmer04]

Hey @Wwwsylvia! Thanks for the response

Yes, you're right. The problem I am trying to resolve is populating an oras credential store robustly directly from in-memory (io.Reader) a plain text representation of standard JSON Docker credentials. It can be read-only, it's sufficient, but it should be as good as FileStore in handling all of the cases, see the below.

Here in FileStore operation Get is implemented like that

oras-go/registry/remote/credentials/file_store.go

Lines 65 to 68 in 52b94a7

	// Get retrieves credentials from the store for the given server address.
	func (fs *FileStore) Get(_ context.Context, serverAddress string) (auth.Credential, error) {
	return fs.config.GetCredential(serverAddress)
	}

where

oras-go/registry/remote/credentials/internal/config/config.go

Lines 171 to 198 in 52b94a7

	// GetAuthConfig returns an auth.Credential for serverAddress.
	func (cfg *Config) GetCredential(serverAddress string) (auth.Credential, error) {
	cfg.rwLock.RLock()
	defer cfg.rwLock.RUnlock()
	authCfgBytes, ok := cfg.authsCache[serverAddress]
	if !ok {
	// NOTE: the auth key for the server address may have been stored with
	// a http/https prefix in legacy config files, e.g. "registry.example.com"
	// can be stored as "https://registry.example.com/".
	var matched bool
	for addr, auth := range cfg.authsCache {
	if toHostname(addr) == serverAddress {
	matched = true
	authCfgBytes = auth
	break
	}
	}
	if !matched {
	return auth.EmptyCredential, nil
	}
	}
	var authCfg AuthConfig
	if err := json.Unmarshal(authCfgBytes, &authCfg); err != nil {
	return auth.EmptyCredential, fmt.Errorf("failed to unmarshal auth field: %w: %v", ErrInvalidConfigFormat, err)
	}
	return authCfg.Credential()
	}

contains logic to make it work for all cases.

There is memoryStore, but I don't see a way how easily implement generating such a store from standard JSON Docker credentials than handles all of edge cases, etc. like FileStore with its underlying Config struct that is coupled with actual file by a path field.

The problem is that there is a path field in Config (this structure maps JSON Docker creds). It's hard to break that coupling, this is why I proposed a workaround in PR to avoid breaking anything. Maybe you have an idea of how to tackle it differently?

[Wwwsylvia]

The problem is that there is a path field in Config (this structure maps JSON Docker creds). It's hard to break that coupling, this is why I proposed a workaround in PR to avoid breaking anything. Maybe you have an idea of how to tackle it differently?

The file store and the internal Config struct was designed for reading/writing Docker config files. If read-only is sufficient, we can implement a new store like ReadOnlyFileStore that loads the config from a reader. To support such store, we also need to implement a new Config like ReadOnlyConfig. This way we won't add complexity to the existing implementation and can gain better performance in the read-only scenario.

It would look somewhat like:

type ReadOnlyConfig struct {
	Auths map[string]AuthConfig
}

func LoadFromReader(reader io.Reader) (*ReadOnlyConfig, error) {
	// TODO: read from the reader and decode the content into Auths
	return &ReadOnlyConfig{}, nil
}

func (c *ReadOnlyConfig) GetCredential(serverAddress string) (auth.Credential, error) {
	// TODO: get the credential from Auths with best effort
	return auth.Credential{}, nil
}

type ReadOnlyFileStore struct {
	*config.ReadOnlyConfig
}

func NewReadOnlyFileStoreFromReader(reader io.Reader) (*ReadOnlyFileStore, error) {
	cfg, err := config.LoadFromReader(reader)
	if err != nil {
		return nil, err
	}
	return &ReadOnlyFileStore{ReadOnlyConfig: cfg}, nil
}

func (fs *ReadOnlyFileStore) Get(serverAddress string) (auth.Credential, error) {
	return fs.ReadOnlyConfig.GetCredential(serverAddress)
}

func (fs *ReadOnlyFileStore) Put(serverAddress string, cred auth.Credential) error {
	return errors.New("cannot put credentials in read-only file store")
}

func (fs *ReadOnlyFileStore) Delete(serverAddress string) error {
	return errors.New("cannot delete credentials in read-only file store")
}

[programmer04]

Thanks for the detailed guidance @Wwwsylvia I've updated PR (code, title, description) according to the solution proposed by you in #850 (comment) I even tested it with my code Kong/gateway-operator#940. Please take a look

[Wwwsylvia]

nit: Should this be called LoadFromReader to be consistent with Load??

[Wwwsylvia]

Since the config is read-only and we don't need to write anything back, we can simply decode the content into auths for read. How about:

func LoadFromReader(reader io.Reader) (*ReadOnlyConfig, error) {
	cfg := &ReadOnlyConfig{}
	if err := json.NewDecoder(reader).Decode(&cfg.auths); err != nil {
		return nil, err
	}

	return cfg, nil
}

[programmer04]

Exactly, my reasoning is that since it's read-only, the whole parsing can be done during creation because entries won't change in the runtime so I can have a map of auth.Credential ready for use and validated see the test TestReadOnlyFileStore_Create_fromInvalidConfig. In GetCredential and later Get` there is only access to a map; see also the comment in #850 (comment) and please tell me what you think. I can adjust my code according to your guidance but I need to understand how to do it

[Wwwsylvia]

We can refactor this in another way using generics, so that it can be reused by both config and readonly config:
Something like:

func matchAuth[V any](auths map[string]V, serverAddress string) (V, bool) {
	if v, ok := auths[serverAddress]; ok {
		return v, true
	}
	for addr, v := range auths {
		if toHostname(addr) == serverAddress {
			return v, true
		}
	}
	var zero V
	return zero, false
}

[programmer04]

It's currently used by both stores. For readonly in LoadFromReader where I perform parsing all entries to auth.Credential because they won't change during the lifetime of such a config, it can be done here. There is info whether the whole config is valid and no computation is needed for getting a credential.

For file backedd config it's used in method GetCredential. They need to be parsed and looked up on each call because they may change.

I moved it to a dedicated file helpers.go for better visibility and adjusted some names

[Wwwsylvia]

nit: How about: "operation not allowed: the store is read-only"?

[Wwwsylvia]

Should we put this in a separate file for better readability?