sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.12k stars 1.29k forks source link

Technical plan: app account federation / "don't require creating an admin account" #47998

Closed slimsag closed 10 months ago

slimsag commented 1 year ago

App has some requirements / needs:

  1. We want to reduce the time it takes for someone to download it and see results / the value of Sourcegraph (our north star for now.)
  2. We want to gate certain features/functionality based on when users are "signed in" or not. On the Sourcegraph.com side we need to use this as an opportunity to gather people's info (e.g. email) so we can funnel them into a Cloud trial later if applicable.
  3. In the future, we want to add opt-in features which will require talking to Sourcegraph.com - for example:
    • Searching OSS code by querying sourcegraph.com/search API which has a large corpus of OSS code so you don't have to clone it all on your local machine.
    • Cody integration by making an API request to the Cody API on Sourcegraph.com which will be done via an API as it requires a ton of compute/GPU resources.

The above requirements/needs mean that:

  1. We would like to get rid of the "create an admin account" flow when App starts for the first time; it doesn't make sense for a desktop app and adds a needless hurdle to seeing search results on your private code. Many Sourcegraph features are written entirely around the idea of an account, so we still need the user to have an admin account they are using - just not know it, effectively. The app can still show them as signed in, it just happened automatically for them.
  2. We would like some way for users to optionally "sign in" to e.g. a Sourcegraph.com account, so we can capture their email addy and unlock other functionality.

Proposal / technical plan

  1. (backend) When App starts, we effectively auto-generate admin account credentials for you and store them somewhere (e.g. in the DB)
  2. (backend) If you access Sourcegraph from localhost:3080, we identify that the request came from the local machine and assume those credentials
  3. (backend) When we want to gate some functionality / have a user Connect their Sourcegraph.com account, we do NOT actually have any concept of federated accounts. Instead, the user produces an access token for their Sourcegraph.com account and we store that access token in the site config.
    • This access token can be used for making API requests to Sourcegraph.com in the future (cody, OSS search), for the Starship launch version it wouldn't really be used.
  4. (frontend) To make the experience of connecting a Sourcegraph.com account nice, we can redirect the user to Sourcegraph.com, have them sign in, automatically create an access token for them, and redirect them back to the app.
    • This page already exists thanks to @abeatrix - it was originally intended for the VS Code extension we just never used it. e.g. https://sourcegraph.com/user/settings/tokens/new/callback?requestFrom=$SOURCE
    • We would just need to add a new allowlisted URL $SOURCE which redirects to a localhost:3080/foo page where the frontend code could capture the new access token and store it in the site config. See https://github.com/sourcegraph/sourcegraph/pull/35339 for details.

Problem

Points 1 and 2 mean that the e.g. GraphQL API of Sourcegraph would effectively be an open API endpoint for requests coming from the local machine; is this a security risk, why / why not? Where do we store these credentials?

I'm not sure if we can solve that concern, we may have to keep the admin account creation if we cannot. Luckily, I think the "Connect your Sourcegraph.com account" functionality can be implemented independently.

slimsag commented 1 year ago

cc @keegancsmith @marekweb since I think you're either already working on this or thinking about working on things related to this.

cc @taylorsperry @mrnugget @ryphil since I think you're interested in this

slimsag commented 1 year ago

Here's the only way I can think of to resolve the "no admin account is unsafe" problem:

  1. The admin credentials are generated and stored somewhere (a file on disk would be fine possibly, if someone has FS access them they are already elevated in some sense)
  2. Automatic sign-in does NOT happen based on the origin of the request. localhost:3080 requests are denied generally.
  3. Instead, we implement automatically opening your browser #47995 and as part of that include the credentials in the URL. e.g. your browser opens to http://localhost:3080?app-sign-in-password=$PASSWORD
  4. The frontend recognizes ?app-sign-in-password and automatically issues a login request.

This would mean that generic requests to localhost:3080 are not authenticated. If you navigate to localhost:3080 on your own, you would not be authenticated automatically. We could also include the ?app-sign-in-password in the URL printed to the terminal.

The downside is this is more work and may not be achievable in the short time we have.

mrnugget commented 1 year ago

@slimsag why would we have to store the credentials somewhere? I assume we could also do what a lot of other potentially-hosted-somewhere software does and use a default password that we'll let the user know about. Grafana's Docker container, for example, has a default password and tells you how to change it later.

kopancek commented 1 year ago

Option 1

What @mrnugget suggests above seems like the path of least amount of work - we don't need to do anything, just provide a default password in the database and tell user how they can login and keep the login form intact. Does it really need to be frictionless to the point of not even giving credentials in an input?

Option 2

I think we could still use a default password for frictionless login, but let users change it to something else.

Option 3

If both of the above are no good, then at least store the password on the disk in an encrypted file instead of plaintext. Or at least hashed and salted.

Other remarks

I don't like providing the credentials in the URL from the security perspective. Browser history might be accessed by bad actors and the password would be leaked. We can point the browser to a new route, which will create a default session, store a cookie in the browser and redirect to sourcegraph home page.

The session/cookie can even be configured to only be valid for a limited amount of time, after which the user will be forced to create a federated account.

keegancsmith commented 1 year ago

We can generate the password and just store it in the DB like normal. That is persistant. If a user wants to change it then we need to ensure password change workflow works in app (which we need to do anyways).

I like avoiding the login screen, this is an app not a web application. I agree password in url is bad. We can generate a nonce each time app is started that is valid for one login. When the user clicks it we auto-login based on the nonce.

taylorsperry commented 1 year ago

I'd like to note that the most important thing is that this gets shipped for Starship, but it would be great if we could do this in a way that makes our lives easier when we introduce the option to sign in, upgrade to a Superdev license (will require some coordination with IAM), and transition to a Cloud trial (will require coordination with Cloud--can we make configuration persist?).

slimsag commented 1 year ago

@keegancsmith I'm going to start working on this; any tips? How are you thinking the nonce or DB side would work?

keegancsmith commented 1 year ago

Given we are all one process, I thought you could generate a random nonce in memory then print it out to the console with something like ?nonce=DEADBEEF. Then in our auth middleware if we get nonce, we do the magic to set cookies/etc which make you the admin account, unset the nonce (so only one req allowed). Does that make sense?

philipp-spiess commented 1 year ago

@slimsag From a UI perspective, do you have any special place in mind where you'd want the "Connect your Sourcegraph.com account" button to be surfaced? I can look into:

How does that sound?

philipp-spiess commented 1 year ago

We would also need a graphql endpoint to check if the account is already connected and ideally we can also get the username from sourcegraph.com so we can show something like Connected with your Sourcgraph.com account as philippspiess and an option to unlink

taylorsperry commented 1 year ago

From a UI perspective, do you have any special place in mind where you'd want the "Connect your Sourcegraph.com account" button to be surfaced?

I think we should have a "Sign in" button in the nav bar (similar to when you're unauth'd on dotcom). Post-Starship, we'll add some gates that allow additional functionality for signed-in users, so eventually we'll want sign-in prompts in all the Limited Access notices and, as you mentioned, probably some prompts in user settings. Unless the user settings dropdown should only be available if you're signed in (as it is on dotcom).

philipp-spiess commented 1 year ago

I think we should have a "Sign in" button in the nav bar (similar to when you're unauth'd on dotcom).

Makes sense but I think this can be a bit confusing (especially for starship). Please correct my mental model if I’m wrong, but:

Another thought that I just had is that, if we do not plan to use the dot-com account connected status at all for starship, does it even makes sense to implement the button for that? I would be super annoyed as a user if I see a CTA, decide to follow it, and then realize that I get nothing out of it (especially after having to sign up an account)


Unrelated to the specific discussion above, I also want to point out that any changes to dotcom that we will have to make for this (especially the new auth token flow) is going to be affected by the feature freeze and we need to land them by Monday.

slimsag commented 1 year ago

@taylorsperry @philipp-spiess this is part of why I've said we should stop calling this "sign in" - it's confusing and misleading for multiple reasons (because it's not a real account.) Let's call this "Connect to Sourcegraph.com" - then we do not have a conflict with any existing account terminology at all.

I agree we shouldn't have a useless "Connect to Sourcegraph.com" button anywhere; we should wait to merge it until we have at least one hard requirement for it. I think that e.g. restricting access to the batch changes page would be the first of such requirements.

That said, I'd like to see the access token for this end up in the site config @philipp-spiess - how hard do you think that would be to get working (in a PR, even if we don't merge yet)? For example, I'd like to be able to read the access token from the backend Go code - so that we can do things like federated search results in the future.

philipp-spiess commented 1 year ago

That said, I'd like to see the access token for this end up in the site config @philipp-spiess - how hard do you think that would be to get working (in a PR, even if we don't merge yet)? For example, I'd like to be able to read the access token from the backend Go code - so that we can do things like federated search results in the future.

Sounds good. Would you be fine with having this implemented on the front-end? We should be able to use the GraphQL APIs from the admin backend to change the site config? This doesn't sound super complex, but priority wise I will likely start with this after the other UI fixes that I'd like to get in by tomorrow.

slimsag commented 1 year ago

Would you be fine with having this implemented on the front-end? We should be able to use the GraphQL APIs from the admin backend to change the site config?

Yes, this seems like the right approach to me. 👍

This doesn't sound super complex, but priority wise I will likely start with this after the other UI fixes that I'd like to get in by tomorrow.

Sounds great to me 👍 If it lands post-code-freeze that's alright, since this is a stretch goal and we probably won't have a feature we want to gate in time anyway. Of course, it'd be nice to have before the code freeze in case we can squeeze such a thing in before the starship launch - so if we can make it happen let's do it. :)

