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

record-tester: added access control testing #220

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gioelecerati
Copy link
Member

@gioelecerati gioelecerati commented Nov 10, 2022

Fixes #204

@gioelecerati gioelecerati requested a review from a team as a code owner November 10, 2022 16:04
@gioelecerati gioelecerati marked this pull request as draft November 10, 2022 16:04
@gioelecerati gioelecerati marked this pull request as ready for review November 17, 2022 00:10
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -156,6 +184,19 @@ func (rt *recordTester) Start(fileName string, testDuration, pauseDuration time.
mediaURL := fmt.Sprintf("%s/%s/index.m3u8", ingest.Playback, stream.PlaybackID)
glog.V(model.SHORT).Infof("RTMP: %s", rtmpURL)
glog.V(model.SHORT).Infof("MEDIA: %s", mediaURL)

if rt.accessControl && (rt.signingKey != "" && rt.publicKey != "") {
Copy link
Member

Choose a reason for hiding this comment

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

Having accessControl and not the signing key should be an explicit fatal error. We should not ignore the setting because another required option was not provided, it is better to just fail the whole test.

Suggested change
if rt.accessControl && (rt.signingKey != "" && rt.publicKey != "") {
if rt.accessControl {
if rt.signingKey == "" || rt.publicKey == "" {
msg := "Signing key and public key are required for access control test"
glog.Fatal(msg)
return 1, errors.New(msg) // (Fatal log above already exits the process but you know just in case, leave it clear here that we end with an error)
}

Comment on lines +195 to +198
} else {
glog.Warningf("No access control for stream id=%s playbackId=%s name=%s mediaURL=%s", stream.ID, stream.PlaybackID, streamName, mediaURL)
return 2, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

No reason to log this as a warning or returning. Executing a test without access control is perfectly acceptable. And we already log above when we use access control so no need to log when we don't.

Suggested change
} else {
glog.Warningf("No access control for stream id=%s playbackId=%s name=%s mediaURL=%s", stream.ID, stream.PlaybackID, streamName, mediaURL)
return 2, nil
}
}

// uploader := testers.NewRtmpStreamer(gctx, rtmpURL)
// uploader.StartUpload(fileName, rtmpURL, -1, 30*time.Second)
return es, err
return 0, err
Copy link
Member

Choose a reason for hiding this comment

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

nit: We know err is nil here so we can be explicit. Just so one doesn't start wondering here where this err could be instead of nil (from what I checked, it can't)

Suggested change
return 0, err
return 0, nil

Comment on lines +435 to +443
if err != nil {
glog.Errorf("Unable to parse provided signing key for access control signingKey=%s", rt.signingKey)
}

token, err := unsignedToken.SignedString(pk)

if err != nil {
glog.Errorf("Unable to sign JWT with provided private key for access control signingKey=%s", rt.signingKey)
}
Copy link
Member

Choose a reason for hiding this comment

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

We need to return all these errors. Cannot just log.

Suggested change
if err != nil {
glog.Errorf("Unable to parse provided signing key for access control signingKey=%s", rt.signingKey)
}
token, err := unsignedToken.SignedString(pk)
if err != nil {
glog.Errorf("Unable to sign JWT with provided private key for access control signingKey=%s", rt.signingKey)
}
if err != nil {
glog.Errorf("Unable to parse provided signing key for access control signingKey=%s", rt.signingKey)
return "", err
}
token, err := unsignedToken.SignedString(pk)
if err != nil {
glog.Errorf("Unable to sign JWT with provided private key for access control signingKey=%s", rt.signingKey)
return "", err
}

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.

Create continuous test for access controlled media (secure playback)
2 participants