Skip to content
This repository has been archived by the owner on Mar 23, 2019. It is now read-only.

Handle private registries which don't require auth #917

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Handle private registries which don't require auth #917

wants to merge 1 commit into from

Conversation

pilou-
Copy link
Contributor

@pilou- pilou- commented Apr 10, 2018

ISSUE TYPE
  • Bugfix Pull Request
SUMMARY

Allow to push to private registries: remove exception.

Error was:

$ ansible-container push --push-to myregistry
Parsing conductor CLI args.
Engine integration loaded. Preparing push.	engine=Docker™ daemon
Traceback (most recent call last):
  File "/usr/local/bin/conductor", line 11, in <module>
    load_entry_point('ansible-container', 'console_scripts', 'conductor')()
  File "/_ansible/container/__init__.py", line 19, in __wrapped__
    return fn(*args, **kwargs)
  File "/_ansible/container/cli.py", line 399, in conductor_commandline
    **params)
  File "/_ansible/container/__init__.py", line 19, in __wrapped__
    return fn(*args, **kwargs)
  File "/_ansible/container/core.py", line 959, in conductorcmd_push
    username, password = engine.login(username, password, email, url, config_path)
  File "/_ansible/container/__init__.py", line 19, in __wrapped__
    return fn(*args, **kwargs)
  File "/_ansible/container/docker/engine.py", line 1134, in login
    u'Please provide login credentials for registry {}.'.format(url))
container.exceptions.AnsibleContainerConductorException: Please provide login credentials for registry myregistry.test.
Conductor terminated. Cleaning up.	command_rc=1 conductor_id=6bf66bde1725100e0baf80112dba1112999012dc9c5d94788e363f6ec89d5313 save_container=False
ERROR	Conductor exited with status 1

Fixes #911

@hamza-tumturk
Copy link

This fix should be applied, as it does not make sense to add options --username="" --password="" to ansible-container if no authentication is required.

Error was:

    $ ansible-container push --push-to myregistry
    Parsing conductor CLI args.
    Engine integration loaded. Preparing push.	engine=Docker™ daemon
    Traceback (most recent call last):
      File "/usr/local/bin/conductor", line 11, in <module>
        load_entry_point('ansible-container', 'console_scripts', 'conductor')()
      File "/_ansible/container/__init__.py", line 19, in __wrapped__
        return fn(*args, **kwargs)
      File "/_ansible/container/cli.py", line 399, in conductor_commandline
        **params)
      File "/_ansible/container/__init__.py", line 19, in __wrapped__
        return fn(*args, **kwargs)
      File "/_ansible/container/core.py", line 959, in conductorcmd_push
        username, password = engine.login(username, password, email, url, config_path)
      File "/_ansible/container/__init__.py", line 19, in __wrapped__
        return fn(*args, **kwargs)
      File "/_ansible/container/docker/engine.py", line 1134, in login
        u'Please provide login credentials for registry {}.'.format(url))
    container.exceptions.AnsibleContainerConductorException: Please provide login credentials for registry myregistry.test.
    Conductor terminated. Cleaning up.	command_rc=1 conductor_id=6bf66bde1725100e0baf80112dba1112999012dc9c5d94788e363f6ec89d5313 save_container=False
    ERROR	Conductor exited with status 1

Closes #911
@pilou-
Copy link
Contributor Author

pilou- commented Jun 20, 2018

@Voronenko could you review this one too :) ?

@Voronenko
Copy link
Contributor

That's the edge case, @pilou- . Generally, I do not think it is good idea to have private registry without authorization from security point of view.

@pilou-
Copy link
Contributor Author

pilou- commented Jun 20, 2018

@gregdek could you merge this one ?

@gregdek gregdek requested a review from Voronenko June 20, 2018 17:39
@gregdek
Copy link
Contributor

gregdek commented Jun 20, 2018

Would like to hear more feedback from @Voronenko on this one -- are you saying that this PR is solving an edge case at the expense of breaking the private registry use case?

@pilou-
Copy link
Contributor Author

pilou- commented Jun 20, 2018

This PR doesn't break private registry use case.

Credentials (meaning: username and password) can still be provided:

Currently, ansible-container:

  1. always require credentials (due to the username check removed by this pull-request)
  2. on the other hand, private registry without authentication can be used when --username and --password command line switches are provided (regardless the values)

ansible-container should not require --username and --password to be provided when a registry without authentication is used.

@Voronenko
Copy link
Contributor

@gregdek Nope, it does not break. "Edge case" - wat I want to say, is that having private docker registry without authentication (or even filtered by IP) is the bad security practice. You will rarely meet such setup even in staging environment.

So I would see allowance procedure either as attribute near registry creds, indicating that, or perhaps specifying both username and password to empty strings and handle that ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pushing to a Private Repo that does not require login
4 participants