Webhook URL #752

darkdarkdragon · 2019-02-26T13:04:24Z

What does this pull request do? Explain your changes. (required)
Webhook URL option - allows authenticate incoming streams through call to webhook

Specific updates (required)
Adds webhookUrl option to Livepeer node - this URL get called upon receiving RTMP connection and manifest id being passed to it. If this request returns non 200 response, then RTMP connection being denied.

How did you test each of these updates (required)

Does this pull request close any open issues?
Fixes #703

Checklist:

README and other documentation updated
Node runs in OSX and devenv
All tests in ./test.sh pass

darkdarkdragon · 2019-02-26T13:04:54Z

@j0sh If this looks ok, then I will (at least try) add tests for it.

darkdarkdragon · 2019-02-26T14:26:50Z

added tests

j0sh · 2019-02-26T19:13:12Z

cmd/livepeer/livepeer.go

@@ -102,6 +102,7 @@ func main() {
 	faceValue := flag.Float64("faceValue", 0, "The faceValue to expect in PM tickets, denominated in ETH (e.g. 0.3)")
 	winProb := flag.Float64("winProb", 0, "The win probability to expect in PM tickets, as a percent float between 0 and 100 (e.g. 5.3)")
 	maxSessions := flag.Int("maxSessions", 10, "Orchestrator only. Maximum number of concurrent transcoding sessions")
+	webhookURL := flag.String("webhookUrl", "", "Authentication webhook URL")


Maybe the description could be "RTMP authentication webhook URL". Should we also call it something like authUrl so we don't have to rename things if/when we add more hooks later on?

j0sh · 2019-02-26T19:15:02Z

cmd/livepeer/livepeer.go

@@ -379,6 +380,19 @@ func main() {
 		// TODO provide an option to disable this?
 		*rtmpAddr = defaultAddr(*rtmpAddr, "127.0.0.1", RtmpPort)
 		*httpAddr = defaultAddr(*httpAddr, "127.0.0.1", RpcPort)
+		if *webhookURL != "" {


Perhaps factor this out into a getWebhookURL helper? Eg, server.WebhookURL = getWebhookURL(*webhookURL)

j0sh · 2019-02-26T19:33:35Z

server/mediaserver.go

+		}
+		s.connectionLock.RUnlock()
+
+		if !authenticateStream(string(mid)) {


Let's authenticate first thing, before acquiring the lock.

I'm thinking we should pass in the full URL rather than the ManifestID. That way the auth endpoint can inspect the entire host + path, and enforce their own rules about path construction.

The thought was to have the webhook URL return the manifest ID. We should also structure the response to leave room for further customization, eg transcoding options for the stream.

GET https://host/webhook/endpoint Request Body: { "url" : <url> } Response Body: { "manifestID" : <manifestID> }

We could also do a POST here since a GET with a body is a bit unusual.

func createRTMPStreamIDHandler(s *LivepeerServer) func(url *url.URL) (strmID string) { return func(url *url.URL) (strmID string) { // Create a ManifestID // If an auth webhook is defined, get the mid from the webhook // Else if manifestID is passed in, use that one // Else create one var mid string if WebhookURL != "" { mid, err = authenticateStream(url) if err != nil { return "" } } else { mid = parseManifestID(...) } ```

Let's authenticate first thing, before acquiring the lock.

Previous check will allow us to quit early in case of same Manifest ids, so why not?

The thought was to have the webhook URL return the manifest ID

I really don't remember discussing such thing. Can you point me to such discussion?

We should also structure the response to leave room for further customization,

This is starting to be something other from what we're discussed.

Previous check will allow us to quit early in case of same Manifest ids

If the manifest ID is defined by the webhook then the webhook needs to come before. Also avoids acquiring the lock which also helps.

I really don't remember discussing such thing. Can you point me to such discussion?

Yeah, we had a lot of discussions around this and some details may have been lost. This was mentioned here https://hackmd.io/n0Lq1l6_SJCe0VnVHf4G-A#Webhook-Flow which was outlined as part of the discussion here :
https://docs.google.com/a/livepeer.org/document/d/1ThTyFeTjTfnWWj-HeNCMnkglD7Vf-9YvA71fB9dKVQs/edit?disco=AAAACet6oNM

This is starting to be something other from what we're discussed.

How so? The end result is the same, we aren't changing the scope of the work here. But tweaking the webhook format makes the construct much more versatile.

Once we merge this, it effectively becomes part of the public API, and it's harder to make breaking changes (even internally), so it helps a lot to leave a little room for expansion.

https://hackmd.io/n0Lq1l6_SJCe0VnVHf4G-A#Webhook-Flow

missed this one

was outlined as part of the discussion here :
https://docs.google.com/a/livepeer.org/document/d/1ThTyFeTjTfnWWj-HeNCMnkglD7Vf-9YvA71fB9dKVQs/edit?disco=AAAACet6oNM

Here is no mention of webhook defining manifest ids.

How so? The end result is the same, we aren't changing the scope of the work here.

Webhook's server should be more complicated now, plus our logic gets more complicated - what to do if webhook returned manifest id that we already have? Or if it is invalid manifest id?

Here is no mention of webhook defining manifest ids.

There seems to have been quite a bit lost "in translation" when migrating from the old doc to the new one.

Alternative Implementation - Webhook:
Start up node with a -streamAuthWebhook http://host/streamauth
RTMP stream comes in
Node calls the streamAuthWebhook with the RTMP URL, and other parameters?
Node receives manifest ID in response (disconnects incoming rtmp stream if webhook response indicates an invalid stream)
Stream is available for playback via the manifest ID

The behavior spec in the old doc also details how it would be desirable to have the input URL to be decoupled from the output URL to prevent input hijacking or guessing playback URLs given an input. Having the webhook return the manifest ID enables that.

Do you think that is not useful?

Webhook's server should be more complicated now

Reading a JSON request and replying with JSON is standard, as far as APIs go.

plus our logic gets more complicated

Our logic stays the same. The only thing that changes is the webhook gets called first to set the manifest id, if necessary.

A webhook is a large feature as far as API surface area goes, and we want to extract as much utility from it as possible. Right now its utility is limited.

I've added POST API, with only one addition - if webhook returns 200 with empty body - it is enough to pass authentication. This will simplify creation of simple webhooks.

j0sh · 2019-02-27T16:52:58Z

cmd/livepeer/livepeer.go

+		return "", true
+	}
+	glog.Infof("Using webhook url %s", u)
+	return "", false


return u ?

j0sh · 2019-02-27T16:54:08Z

cmd/livepeer/livepeer.go

@@ -499,6 +505,23 @@ func main() {
 	}
 }

+func getAuthWebhookURL(u string) (string, bool) {


Perhaps returning the error here rather than a bool would be more informative

j0sh · 2019-02-27T16:57:16Z

cmd/livepeer/livepeer.go

-
+		var err bool
+		if server.AuthWebhookURL, err = getAuthWebhookURL(*authWebhookURL); err {
+			return


The logging + exit can be done once here by returning an error:

if server.AuthWebhookURL, err = getAuthWebookURL(*authWebhookURL); err != nil { glog.Fatal("Error setting auth webhook URL ", err) }

I've been trying to use glog.Fatal around livepeer.go to log and exit.

Then maybe some test cases can be added in the new script checking cli args - https://github.com/livepeer/go-livepeer/blob/master/test_args.sh

j0sh · 2019-02-27T17:56:39Z

server/mediaserver.go

+	if AuthWebhookURL == "" {
+		return core.ManifestID(mid), true
+	}
+	payload := "{\"manifestID\":\"" + mid + "\",\"url\":\"" + url + "\"}"


I think you can use backticks to avoid escaping, like `{"url":"`+url.String()+`"}`. Using fmt.Sprintf might also help readability around concatenation.

Don't think passing in the manifestID is necessary since any query strings can be extracted from the URL itself.

Originally it was done to pass randomly generated manifest id, but you're right, there is no point in that. Fixed.

j0sh · 2019-02-27T17:58:49Z

server/mediaserver.go


+func authenticateStream(url, mid string) (core.ManifestID, bool) {


Returning an error rather than a bool would help upstream logging and avoid the need for logs within this function

j0sh · 2019-02-27T18:10:39Z

server/mediaserver.go

@@ -154,19 +162,59 @@ func createRTMPStreamIDHandler(s *LivepeerServer) func(url *url.URL) (strmID str
 			mid = core.RandomManifestID()
 		}

+		var auth bool
+		if mid, auth = authenticateStream(url.String(), string(mid)); !auth {


I think the flow clearer if we called authenticateStream first, before the manifestID checks. This also avoids having to pass in the manifest ID itself, which makes authenticateStream easier to understand (single responsibility). Also, returning an error rather than bool is more helpful.

mid, err := authenticateStream(url) if err != nil { glog.Error("Could not authenticate stream: ", err) return "" } if mid == "" { mid = parseManifestID(url.Query().Get("manifestID")) } if mid == "" { mid = core.RandomManifestID() }

j0sh · 2019-02-27T18:12:06Z

server/mediaserver.go

 		// Ensure there's no concurrent StreamID with the same name
 		s.connectionLock.RLock()
-		defer s.connectionLock.RUnlock()


We can probably leave the defer here since subsequent operations are not expensive. Less error prone

j0sh · 2019-02-27T18:15:33Z

server/mediaserver_test.go

+	mux.HandleFunc("/auth/invalidjson", func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte("{manifestID\":}"))
+	})
+	go srv.ListenAndServe()


Spinning up a live HTTP server for testing can be unpredictable / problematic so it's best to avoid it if possible. It's not always possible, but here I think we can avoid it here we follow the format that's used in https://github.com/livepeer/go-livepeer/blob/90c10193860999497cef5f1031fec4890bb9f19e/server/handlers_test.go

j0sh · 2019-02-27T18:16:39Z

cmd/livepeer/livepeer.go

@@ -106,6 +106,9 @@ func main() {
 	gsBucket := flag.String("gsbucket", "", "Google storage bucket")
 	gsKey := flag.String("gskey", "", "Google Storage private key file name (in json format)")

+	// API
+	authWebhookURL := flag.String("authWebhookUrl", "", "Authentication webhook URL")


Can the message mention that it's a "RTMP authentication webhook URL" ? That makes it clearer what it's used for.

j0sh · 2019-02-27T18:21:27Z

cmd/livepeer/livepeer.go

+	}
+	if p.Scheme != "http" && p.Scheme != "https" {
+		glog.Error("Webhook URL should be HTTP or HTTP")
+		return "", true


return "", fmt.Errorf("Webhook URL should be HTTP or HTTPS") then we can avoid the logging.

…l to webhook

darkdarkdragon · 2019-02-27T21:42:09Z

@j0sh Looks like I've addressed all the comments

j0sh

Tested with a local auth server and works nicely!! Just one style nit, otherwise LGTM 🚢

j0sh · 2019-02-27T21:52:58Z

server/mediaserver.go

-		mid := parseManifestID(url.Query().Get("manifestID"))
+		var mid core.ManifestID
+		var err error
+		if mid, err = authenticateStream(url.String()); err != nil {


Just a tiny style nit, but we don't need to predeclare the vars here (and would save one line overall):

mid, err := authenticateStream(url.String()) if err != nil {

darkdarkdragon requested a review from j0sh February 26, 2019 13:04

darkdarkdragon force-pushed the it/webhook branch from 82e9170 to efa0681 Compare February 26, 2019 14:24

darkdarkdragon force-pushed the it/webhook branch from efa0681 to 9f6aaa2 Compare February 26, 2019 19:26

darkdarkdragon closed this Feb 26, 2019

j0sh reviewed Feb 26, 2019

View reviewed changes

darkdarkdragon changed the base branch from it/cleanvendor to master February 26, 2019 19:45

darkdarkdragon reopened this Feb 26, 2019

darkdarkdragon force-pushed the it/webhook branch from 57128e5 to a14173f Compare February 27, 2019 16:16

j0sh requested changes Feb 27, 2019

View reviewed changes

darkdarkdragon force-pushed the it/webhook branch from a14173f to f319978 Compare February 27, 2019 21:14

Webhook URL option - allows authenticate incoming streams through cal…

0bb05b2

…l to webhook

darkdarkdragon force-pushed the it/webhook branch from f319978 to 0bb05b2 Compare February 27, 2019 21:40

j0sh approved these changes Feb 27, 2019

View reviewed changes

darkdarkdragon merged commit fe17294 into master Feb 27, 2019

darkdarkdragon deleted the it/webhook branch February 27, 2019 22:46

j0sh mentioned this pull request Feb 27, 2019

Stream Key Support livepeer/lpms#88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhook URL #752

Webhook URL #752

darkdarkdragon commented Feb 26, 2019

darkdarkdragon commented Feb 26, 2019

darkdarkdragon commented Feb 26, 2019

j0sh Feb 26, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 26, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 26, 2019

darkdarkdragon Feb 26, 2019

j0sh Feb 26, 2019

darkdarkdragon Feb 26, 2019

j0sh Feb 26, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

j0sh Feb 27, 2019

darkdarkdragon Feb 27, 2019

darkdarkdragon commented Feb 27, 2019

j0sh left a comment

j0sh Feb 27, 2019


		func authenticateStream(url, mid string) (core.ManifestID, bool) {

Webhook URL #752

Webhook URL #752

Conversation

darkdarkdragon commented Feb 26, 2019

darkdarkdragon commented Feb 26, 2019

darkdarkdragon commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdarkdragon commented Feb 27, 2019

j0sh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment