Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

Cache encryption keys #1433

Closed
wants to merge 1 commit into from
Closed

Conversation

benwilber
Copy link

@benwilber benwilber commented Jun 28, 2018

Description

See: #1395

AES encryption keys are fetched for every segment even if the same key is used to encrypt multiple segments (per HLS specification.) This causes unnecessary load on the key server if access to encryption keys are guarded by user-specific authentication and authorization mechanisms.

Specific Changes proposed

Cache encryption keys based on their resolved URI and only fetch keys when they're needed to decrypt the next series of segments.

Requirements Checklist

  • [x ] Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

This is a rather crude PR because it doesn't update the tests (they fail now) and I'm not even sure this is the correct way to implement this. I'm not very experienced with this tech stack (I can't figure out how the testing libraries are supposed to work) so this is more "request for guidance" to implement this bug fix.

All feedback or assistance is welcome!

@aaronchenweb
Copy link

I got same problem, please help

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and sorry for the delay.

To maintain backwards compatibility, what do you think of adding an option that defaults to the original behavior, but if set to true enables caching. Maybe something like cacheEncryptionKeys? And maybe a test or two around it.

Thanks again.

@forbesjo
Copy link
Contributor

@benwilber are you still interested in this feature?

@benwilber
Copy link
Author

@forbesjo Yes I am still interested in implementing this fix. Unfortunately I'm not very familiar with this tech stack and have been having trouble updating the tests so that this case is properly covered. If anybody could provide any assistance, or point me in the right direction I would appreciate it.

Thanks!

@forbesjo
Copy link
Contributor

Thank you for your PR but we will not be accepting new changes for this repo and will be archived very soon. I would advise you to open your PR against the next iteration of this project at https://github.com/videojs/http-streaming.

@forbesjo forbesjo closed this Jan 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants