-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
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. |
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.
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.
So far I find a file stored in subprojects to be the most convenient:
- does not depend on environment variables, thus cannot be easily hijacked by using environment variables
- has to be set once per project, as opposed to a
wrap
subcommands argument that will have to be set repeatedly when installing each wrap - may be checked-in to the version control system, if project maintainer chooses to, eliminating the need for other developers to set it manually
Cons:
- it adds steps to the usual flow of
setup
+compile
. Some build systems, that have wrappers around configuration scripts may need custom handling. Buildroot and Yocto come to mind, but these are bad examples, as both of them explicitly disable fallback and provide own mechanisms to deal with dependencies.
2553cb1
to
9020ee1
Compare
b283fb0
to
86b252b
Compare
69c69bf
to
68fa0a5
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 and an authority(net loc) with either the upstream wrapdb address or a user-defined one stored in the subprojects/wrap-sources.json file. The file may be checked into the project's version control system for persistent use. User is expected to use `meson wrap set-sources <wrapdb-url>` command to create the file. <wrapdb-url> may be any valid url that urllib can do urlopen() for: http, https, ftp, file protocols should work. Note that some of these are insecure. Address may contain a path prefix, wrapdb path component will be appended to it.
68fa0a5
to
15a93f3
Compare
Squashed commits |
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 and an authority(net loc) with either the upstream wrapdb address or a user-defined one stored in thesubprojects/wrap-sources.json
file. The file may be checked into the project's version control system for persistent use.The user should use
meson wrap set-sources <wrapdb-address>
command to create the file.Justification: