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

CP-35846: Restrict access to internal yum repo server (members only) #4672

Merged
merged 1 commit into from
Apr 8, 2022

Conversation

robhoes
Copy link
Member

@robhoes robhoes commented Apr 7, 2022

This restricts the /repository endpoint to local-root only. This
endpoint is exposed only on the coordinator, and used by other pool
members to access the pool's yum repo mirror.

Yum calls to this endpoint now require a pool secret to be used in a
cookie, which is implemented through a yum plugin.

Signed-off-by: Rob Hoes rob.hoes@citrix.com

for name in repos.repos:
repo = repos.repos[name]
if repo.getConfigOption('ptoken'):
repo.http_headers['cookie'] = "pool_secret=" + ptoken
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're sending the pool token along with every request we should probably forbid http:// URIs in our yum config where ptoken is used. i.e. you either use an internal mirror through http, or the one through ptoken but that must be https.

Copy link
Member Author

@robhoes robhoes Apr 7, 2022

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

And by the way, the code where this plugin is used doesn't use https directly in the yum config, but goes through stunnel to provide TLS and cert checking: https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/repository_helpers.ml#L1009.

@robhoes robhoes requested a review from minglumlu April 7, 2022 15:46

repos = conduit.getRepos()
for name in repos.repos:
repo = repos.repos[name]
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth to restrict the repo URL to "http://127.0.0.1:8000"?

Copy link
Member Author

Choose a reason for hiding this comment

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

How exactly? Something like if repo.getConfigOption('baseurl') != "http://127.0.0.1:8000": continue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to worry about the token containing anything that would need escaping when passed in an URL? Or would whoever constructs the URL take care of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It goes into a cookie header, not the URL. And it's a fixed format, defined by xapi, that doesn't have weird characters.

Copy link
Member

Choose a reason for hiding this comment

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

How exactly? Something like if repo.getConfigOption('baseurl') != "http://127.0.0.1:8000": continue?

Yes. Maybe we have to have some hard-coding in this plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've now added a restriction for the baseurl to localhost.

This restricts the /repository endpoint to local-root only. This
endpoint is exposed only on the coordinator, and used by other pool
members to access the pool's yum repo mirror.

Yum calls to this endpoint now require a pool secret to be used in a
cookie, which is implemented through a yum plugin.

Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
@robhoes robhoes merged commit 50ec142 into xapi-project:master Apr 8, 2022
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.

4 participants