Skip to content
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

api: Allow hackers to create dev API keys with CORS #985

Merged
merged 49 commits into from
Apr 11, 2022

Conversation

victorges
Copy link
Member

@victorges victorges commented Apr 2, 2022

What does this pull request do? Explain your changes. (required)

This creates a hopefully complete solution for all the CORS problems we have been having with hackers.
During hackathons, the hackers very frequently don't have the time nor the expertise to create backends
for their applications. In those cases, they frequently encounter issues when deploying their applications
for the first time to a hosted platform, which is something they normally do in the last minute of hackathons
and then render some broken demos and frustration on hackers and our side with the CORS FAQs.

This will also reduce the complexity for getting started with Livepeer, by not requiring hackers to run some
dummy proxies like this with in the end just expose
their API/accounts anyway...

I also aimed for an experience in the dashboard that will not only allow them to create API keys that are
enabled for CORS usage, but also provide some helpful/educational text so they can understand what they
are doing when they enable an API key for CORS access.

Finally, to reduce the impact of hackers enabling CORS on their keys and just forgetting about it until their
app goes live, I've also limited the power that an API key has by default if it has CORS enabled (but if they
(hopefully) know what they''re doing they can also create it with full access). This will make sure that the
API key won't have powers like listing all streams/assets that exist in the account nor deleting resources
they don't already know the ID for. I tried only allowing access to creation APIs and APIs that require the
caller to know at least one resource ID, like an asset or a stream (meaning they probably got that ID from
somewhere else like a stream that they just created or saved in cookies).

Made sure that all APIs called by the Video NFT SDK are also allowed for those restricted keys.

Specific updates (required)

Implementing this was a little complex since it require some structural changes in our authorization logic.
To allow changing the CORS allow list depending on the API key, we needed to parse and fetch the API key
before the CORS middleware. Also didn't want to move CORS all the way to our API handlers, so
the first step was to:

  • Split authorization middleware in 2: an authenticator (global), which parses/validates the credentials, and an
    authorizer, which checks if those credentials give access to the currently called API.

Apart from that refactor:

  • Created param in the API to give them CORS access to specified allowed origins;
  • Restricted access of those API keys by default, only allowing access to APIs that have allowCorsApiKey: true in the authorization parameters
  • Created another param in the API keys to give them full access even with CORS configured
  • Updated API key creation dialog in the dashboard to set the above params in API keys

Also fixed some other CORS issues in the API:

  • Remove the static list of allowed origins, which contained all of prod, staging and development values, and receive them as a CLI arg/env instead
  • Remove CORS middlewares from VOD. They were never used anyway since afaik the outer/global middleware would already return the CORS stuff in the response (at least in the complex OPTIONS requests). Even if that's not the case, it is not necessary anymore anyway.
  • Allow any origin to access specifically the /api/asset/upload/:url API, since that's one that we expect frontends to call with no need for any kind of authorization.

-

  • How did you test each of these updates (required)
  • Created API keys with some combination of allowed origins like localhost, localhost:3000, example.com etc
  • Called different APIs with those keys, checking that the restricted/full access works and also that the CORS headers in the response come OK

Kinda still need testing if OPTIONS requests will work as I hope. My concern here is that the browser won't include
the Authorization header in the request and then the server doesn't know what is the API key to know what origins
it can come from.

Does this pull request close any open issues?
Implements #986

Screenshots (optional):
Screen Shot 2022-04-04 at 18 39 28
Screen Shot 2022-04-04 at 18 40 12
Screen Shot 2022-04-04 at 18 40 43

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@vercel
Copy link

vercel bot commented Apr 2, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/livepeer/livepeer-com/Av7pG5JJBRKy8YFN4KUx5UHduURi
✅ Preview: https://livepeer-com-git-vg-fixcors-once-and-for-all-livepeer.vercel.app

[Deployment for 315a669 failed]

@victorges victorges force-pushed the vg/fix/cors-once-and-for-all branch from de2bac4 to 6217223 Compare April 3, 2022 16:05
@victorges victorges force-pushed the vg/fix/cors-once-and-for-all branch from e660ce8 to d66a44c Compare April 4, 2022 03:16
@victorges victorges force-pushed the vg/fix/cors-once-and-for-all branch from d66a44c to 756f319 Compare April 4, 2022 03:29
@victorges victorges force-pushed the vg/fix/cors-once-and-for-all branch 2 times, most recently from 43d6a98 to 5048cd1 Compare April 4, 2022 03:35
@victorges victorges changed the title vg/fix/cors once and for all api: Allow hackers to create dev API keys with CORS Apr 4, 2022
@victorges victorges marked this pull request as ready for review April 4, 2022 21:41
@victorges victorges requested a review from a team as a code owner April 4, 2022 21:41
Copy link
Member

@iameli iameli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through all the {allowCorsApiKey: true} instances to me and they seem reasonable. And I love the tooltip text. LGTM!

Still keep backward compatibility since the NFT SDK is already
calling the old one.
Made it a little shorter just with usage information
and a link to the getting an API key guide.
It is not true anymore!

Also remove the repeated beta badge that messes with the
header. We already have the note right below and it's not
**that** beta anymore lol
@victorges victorges merged commit 8ce86ad into master Apr 11, 2022
@victorges victorges deleted the vg/fix/cors-once-and-for-all branch April 11, 2022 23:02
@victorges
Copy link
Member Author

Hey @anarkrypto ! Just letting you know that we've just deployed these changes today! You should be able to use the /upload API now directly from your frontend :)

You can also test the new feature of creating CORS API keys, but it seems like you've got all that figured out with a proper backend already!

Let me know if you have any issues using any of those! Cheers

@anarkrypto
Copy link

anarkrypto commented Apr 13, 2022

Excellent news @victorges !

It is an honor for sea.tube to be one of the first to test this new feature.

I integrated in mvp.sea.tube and the direct-upload to the livepeer is working perfectly

However I was having some issues when transcoding or exporting to IPFS: The Task phase simply continues in "waiting" forever,

The issue does not appear to be CORS related, as it also occurs locally.

I'm using the api code in js from https://github.com/livepeer/video-nft

Here is our livepeer integration code, I don't see any issue with the code.
https://github.com/sea-tube/seatube/tree/main/src/pages/api
https://github.com/sea-tube/seatube/tree/main/src/services/livePeer

You can test locally or on https://mvp.sea.tube/home by clicking on the "add video" icon in the navbar.
The project is really a pre-alpha, therefore, for now, I don't have an interface just to test this functionality, but you can check the network logs in the console.

Can you verify this behavior ? If necessary I can open a new issue.

Thank you!

@victorges
Copy link
Member Author

@anarkrypto Thanks for trying it out. Also thanks for reporting the issue with the tasks, I've just fixed it! :)

Long story:
We indeed had problems in the NYC region which your backend seems to be using (probably just the closest one to where it is deployed). We have been handling a couple weird reliability issues this week, and in that process we had to basically reprovision all of our clusters. NYC was one of those, and after the reprovision it was left with a problem that affected only the VOD API. We don't have E2E tests for that part of our API yet so they weren't detected automatically either :/

I've at least create a specific alert for that error now, so if it happens again along these reprovisionings we will be aware of it ASAP. In the short to medium term we will also deploy some kind of E2E testing so we are also aware of any other issues that may come up.

Thanks again for reporting it! For any other issues that you find feel free to contact me directly not to use this PR. I think we're connected on email and I've got some other contact info here.

@anarkrypto
Copy link

anarkrypto commented Apr 13, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow direct cross-origin uploads
4 participants