libimage: search: add IdentityToken option for authentication #1328
Conversation
IdentityToken will be needed if we use the search command with authfile option. Signed-off-by: Toshiki Sonoda <sonoda.toshiki@fujitsu.com>
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sstosh, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@sstosh are you using |
@vrothberg |
| @@ -65,6 +65,9 @@ type SearchOptions struct { | |||
| // "username[:password]". Cannot be used in combination with | |||
| // Username/Password. | |||
| Credentials string | |||
| // IdentityToken is used to authenticate the user and get | |||
| // an access token for the registry. | |||
| IdentityToken string `json:"identitytoken,omitempty"` | |||
There was a problem hiding this comment.
Why is the JSON annotation there? I wouldn’t expect libimage structs to be passed across processes or RPCs.
| sys.DockerAuthConfig = authConf | ||
| // We should set the authConf unless a token was set. That's especially | ||
| // useful for Podman's remote API. | ||
| if options.IdentityToken != "" { |
There was a problem hiding this comment.
- Doesn’t this completely break uses of non-
IdentityTokencredentials? - I don’t understand what this condition is trying to express at all, and how it relates to the comment. If a token was set, we AFAICS need to set
authConfas well, as this code does — but the comment says it doesn’t. - I also don’t understand how, if at all, it relates to the remote API; AFAICS remote/local should make no difference to the general structure of this code.
- I think it would be more reliable for callers, and also allow a clearer expression of this code, if the code enforced that only one source of credentials were used.
numCredsSources := 0
if options.Username != "" { numCredsSources++; … }
if options.Credentials != "" { numCredsSources++; … }
if options.IdentityToken != "" { numCredsSources++; … }
if numCredsSources > 1 { /* error */ }
// possibly decide based on numCredsSources != 0There was a problem hiding this comment.
Doesn’t this completely break uses of non-IdentityToken credentials?
Thank you for your reply.
I verified that search with AuthFilePath option is failed when we use of non-IdentityToken credentials.
I'll fix it to use getDockerAuthConfig() such as Pull.
IdentityToken will be needed if we use the search command
with authfile option.
Signed-off-by: Toshiki Sonoda sonoda.toshiki@fujitsu.com