-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Introduce user-defined wrapdb mirror #13953
base: master
Are you sure you want to change the base?
Conversation
c704280
to
1182455
Compare
This consolidates all queries to the wrapdb to go through the open_wrapdburl() function. The function handles a domain-specific url scheme wrapdb. When encountered, it substitutes the scheme with https and an authority(netloc) with either upstream wrapdb address or a user-defined one from the MESON_WRAPDB_MIRROR environment variable.
1182455
to
840f286
Compare
This has the potential to be a really nasty way of hijacking user queries for malware injection. Not to mention all the other ways in which environment variables are terrible for storing persistent configuration. |
@jpakkane you mean adding more malware, once attacker already gained control over the users shell environment? Or getting the first handful of malware by user getting source code from the untrustworthy mirror? I'd be glad to hear for alternative more secure options. Providing the mirror address as a Getting code from untrusted sources IMO is a user problem. I honestly don't expect anyone to use random third party mirror found on the search engine. |
Ok, I can see this being abused on a build machine, as a stepping ladder of a more complex attack.What's worse - it will affect users that are not aware of this variable. |
8e6b1d6
to
babf222
Compare
Environment variables are prone to shell injection attacks, especially on build servers that many people may have access to. Changing address in a file requires an explicit command that is supposedly under the control of a trusted developer.
babf222
to
51a23d2
Compare
@jpakkane, I changed it to use an explicit user-provided address, unaffected by environment variables, limited to a single workspace. It looks tempting to store the address inside |
docs/markdown/Using-wraptool.md
Outdated
``` | ||
|
||
You will be limited to the wraps available on the mirror, only one source is used at a time. | ||
The address is stored in `subprojects/wrapdb-mirrors.json`, remove the file to use upstream address. |
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.
I am not sure I like the UI for this feature. I think that the use case you have in mind for it is very different from what I have in mind. Why isn't a command line argument to meson wrap
enough? If this is a functionality intended mostly for corporate environment where access to the "upstream" wrap database is somehow forbidden, doesn't a network level redirect provide a transparent solution for the same problem?
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.
Command-line argument will have to be set for each of the update
and install
sub-commands, and transparent fallback can not happen unless the address is stored somewhere.
May I propose an optional positional argument to the update-db
command that will create the "persistent" file?
Actually, you caught my mental confusion on what what it is for. I'm calling it a mirror, but it is not what I want it for - I want a different wrap-db-source
.
This is indeed for a corporate environment, but a simple redirect is not acceptable. All sources should be stored on the company-controlled machines, wraps and source updates have to be audited to an extent. Wraps for the subprojects developed by other teams, that have no interest in using meson yet are to be served from this source as well.
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.
This is indeed for a corporate environment, but a simple redirect is not acceptable. All sources should be stored on the company-controlled machines, wraps and source updates have to be audited to an extent. Wraps for the subprojects developed by other teams, that have no interest in using meson yet are to be served from this source as well.
I don't understand why a redirect is not acceptable. If that is your policy, most likely you have measures in place that forbid access to https://wrapdb.mesonbuild.com
. You just need to change those measures to redirect access to that resource to https://wrapdb.localnet
or whatever you like. Then you just serve your own wrap-db from that URL. Your wrap-db may contain your hand-picked selection of wraps. Why would this not work?
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.
May I propose an optional positional argument to the
update-db
command that will create the "persistent" file?
AFAIK running meson wrap update-db
is not required to use other meson wrap
commands. Therefore, making the configuration of the wrap database URL dependent on running it seems at least counterintuitive.
One thing I haven't understood yet is whether you want a setting that is per system, per user, per project, or per specific wrap. Depending on what is the use case the configuration would need to be stored in a different location.
I am also not convinced that there is the need of a meson command to set the WrapDB URL instead that a simple configuration file format.
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.
You just need to change those measures to redirect access to that resource to https://wrapdb.localnet or whatever you like. Then you just serve your own wrap-db from that URL. Your wrap-db may contain your hand-picked selection of wraps. Why would this not work?
- It will break TLS
- Let's say that policy is only enforced on the CI machines
- I have no control over this redirect
- meson is overly attached to this specific address hardcoded in a few places
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.
One thing I haven't understood yet is whether you want a setting that is per system, per user, per project, or per specific wrap. Depending on what is the use case the configuration would need to be stored in a different location.
Ideally setting wrapdb source per project with all of its subprojects. + A single instance of wrap server on the network.
AFAIK running meson wrap update-db is not required to use other meson wrap commands. Therefore, making the configuration of the wrap database URL dependent on running it seems at least counterintuitive.
Certainly, it will not require I see what you mean now: other commands do query release data directly from the server bypassing local wrapdb.json file. I don't like it.update-db
with the changes proposed in this MR. Only commands that query wrapdb need the update command whether with the source set or not. I don't think that one is supposed to have wrapdb.json
file checked-in, and without it status/info/search/list
won't work.
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.
May I propose an optional positional argument to the update-db command that will create the "persistent" file?
I did it, but I don't like it. Will revert to a separate wrap subcommand.
docs/markdown/Using-wraptool.md
Outdated
If for whatever reason you want to use self-hosted or proxied Wrap database *(since 1.X.X)* you may set mirror address to be used for the workspace: | ||
|
||
```console | ||
$ meson wrap set-db-mirror user:[email protected]:8080 |
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.
I really don't like that this does not allow to set a protocol. For now the wrap database speaks only https, but I can imagine a future where a different protocol is implemented.
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.
Like ftp, sftp, git? This will require parsing urls differently. An ssh/git user might want to pass a key somehow. Doable, but not sure if I want to open this can of worms yet.
Having a file:
or plain http:
protocol will be convenient for local testing, but not really useful otherwise.
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.
I don't understand what you mean with "opining this can of worms". You are making the location of the wrap database configurable. What I am asking is to do not assume that the protocol is https and make the protocol part of the URL part of the configuration string instead of hard-coding it to be https. Where are the worms in this request?
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.
Wish granted. There is enforcement of https
in the code with the assumption that the only other alternative is http
. I didn't get it right yet. WIP.
This changes moves responsibility of setting wrapdb source to the `wrap update-db` command. Improve wording.
Extend source address to accept other protocols. urllib can work with http(s), file, ftp protocols, so this sould work, but type annotations are not correct for the latter two. And due to enforcement of the https for secure connections protocol selection options are still limited.
2553cb1
to
9020ee1
Compare
This consolidates all queries to the wrapdb to go through the
open_wrapdburl()
function. The function handles a domain-specific URL scheme,wrapdb
. When encountered, it substitutes the scheme withhttps
and an authority(net loc) with either the upstream wrapdb address or a user-defined one stored in thesubprojects/wrap-mirrors.json
file.The user should use
meson wrap set-db-address <mirror-address>
command to create the file. It looks a bit more secure to me than using an environment variable.Justification: