Docs here: https://github.com/sourcegraph/docs/pull/561
This PR adds support for using Bitbucket Server OAuth2 application links
for sign-in and permission syncing.
When used for permission syncing, the user's oauth token is used to
fetch user permissions (and now permissions are fetched via the server).
## Test plan
Tests added and updated.
## Changelog
- Sourcegraph now supports Bitbucket Server OAuth2 application links for
user sign-in and permission syncing.
They should not be used outside of cmd/frontend, so making it a frontend
internal package.
While doing that, I realized that there is a coupling dependency between
authz providers and auth (which is authN) providers: GitLab code host
connections can do authz mapping via the usernames of another OIDC or
SAML auth provider
(https://sourcegraph.com/docs/admin/code_hosts/gitlab#administrator-sudo-level-access-token).
It turns out this feature does not work anymore, since at least several
releases, because we don't actually instantiate auth providers outside
of `cmd/frontend` and thus the mapping will never find anything (auth
providers don't explode when queried before init, unlike authz).
This only now became clear as I moved this code, and the dependency
graph was broken, so that's a nice property of these cleanups I guess 😬
Since it doesn't seem to work for quite some time, I opted for removing
it, and added a changelog entry about it. Not sure if that is
sufficient, I raised a thread here:
https://sourcegraph.slack.com/archives/C03K05FCRFH/p1721848436473209.
This would've prevented this change and needed more refactoring as
unfortunately we cannot map an auth provider by the conf type to a
record in the `user_external_accounts` table and need to actually
instantiate it.
Test plan: Compiler doesn't complain, tests still pass.
## Changelog
GitLab code host connections were [able to sync permissions by mapping
Sourcegraph users to GitLab users via the username property of an
external OIDC or SAML
provider](https://sourcegraph.com/docs/admin/code_hosts/gitlab#administrator-sudo-level-access-token)
that is shared across Sourcegraph and GitLab. This integration stopped
working a long time ago, and it has been removed in this release.
Previously, we would store authz providers globally and refresh them
every now and then.
However, creating the providers is fairly cheap (1.3ms in a local trace)
so we should not keep them in memory and remember to not forget to start
the watcher routine.
This will help for multi-tenant Sourcegraph in that providers are now
computed for the context in question, and not held globally. Keeping
potentially 100k authz providers in memory will not scale.
Test plan: Still works, local Jaeger traces are quite acceptable.
This is a register call that is easy to forget. When forgotten, all queries against the repo store will block forever.
In addition, this adds a hard-dependency on conf to every services startup, plus a busy loop. With multi-tenant, this will not work great because authz providers would be a global, and we instead want most things to be ephemeral so they're per-provider. This is a step toward that, but doesn't yet remove the providers global variable.
Good news, it turns out that we don't actually need to register the providers in every service! The reason they were required was to check if zero providers are configured, or if authzbypass mode is enabled.
Authz bypass mode is usually ON, except when there are problems with the authz providers, meaning some authz providers might not be able to sync permissions. Bypassing of permissions is only ever happening if there are ALSO zero providers configured.
So this is basically an optimization for the case where an instance has zero authz configured so that the SQL queries are a bit simpler. This also helps in tests because with bypass mode on and no providers configured, authz enforcement is effectively off in the repo store.
This makes it so that in tests we need to do slightly more work, but also makes for a more realistic test vs at runtime setup. Also, it's highly recommended to use mocks for DB wherever possible in more high-level components to keep tests fast.
To never have a scenario where we accidentally mess up here and enable bypass mode erroneously, this PR drops that entirely. Authz is always enforced, but when a code host connection is unrestricted (i.e., will not spawn a provider) the repos are still visible, so this should be no change over before.
## Test plan
The stack starts and works, and all CI tests are still passing. Code review should help as well.
The perforce permission syncer has been adapted to now read and save the HOST field from the perforce protections table, which contains the IP address(es) that the path rule that it applies to. It uses the updated sub_repository_rules store methods introduced in https://github.com/sourcegraph/sourcegraph/pull/63811/.
### Notes
- There is some existing logic in the permissions syncer that attempts to re-insert the existing sub_repo_permissions if we encounter a temporary (timeout, etc.) error when syncing. However, there is an edge case: what do we do if the existing permissions don't have an IP address associated with them yet (they were inserted before the updated permission syncer ran)? For simplicity, in this PR I leaned towawrds correctness - I fail the operation outright (I'd rather temporarirly lock someone out rather than accidentally leak information). I implemented a more robust straetgy for this in https://github.com/sourcegraph/sourcegraph/pull/64086.
## Test plan
- The existing unit tests have been adapted to use the new authz.SubRepoPermissionsWithIP structs (I use wildcard IP addresses).
- The big new test to pay attention to is TestScanIPPermissions (and the associated `sample-protects-ip.txt` file).
## Changelog
- The perforce permissions syncer has been updated to save the IP address associated with each sub_repository_permissions rule.
Closes https://linear.app/sourcegraph/issue/SRC-459/
Closes
This PR adds support for saving and retreiving the IP addressess
associated with each path rule in the sub_repo_permissions store.
It does this by:
**Adding a new permissions type to the internal/authz package**:
1be7df6d79/internal/authz/iface.go (L52-L96)
**Adding new `*WithIPs` versions of all the setter and getter methods**
The new setter methods uses the above `authz.SubRepoPermissionsWithIPs`
type that write to the appropriate `ips` column in the DB.
The new getter methods retrieve the ip addresses associated with each
path entry. However, here there is an additional complication: It's
possible for someone to call the `*WithIPs` getters when the ips column
is still NULL (indicating that the perforce syncer hasn't been updated /
ran in order to save the IP addresses from the protection table yet.
| repo_id | user_id | version | updated_at | paths | ips |
|---------|---------|---------|------------|-------|-----|
| 1 | 1 | 1 | 2023-07-01 10:00:00 | {`"/depot/main/..."`,
`"/depot/dev/..."`, `"-/depot/secret/..."`} | NULL |
| 2 | 1 | 1 | 2023-07-01 11:00:00 | {`"/depot/public/..."`,
`"-/depot/private/..."`} | NULL |
In order to address this, the getters each have a `backfill` boolean
that allows the caller to choose the behavior that they want.
- If `backfill = true`, the paths without IP entries will be returned
with a `*` (wildcard) IP indicating that any client IP address is okay.
(This is effectively the behavior we have today since we don't check IPs
for sub_repo_permisisons). I imagine this can be used when callers don't
care about enforcing IP-based permissions (such as when IP address
enforcement is disabled in site configuration).
- If `backfill = false`, if the IPs column is NULL - an error is
returned instead of backfilling ("The IP addresses associated with this
sub-repository-permissions entry have not been synced yet."). This
allows for callers that care about IP address enforcement to know
_explicitly_ if the IP address information hasn't been updated yet - so
we can't know whether or not the user is able to view the file (e.g when
IP based enforcement is enabled).
**Ensuring that the old setter methods set the IPs column to NULL**:
self-explanatory, if someone uses the non `*WithIP` variants of the
setters, we want to ensure that we zero out that column so that we don't
leave stale / inconsistent information for those Path entries.
---
Overall, the design this adds the new IP address functionality without
having to immediately update all the call sites in the codebase to force
them to interpret all this information (which would make for a
gargantuan PR). Eventually, we should be able to simply delete the old
versions of the setters/getters once the IP address functioanlity has
been threaded through everywhere.
## Test plan
Extensive unit tests.
For each new setter and getter, I added unit tests that tested along all
of the following dimenisons:
- **initial store state**: empty database, database seeded with
permissions with no IP information (paths column only), database seeded
with permissions that have the IP information synced
- **insertion method**: was the data for the test inserted **with IP
information** (using the `withIP` variant of upsert, etc.), or was it
inserted with the old legacy way with no ip information
- **retreieval method**: was the data reterived with the legacy getters
(that don't look at the IP information), with the new IP getters that
either backfill (if the IP information for that paths entry hasn't been
synced yet, it will return an `*` for that entry), or avoids backfilling
(will return the information in the IPs column, or hard-error)?
## Changelog
- The sub_repository_permissions_ database store can now save and
retrieve the IP addresses associated with each path rule.
Recently, this was refactored to also allow using the redispool.Store.
However, that makes it very implicit to know where something is being
written, so instead we pass down the pool instance at instantiation.
This also gives a slightly better overview of where redispool is
actually required.
Test plan: CI passes.
Historically, sourcegraph.com has been the only instance. It was
connected to GitHub.com and GitLab.com only.
Configuration should be as simple as possible, and we wanted everyone to
try it on any repo. So public repos were added on-demand when browsed
from these code hosts.
Since, dotcom is no longer the only instance, and this is a special case
that only exists for sourcegraph.com.
This causes a bunch of additional complexity and various extra code
paths that we don't test well enough today.
We want to make dotcom simpler to understand, so we've made the decision
to disable that feature, and instead we will maintain a list of
repositories that we have on the instance.
We already disallowed several repos half a year ago, by restricting size
of repos with few stars heavily.
This is basically just a continuation of that.
In the diff, you'll mostly find deletions. This PR does not do much
other than removing the code paths that were only enabled in dotcom mode
in the repo syncer, and then removes code that became unused as a result
of that.
## Test plan
Ran a dotcom mode instance locally, it did not behave differently than a
regular instance wrt. repo cloning.
We will need to verify during the rollout that we're not suddenly
hitting code paths that don't scale to the dotcom size.
## Changelog
Dotcom no longer clones repos on demand.
Sourcegraph.com only has public repos, so it is safe to allow access to
all repositories by all users.
This now-deleted code was originally added in
https://github.com/sourcegraph/sourcegraph/pull/26345 to support private
repositories on dotcom. We do not currently support private repositories
on dotcom anymore, and when we do add back that feature, we can figure
out how best to do it and will benefit from fewer special-cases for
dotcom.
## Test plan
CI
It seems many of our doc links for code hosts are broken in production
due to a url changed from external_services to code_hosts. I did a find
an replace to update all the ones I could find.
When authz is enabled for a GitHub code host connection, but there is no corresponding auth.providers entry, this code currently panics, because `GetOAuthContext` returns nil, and then the `RefreshFunc` in the OAuthBearerToken tries to use that context when it refreshes.
This is a misconfiguration, and cannot work (we need both the authz setting, and the auth provider), but the worker pod should not just panic because of that, so we return a nicer error here now.
Test plan:
Added a unit test.
This makes the dependency on conf clearer so that you understand that you must have a working connection to confserver when you're calling this function in the package.
Test plan:
Existing test suites
To bring Gerrit support more in line with other code hosts and because a customer recently ran into this limitation, this PR adds support for the SSH flag.
The diff is mostly straightforward, with two things to watch out for:
- The clone URL construction in makeRepo was wrong previously, it missed the `/a/`. This is only used for visuals, but still annoying. So I moved the whole construction into here from the gitserver cloneURL package.
- The SSH hostname and port are configurable in Gerrit, so to construct the right URL we need to fetch some instance metadata. This would be costly to do in the gitserver method, so we persist all the info needed to construct clone URLs "offline" during the cloning process by storing all the data for HTTP and SSH clones on the repo metadata column. This is mostly in line with other code hosts as well, see GitLab right above in the gitserver/cloneurl package.
Closes https://github.com/sourcegraph/sourcegraph/issues/60597
## Test plan
Added tests for the various stages, recreated recorded responses, and tried both HTTP and SSH cloning locally, both worked with out Gerrit instance.
I had a suspicion another test was failing due to racing with reading
dotcom.SourcegraphDotComMode and another test didn't unset it. It turned
out this wasn't the case, but I ended improving the API to avoid this
issue. Most call sites should be easier to read.
Test Plan: go test
This PR removes the global logic that computed the perms mapping. There's nothing that's heavy to compute in there, and there's a massive footgun in the case a service forgets to call the watcher.
This also reduces cross-cmd imports.
## Test plan
Existing tests do not fail.
Simple refactor, moved the methods into internal/dotcom so that we don't import across package boundaries.
Simply moved code, existing test suites will find issues.
Adds a new:
- gazelle generator
- rule + rule targets + catchall target
for generating go-mockgen mocks & testing for their being up-to-date.
Each go_mockgen macro invocation adds targets for generating mocks, copying to the source tree, as well as testing whether the current source tree mocks are up-to-date.
How to use this: `bazel run //dev:go_mockgen` for the catch-all, or `bazel run //some/target:generate_mocks` for an individual package, and `bazel test //some/target:generate_mocks_tests` to test for up-to-date-ness. There is no catch-all for testing
This currently uses a fork of go-mockgen, with an open PR for upstream here: https://github.com/derision-test/go-mockgen/pull/50.
Closes https://github.com/sourcegraph/sourcegraph/issues/60099
## Test plan
Extensive testing during development, including the following cases:
- Deleting a generated file and its entry in a go_library/go_test `srcs` attribute list and then re-running `sg bazel configure`
- Adding a non-existent output directory to mockgen.test.yaml and running the bash one-liner emitted to prepare the workspace for rerunning `sg bazel configure`
The existing config tests a lot of existing paths anyway (creating mocks for a 3rd party library's interface, entries for a given output file in >1 config file etc)
This PR migrates the ArchiveReader function's use of the exec endpoint to be in line with the rest of #59738
Additionally, sub-repo permission checks are now done on the server instead of in the client code.
---------
Co-authored-by: Erik Seliger <erikseliger@me.com>
We have a number of docs links in the product that point to the old doc site.
Method:
- Search the repo for `docs.sourcegraph.com`
- Exclude the `doc/` dir, all test fixtures, and `CHANGELOG.md`
- For each, replace `docs.sourcegraph.com` with `sourcegraph.com/docs`
- Navigate to the resulting URL ensuring it's not a dead link, updating the URL if necessary
Many of the URLs updated are just comments, but since I'm doing a manual audit of each URL anyways, I felt it was worth it to update these while I was at it.
This is a bigger change than I want, but the version of exp/slices we
used had a change from taking a Less func for SortFunc to a Compare. To
update a vulnerability we also bring in a new version of exp/slices, so
I seperated out this change.
All our uses of exp/slices are now covered by the stdlib, so I migrate
all our uses to it. In particular this mostly meant updating custom Less
functions to using the stdlib cmp.Compare.
The only functional change was a potential bug I found in
filter_file_contains.go. The comment for the sort says sort by Start
offset, but the code was incorrectly comparing the end offset for the
2nd argument. This makes sort's Less function break the expected
invariants, so the code was quite wrong. I expect in practice we didn't
notice it since most ranges did not overlap. This fix makes the compare
function respect the expected invariants and just seems correct.
Test Plan: CI and grep returns nothing for "golang.org/x/exp/slices" in
go files.
Currently, in the SubRepoPermsJob, we filter each search event in the stream serially. This is problematic because these checks require a database call, which means we're making a serial database call for each unique repo in our search results. We pay this cost whether or not the repo is a perforce repo because we're making the DB call specifically to check whether the repo is a perforce repo. This is currently adding ~2ms per unique repo in a set of search results when subrepo permissions are enabled (such as on S2).
We had various layers of caching of this information that I consolidated into a cache on the checker itself so the caller doesn't need to worry about cache logic and so the cache can be shared between dependents of a client. Whether a repo is a perforce repo is likely to be quite stable, so I set a default cache TTL of 1 hour. This can realistically probably be higher, but I'd rather err on the side of caution since this is a security feature. The cache itself is very dense since it uses roaring bitmaps. I am not concerned about memory usage given that we already have a much larger cache for the actual rules.
Since the cache lives on the client, it persists for the lifetime of the client. In most cases, this is the lifetime of the process since we use the global DefaultSubRepoPermsClient in most cases.
* First pass at moving moving logging linting into Bazel
* fixed negation operators
* Update dev/linters/logging/logging.go
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
* added more exceptions and refactored one or two impls
* added nogo lint pragmas to offending files
* ran configure
* reverted git-combine refactor
* ran configure
* reverted test as well
---------
Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
Now that we're running go 1.21, we can take advantage of its new features. This cleans up a few utilites that have since been included in the standard library.
* make the sorting more stable in this test
This is diffcult to catch in the act but here is one data point where
the sorting order isn't stable https://buildkite.com/sourcegraph/sourcegraph/builds/249376#018b89e8-b855-4663-939a-dc8c39847dfb
* bazel configure
* std lib slices only in 1.21
* gofmt
* use less instead of compare
* refactor to just use stablefunc
* cmp module is only 1.21
* fix ordering
This PR proposes a new pattern for instantiating gitserver clients.
When we instantiate a new gitserver client, we should pass in a scope, a description of the environment it's used in.
When a client is passed down to an environment, we can augment the client with an additional scope.
What is this for?
Looking at Grafana charts for dotcom, we see that we make about 2000 requests per second to gitserver. We know what endpoints we're hitting, and what _container_ is making the request.
In Sourcegraph, containers are not a great boundary for services though. Some components stretch across multiple containers, and one container runs many different components, for example our worker container.
While there are probably at least 50 routines owned by various different teams in that container, our current metrics only tell us that worker is making a large amount of requests.
But we don't know who to talk to about it, because, again, worker is basically every team.
With scopes, we get more fine-grained insights and can group the metric by (container, op, scope), to get full insight into what _component_ (not _container_) is talking to gitserver.
* log: remove use of description paramter in Scoped
* temporarily point to sglog branch
* bazel configure + gazelle
* remove additional use of description param
* use latest versions of zoekt,log,mountinfo
* go.mod
This exposes a proper API that we exactly know the surface of and know what to test for. If there are any issues with this endpoint, it will be very clear what that is, vs. a user-error calling p4exec with invalid arguments or so.
Also, this reduces the risk of accidentally exposing a p4 command that should not be exposed.
* Change the protects parsing to ignore hosts
Needs to be gated by code host or site config.
* Add a host config setting to control behavior
The setting is "authorization"/"ignoreRulesWithHost" - a boolean value
that defaults to `false`, which preserves current behavior.
When set to `true`, it turns off all processing of protects rules
that have anything other than the wildcard value in the Host field.
Added a test for that behavior in the protects parsing.
* Add CHANGELOG entry
* Adjust tests to accomodate skipping empty lines
* refactor: combine comment and blank line detection
* refactor: change variable/parameter name
`ignoreHostRules` to `ignoreRulesWithHost`
which matches the name of the config field and is a better variable name.
* add docs for "ignoreRulesWithHost"
Open questions:
Rate limiter keys never expire - problem?
Should we return an error when time goes backwards? There could be slight time drift across machines
GitHub hides all rate limiter errors as a rateLimitReached error
Document that git-rps are now enforced across all gitservers, and the configured value should no longer be divided by the number of shards.
The open questions still remain, but I want to test this on dotcom and S2 while we keep thinking about those.
* Fix sudo error for empty token header
If a header such as `Authorization: token` was passed, it would throw a
sudo error on DotCom. We now make sure that we have the correct schema
to ensure it's truly a sudo scheme.
* fix test name
* don't capture logs
Co-authored-by: Erik Seliger <erikseliger@me.com>
---------
Co-authored-by: Erik Seliger <erikseliger@me.com>
After recent changes, I don't think we will have the capacity to move this over the finish line and it doesn't seem to be a high priority. Let's close this out for now and revisit later if needed.
We'll have all the writeups we made and this diff for future inspiration.