taylorsperry commented 1 year ago

This all makes sense to me: great to knock out the logic on this, but let's wait to merge until "connecting to doctom" actually means something (whether that's gates that are in place or OSS integration). And I understand the rationale behind framing it as "connect to dotcom" instead of "sign in" we just need to think through the UX so that the meaning/value is clear to users.

taylorsperry commented 1 year ago

@rafax shared some thoughts with me via DM that I'd like to open up for discussion here:

Is there more context on why we want to use dotcom here? Have we considered any alternatives? I understand this is a Growth (i.e. not Cloud) decision, but I spent some time working on (and cleaning up issues with) sourcegraph.com and I don’t think using it for new functionality is a good idea. Dotcom doesn’t have any guarantees around reliability (no uptime SLA, poor historical uptime / stability since it is used as testing env), we explicitly removed all “private” data from there to avoid having to go through SOC2 for it (so, we likely should not add new private data) and the barebones “account” functionality makes it hard to integrate with DotCom from other systems (I imagine from App too) - that part of the reason why we were considering building a dedicates “Sourcegraph Accounts” service for One-click Signup project.

rafax commented 1 year ago

I guess this issue provides the context I asked about :)

If we delegate obtaining the API token to the user and don't use sourcegraph.com for anything "critical" (mostly gating features to "connected" users, which can likely tolerate some downtime) for the foreseeable future, my concerns are less important right now.

In the future, I think we should consider using a dedicated IdP (that would make the "connect to sourcegraph.com" flow easier, but also simplify App-to-Cloud conversions), and we should definitely involve @sourcegraph/security if we plan to pass customer private data through sourcegraph.com (Cody use case above).

slimsag commented 1 year ago

I'm pretty surprised to keep hearing what I perceive to be some sentiment that we will (or should) get rid of sourcegraph.com, if the intent is to do that then we should propose that and do that. But it feels pretty weird to me that we're in some in-between state where I keep hearing people say we shouldn't use it for new features, shouldn't rely on it at all, etc. Bizarre to me. Am I misreading/missing something?

AFAIK We also require all customers' admins have a Sourcegraph.com account already today (even if they are a Cloud customer) in order for them to manage their sales subscription with us / do stripe payments / etc once they sign, I assume that qualifies as private data as well..

rafax commented 1 year ago

@slimsag not sure if the "sentiment to get rid of sourcegraph.com" you describe is related to what I wrote above - can you confirm / deny?

Personally, I don't think we should get rid of sourcegraph.com as long as we believe that having an public index of OSS code brings us value that is higher than the cost of maintaining it, to the point where last year I suggested making it a Cloud instance to hopefully it feature-compatible with other instances / reduce operational cost / make it more reliable.

Regarding other aspects of using sourcegraph.com and

"shouldn't use it for new features, shouldn't rely on it at all"

I believe "we" (I wasn't involved in making the decision back in Q3 2022, but I support it) already decided that:

In my mind, this makes sourcegraph.com unsuitable for any new functionalities that process private data or would be covered by uptime / reliability SLO - if we need to build a "Cody API" or "federated Sourcegraph accounts" that we offer to paying customers and depend on from Cloud / On-Prem / App, I'd strongly suggest doing this as a dedicated, secure deployments (separate from DotCom) and that's what we do in Cloud (where our API / control plane will live in isolated projects). I realise certain functionalities (licence management, account management, pings, analytics collection, doc site) are either co-hosted with DotCom or implemented in DotCom mode, and I think we would be best off moving those to dedicated deployments / services and making DotCom "less special" from configuration POV.

if the intent is to do that then we should propose that and do that

I believe that part of the reason why this didn't happen is that sourcegraph.com function and ownership has changed many times in the last ~year, and I personally did not get involved as sourcegraph.com was explicitly placed outside of Cloud scope by @sqs.

AFAIK We also require all customers' admins have a Sourcegraph.com account already today (even if they are a Cloud customer) in order for them to manage their sales subscription with us / do stripe payments / etc once they sign, I assume that qualifies as private data as well..

I'm not aware of this from Cloud POV - as far as I know we don't use Stripe payments for any current customers, subscriptions/licences are handled by Sourcegraph employees and our customers / users don't need to have a sourcegraph.com account.

dcomas commented 1 year ago

We can approach this in multiple ways, a few options could be:

  1. Deploy new services in sourcegraph.com for convenience but at the cost of investing a lot of resources and time to make it resilient, secure and SOC 2 compliant if customer instances connect to it or rely on data from it.
  2. Deploy new services in new subdomains/clusters with fewer constrains, less resources needed and quicker to make the environment they are deployed resilient, secure and SOC 2 compliant.

@slimsag are you able to deploy your new services without touching sourcegraph.com?