hydra

hydra copied to clipboard

Small note at the beginning: the diff looks worse to review than it actually is I guess :) Most of the changes check if a user is authenticated or not and if not, the query will be modified to exclude private projects.

In contrast to hidden this is supposed to make sure that projects are actually private (hidden projects and its contents are still accessible if one knows which URLs to use). This means that:

• everything below /project, /jobset etc. will return a 403 if the project is marked as private.
• Channels, status overviews and search requests will hide anything from private projects if one is not authenticated.
• If Hydra is used as binary cache, it will return a 403 for store paths from builds inside private projects.

Note: I'm not so sure what's the right approach to prohibit requests to "private" channels and NAR URLs. The latter may not need any protection at all, if the hashes are unknown and the store is not entirely queriable. Since Hydra doesn't support basic Auth, the netrc-file from Nix won't be too useful either. I think that for now it may be sufficient to use nix.sshServe for a private binary cache instead unless we want to get rid of the fourth patch entirely :)

cc @grahamc @edolstra

• everything below /project, /jobset etc. will return a 403 if the project is marked as private.

This could allow enumeration. Which may or may not be an issue.

The solution is an annoying one.

Looking at GitLab:

• https://gitlab.com/samueldr/there-really-isnt-a-project-here
• https://gitlab.com/samueldr/this-project-is-private

When you are not logged-in, those two URLs redirect you to a Sign-in page.

When you are logged-in both shows basically the same 404. This ensures there is no information leakage, but is not really "RESTful" (for what it's worth).

I'm not saying either of the methods is better (403s vs. 404s), just showing an example of an implementation.

Seems reasonable and I'm happy to change this. Would love to get a few more responses about whether this makes sense and what to do to get this merged before investing more work here :)

This isn't a thorough review, though I am not sure NARs should be included in the auth: imagine a scenario where a public project builds a NAR and a private project build the same NAR. I'll take another look :)

Yeah, it's hard to apply access control to a Nix store, since you have to check that a path is in some closure resulting from a build in a project to which the user has access. That's a pretty expensive check.

Yeah, it's hard to apply access control to a Nix store, since you have to check that a path is in some closure resulting from a build in a project to which the user has access. That's a pretty expensive check.

@edolstra @grahamc right, I'm not too happy about this either. I temporarily protected by binary cache to only allow requests from my personal VPN.

Do you have an idea how to implement that? Or generally speaking, how to solve this problem? Or do you think that this is generally a bad idea (i.e. including the support for private projects)?

@grahamc Added some docs & implemented a yath testcase for it. The search is also covered by api-test.pl. As discussed in IRC last week, I reverted the binary cache check assuming that hashes are unguessable (also documented this behavior, of course).

If anything else is needed, let me know, but from my PoV this looks pretty fine now :)

@grahamc resolved the merge conflict and enhanced the testcase a bit more. Anything to add? Should be good to go otherwise from my PoV :)

Hmm weird, this test builds fine with make check, but doesn't with nix build. Also, I don't see a test failure in the output. @grahamc any chance you could help me here? :)

@grahamc rebased onto latest master.

Btw, I seriously don't understand this test failure, probably lack of yath knowledge. @grahamc @cole-h any chance you could help me there? :)

Here's the actual error message:

[P:22|F:0|R:1|T:5] Events: 1142 (23:private-projects.t)utf8 "\xE3" does not map to Unicode at /nix/store/9pxc1li1h8sqvagmvn8pmhglinzr2klw-perl5.32.0-Test2-Harness-1.000042/lib/perl5/site_perl/5.32.0/Test2/Harness/Collector/JobDir.pm line 408, <$fh> line 64.    malformed or illegal unicode character in string [F                                    
                                                   ], cannot convert to JSON at /nix/store/9pxc1li1h8sqvagmvn8pmhglinzr2klw-perl5.32.0-Test2-Harness-1.000042/lib/perl5/site_perl/5.32.0/Test2/Harness/Util/JSON.pm line 55, <$fh> line 64.                          

It appears to only occur in nix-build -A checks.x86_64-linux.build.

Yeah, the thing is I can occasionally reproduce it with yath test only and I'm not sure what's causing this :/

@grahamc @cole-h rebased now and removed the scmdiff testcase which caused weird problems when running nix build. Any chance to get another review? :)

@grahamc do you think this has a chance to get merged? If yes, I'd be happy to resolve the merge conflicts :)

@grahamc rebased onto latest master, building worked fine. Would like to test this on my own instance first, but then it should be in a mergeable state again! :)

@grahamc @samueldr is there any interest currently to resume with this? %)

(I don't have a stake in Hydra, I only noticed and evaluated an implementation hazard

I discussed this with @lheckemann last week: we should drop checkProjectVisibleForGuest (if possible everywhere), currently this in the first sub of a controller chain in most controllers since the implementation has the following problems:

• it is easy to forget in a new controller
• it gives a 404, but always the same error, i.e. Project not found whereas an empty query result would give you "$EntityName not found", so you can still distinguish private projects from non-existing entities.

Instead the queries in these actions should be written in a way to take private projects (optionally) into account. We may want to write wrappers for the most common entities somewhere for that.

However, these wrappers shouldn't be used unconditionally (just noting that here because we discussed that), but only where needed to avoid having unnecessarily complex DB queries.