Skip to content
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

mirrors: Make targets_path and metadata_path optional #1153

Conversation

jku
Copy link
Member

@jku jku commented Sep 25, 2020

Now clients can leave out targets_path or metadata_path if the
client knows the mirror does not have that type of targets.

This is backwards compatible: old mirror configs continue to work.

Fixes #1079.

Signed-off-by: Jussi Kukkonen jkukkonen@vmware.com


PR notes:

This is a minor improvement in the warehouse case where one mirror has metadata and targets, and the other mirror has only targets.

Note that this leaves the issue of using os.path.join() on URLs as is: Fixing #1077 in a provably backwards compatible way seems tricky.

I believe this is a good if minor change... but I'll make the case against merging as well:

  • the usefulness in pip/warehouse is minimal: the directory confinement is not expressive enough to be useful so pip has to use two separate mirror configurations anyway -- this should mean that in normal operation no requests will be made to the 'wrong' mirror: I believe they would only happen if the metadata mirror does not respond to a request for e.g. a specific targets bin file. In that case the wrong mirror would be asked as well -- this would be an annoyance, not a security issue
  • it can be argued that the correct solution is to make directory confinement more powerful instead:
    • it should work on both metadata and targets
    • it should be expressive enough to say "allow everything under this directory"

Unfortunately improving directory confinement is likely to be a breaking change.

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Now clients can leave out targets_path or metadata_path if the
client knows the mirror does not have that type of targets.

This is backwards compatible: old mirror configs continue to work.

Fixes theupdateframework#1079

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku
Copy link
Member Author

jku commented Sep 25, 2020

In case it's relevant, these are the configs I keep currently switching in pip depending on what I'm downloading:

config 1 (for downloading index file targets from pypi.org):

        {
            'https://pypi.org': {
                'url_prefix': 'https://pypi.org',
                'metadata_path': 'tuf/',
                'targets_path': 'simple/',
                'confined_target_dirs': ['']
            }
        }

config 2 (for downloading distribution target files from pythonhosted.org):

        {
            'https://pypi.org': {
                'url_prefix': 'https://pypi.org',
                'metadata_path': 'tuf/',
                'targets_path': 'not_real_path/', # TODO should not exist
                'confined_target_dirs': []
            },
            'https://files.pythonhosted.org/packages': {
                'url_prefix': 'https://files.pythonhosted.org/packages',
                'metadata_path': 'not_real_path', # TODO should not exist
                'targets_path': '',
                'confined_target_dirs': ['']
            }
        }

@jku
Copy link
Member Author

jku commented Sep 25, 2020

directory confinement is not expressive enough to be useful so pip has to use two separate mirror configurations anyway

Actually, it's possible that even a very expressive directory confinement might not be able to handle the warehouse setup in a single config... I'd have to really think the solution through (but maybe it's not needed for this PR)

@MVrachev MVrachev mentioned this pull request Sep 28, 2020
3 tasks
Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аside from my comment LGTM.

Comment on lines +406 to +407
metadata_path = SCHEMA.Optional(RELPATH_SCHEMA),
targets_path = SCHEMA.Optional(RELPATH_SCHEMA),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this change a check_match() call will ensure that both metadata_path and targets_path are set. But following this change it's possible to pass an object conformant to MIRROR_SCHEMA with neither value set.

I'm not sure whether we should worry too much about this, it should be fairly obvious to a client if they misconfigure by not setting either value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, does not seem like a major issue to me either.

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@joshuagl joshuagl merged commit 1d9c6ac into theupdateframework:develop Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updater: mirrors configuration tweaks
3 participants