Allow credentials to be forwarded during HTTP requests to API server

[stouset]

Summary

We are trying to run both shlink-web-client and shlink behind an authenticating reverse proxy. Authentication state is indicated through a cookie. If the cookie isn't present, the client is redirected to an authentication page. If the cookie is present (and contains valid session credentials), the request is transparently sent to the backend.

With shlink-web-client, requests to the API server include the API key but are not configured to send other forms of browser credentials, so they are redirected to an authentication page (which obviously the web client has no idea how to handle). We need the cookie to be sent when making these cross-origin requests.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Access-Control-Allow-Credentials

This takes two parts. One, the API server needs to set the Access-Control-Allow-Credentials HTTP header to true and the Access-Control-Allow-Origin to explicitly include the Origin of the web client. This tells the client browser that it can send credentials like cookies for these CORS requests. Luckily, we can instruct our authenticating proxy to force these headers to be set. I don't think there's a convenient mechanism in place to have shlink send the appropriate Access-Control-Allow-Origin header at the moment as it would require the backend to know the origin of the client in advance.

The other half is something we can't fake on our end with our authenticating proxy, unfortunately, and it's instructing the JS HTTP client that it should send credentials along with requests. From a cursory look at the code, I believe this will require a change to shlink-js-sdk to set credentials: "include" when the fetch client is constructed. I believe this should be safe to roll out generally, but I understand if you want to have it be an option exposed in the web-client's server configuration UI.

Use case

Sending stored HTTP credentials along with web-client requests to the API server is necessary in order to allow shlink-web-client to successfully communicate with a shlink API server behind an authenticating proxy.

[acelaya]

I don't see why would you add extra authentication to the server. The API key should be enough.

Can you elaborate on what's the use case for this kind of set up?

[stouset]

The API key is not enough because the API key is exposed to and intrinsically visible to every user of the shlink-web-client. Any user of the web client can trivially save this API key and then replay it to a public-facing server even if they have left the company. Avoiding this would either require rotating the API key any time an authorized user of shlink-web-client leaves the company or it would require every single user being issued a personal API key. For a company with hundreds or thousands of users this is completely impractical, and it further would represent a very significant source of friction for nontechnical users who want to create shortcuts. Not to mention additional work and effort for onboarding and offboarding new users (particularly since there's no convenient way to associate internal employee usernames/user IDs to API keys).

We also simply do not wish to expose the API server directly to the internet. It is highly plausible that someone will create easily-guessable shortcuts where the destination URL is sensitive. Imagine: go/ipo => https://wiki.company/page/secret-jan-1-2020-ipo-plans. Shortcuts are resolvable by the API server without needing to provide an API key at all, and for our own peace of mind we would like users to have to have authenticated with our SSO provider before they can resolve them.

[stouset]

Also I want to reiterate that the change introduced by my PR is effectively a no-op unless a reverse proxy in front of the API server is explicitly configured to send both an Access-Control-Allow-Credentials: true header and an Access-Control-Allow-Origin header that directly includes the Origin of the client itself.

[acelaya]

Thanks for the clarifications.

My first thought is that you should ideally generate different API keys per user, but I get that is less convenient compared to using their corporate account which already exists.

One possibility would be automating the API key generation as part of the user creation, and have it linked to every user account without them even knowing, but Shlink does not currently provide a nice way to do this, as API keys are currently only handled via CLI. Additionally, you would need to add more complexity to the proxy serving shlink-web-client to somehow pass the right API key "down", and again, there's no bullet-proof mechanism to do that at the moment.

So with that in mind, I think the architecture you are putting in place does look like a nice approach to support SSO, and the changes needed in Shlink are very low-impact.

Shlink handles CORS through this middleware, where it sets Access-Control-Allow-Origin: *, the allowed headers based on what the browser requested and the allowed methods based on the router configuration.

I reckon Access-Control-Allow-Origin: * is too permissive for the browser to "want" to send credentials, even if Access-Control-Allow-Credentials: true was set in the server, but this is worth verifying.

In any case, both allowed origins and allow credentials could be customized via env vars or config options in Shlink. That should not be a problem.

As per your SDK PR, I think it's better not to hardcode credentials: 'include', even if it's theoretically a no-op for most cases. We can't be sure how is people using the SDK and I would prefer not to introduce an accidental breaking change.

Instead, it's probably better to add a third optional options argument to ShlinkApiClient constructor:

export type Options = {
  credentials?: 'omit' | 'same-origin' | 'include';
}

class ShlinkApiClient {
  public constructor(httpClient: HttpClient, serverInfo: ServerInfo, options: Options = {})
  {
    // [...]
    this.#options = options;
  }

  // [...]

  private toFetchParams({ ... }: ShlinkRequestOptions): [string, RequestOptions] {
    const credentials = this.#options.credentials;
    // [...]
  }
}

Then shlink-web-client can pass credentials: 'include' here https://github.com/shlinkio/shlink-web-client/blob/a7d6637/src/api/services/ShlinkApiClientBuilder.ts#L26

[acelaya]

BTW, there's work in progress for a new Shlink Dashboard that should better support cases like this. I'm planning to have the first unstable release ready soon, but it won't support SSO from the very beginning, so it won't be an option for you for now.

More detail on Shlink Dashboard here #338

[stouset]

My first thought is that you should ideally generate different API keys per user, but I get that is less convenient compared to using their corporate account which already exists.

If there were a convenient way to do this for users coming in through SSO and a convenient way to auto-configure the web client for each user with these keys, we would 100% prefer going with that approach.

I reckon Access-Control-Allow-Origin: * is too permissive for the browser to "want" to send credentials, even if Access-Control-Allow-Credentials: true was set in the server, but this is worth verifying.

This is correct, and I have verified it with recent Safari and Chrome, and it also matches what's asked for in the spec.

I think it's better not to hardcode credentials: 'include', even if it's theoretically a no-op for most cases.

This is entirely reasonable; I'll update my PR.

[acelaya]

I have just merged shlinkio/shlink-js-sdk#273 and #1511, so this should be considered done.

I'm willing to prioritize a new shlink-web-client release, as it's been a while and it would benefit from dependency updates and a couple new improvements that were already merged. However, I need to merge one more feature before doing that, so it may still take a couple days.

As per Shlink, I'm afraid that will have to wait a couple months until I can cut a new release, but considering you had the server part covered, I suppose that should be fine for now.

[stouset]

Thanks for being so responsive. We would love a reasonably-quick new release for the web client. It's great you're adding expanded CORS configurability directly into the shlink itself (and we'll definitely use it when it comes out!) but as you note that's something we can luckily work around with our proxy for now.