-
Notifications
You must be signed in to change notification settings - Fork 102
feat: introduce credentials.NewMemoryStoreFromDockerConfig
#850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hi @programmer04, thank you for the PR! From the original issue Kong/kong-operator#521, it looks like your scenario involves reading plaintext credentials from If so, rather than modifying the original credentials file store implementation, we could introduce something like To get unblocked, you can consider implementing the If we feel like this functionality is necessary to be in |
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 ( Here in oras-go/registry/remote/credentials/file_store.go Lines 65 to 68 in 52b94a7
where oras-go/registry/remote/credentials/internal/config/config.go Lines 171 to 198 in 52b94a7
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 The problem is that there is a |
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 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")
} |
ReadOnlyFileStore
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/kong-operator#940. Please take a look |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
==========================================
+ Coverage 75.98% 76.05% +0.06%
==========================================
Files 63 63
Lines 6025 6042 +17
==========================================
+ Hits 4578 4595 +17
Misses 1066 1066
Partials 381 381 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ReadOnlyFileStore
credentials.ReadOnlyFileStore
credentials.ReadOnlyFileStore
credentials.NewMemoryStoreFromConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks, @Wwwsylvia for the great collaboration and review. It needs approval from @shizhMSFT too to merge it since he requested changes. And we're done |
credentials.NewMemoryStoreFromConfig
credentials.NewMemoryStoreFromDockerConfig
Signed-off-by: Jakub Warczarek <jakub.warczarek@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @shizhMSFT for the great collaboration and review. It seems I can't merge it, someone with write permissions has to approve CI run for it and click the merge button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@programmer04 Merged. 🎉Thanks a lot for your efforts and the contribution!! |
Thanks @Wwwsylvia! The last question is when do you release a new version? The last one is almost a year old, and looking at the |
@programmer04 I agree, I think we are pretty close to a new release. We will discuss it and hopefully we can make a release next week. |
Hi @programmer04 , just a heads up - We still have to finish all documentation work in the v2.6.0 milestone. We have set the target date of the release to the end of February. |
What this PR does / why we need it:
Currently, struct
Config
that represents a docker configuration file can be populated only with the functionLoad(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
Hence, to overcome the aforementioned problems, this PR introduces a new constructor -
NewMemoryStoreFromDockerConfig
that can populatememoryStore
from standard config in the form of[]byte
.The proposal from @Wwwsylvia and @shizhMSFT described in the #850 (comment)