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

[codecov]

Codecov Report

Attention: Patch coverage is 67.56757% with 12 lines in your changes missing coverage. Please review.

Project coverage is 75.57%. Comparing base (eeb21fc) to head (83d5c64).

Files with missing lines	Patch %	Lines
...ote/credentials/internal/config/readonly_config.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #850      +/-   ##
==========================================
- Coverage   75.67%   75.57%   -0.11%     
==========================================
  Files          63       66       +3     
  Lines        6011     6035      +24     
==========================================
+ Hits         4549     4561      +12     
- Misses       1079     1091      +12     
  Partials      383      383              

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

[Wwwsylvia]

For type safety, maybe we should introduce a new interface ReadOnlyStore that only contains the Get method.

type ReadOnlyStore interface {
	// Get retrieves credentials from the store for the given server address.
	Get(ctx context.Context, serverAddress string) (auth.Credential, error)
}

We can also change the signature of Credential() from

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

Line 72 in eeb21fc

func Credential(store Store) auth.CredentialFunc {

to

func Credential(store ReadOnlyStore) auth.CredentialFunc {

@programmer04 Do you use the Credential() function in your code?
@shizhMSFT What do you think of adding a new interface?

[programmer04]

I think this is a great idea! I implemented this and tested it with my code - Kong/gateway-operator#940 (there is a usage of Credential()) and it works nicely.

Furthermore, I implemented all other suggestions. PTAL