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

Verify CDM host files #283

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Verify CDM host files #283

merged 2 commits into from
Aug 30, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Aug 29, 2017

fix brave/browser-laptop#10449

required https://github.com/brave/browser-laptop-bootstrap/pull/21

Auditors: @bridiver, @bbondy, @bsclifton

To reviewers:

  1. Ask me for the cert, keys and passphrase
  2. Ask me for the signature_generator.py from widevine
    3. Copy signature_generator.py into //third_party/widevine/scripts/signature_generator.py before building with sign_widevine

screen shot 2017-08-29 at 8 36 40 am

screen shot 2017-08-29 at 8 37 44 am

screen shot 2017-08-29 at 8 41 16 am

@darkdh darkdh self-assigned this Aug 29, 2017
@darkdh darkdh changed the title Generate related widvine signatures and verify CDM host files [WIP] Generate related widvine signatures and verify CDM host files Aug 29, 2017
@darkdh
Copy link
Member Author

darkdh commented Aug 29, 2017

mac is ok. working on windows

bbondy
bbondy previously approved these changes Aug 29, 2017
widevine_sign_file("sign_cdm_adapter_for_widevine") {
file = "$root_out_dir/$widevine_cdm_path/widevinecdmadapter.plugin"
+ if (!muon_chromium_build) {
+ signature_file = "$file.dummy"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why this is necessary. You can pass the signature file in widevine_sign_file

Copy link
Collaborator

Choose a reason for hiding this comment

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

^^ we can create our own target instead of using chrome's

Copy link
Member Author

Choose a reason for hiding this comment

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

That was meant for preventing duplicate output

Copy link
Member Author

Choose a reason for hiding this comment

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

ERROR at //third_party/widevine/cdm/widevine.gni:15:3: Duplicate output file.
  action(target_name) {
  ^--------------------
Two or more targets generate the same output:
  WidevineCdm/_platform_specific/mac_x64/widevinecdmadapter.plugin.sig

This is can often be fixed by changing one of the target names, or by
setting an output_name on one of them.

Collisions:
  //chrome:sign_cdm_adapter_for_widevine
  //electron/app/mac:sign_cdm_adapter_for_widevine_on_brave

See //third_party/widevine/cdm/widevine.gni:15:3: Collision.
  action(target_name) {
  ^--------------------

Copy link
Collaborator

Choose a reason for hiding this comment

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

so why is the signature file set to $file.dummy?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just use all the same params as chrome?

Copy link
Member Author

Choose a reason for hiding this comment

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

to make it different from WidevineCdm/_platform_specific/mac_x64/widevinecdmadapter.plugin.sig
cause we don't use it here. we have our own //electron/app/mac:sign_cdm_adapter_for_widevine_on_brave

"$flags",
]
+ if (!muon_chromium_build) {
+ args += [
Copy link
Collaborator

Choose a reason for hiding this comment

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

also confused by this one because there is no source here for signature_generator.py

Copy link
Member Author

Choose a reason for hiding this comment

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

we can't disclose the script from widevine. For now, we have to do manually copy to the destination. Maybe we can use submit it to our private repo and pull it to the right place

@@ -8,9 +8,57 @@

#include "base/strings/stringprintf.h"
#include "base/strings/string_util.h"
#include "content/public/common/cdm_info.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

please move this to atom_content_client. Brightray is deprecated and we want to get rid of these classes

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

@darkdh darkdh changed the title [WIP] Generate related widvine signatures and verify CDM host files Verify CDM host files Aug 30, 2017
#if BUILDFLAG(ENABLE_CDM_HOST_VERIFICATION)
if (cdm_host_file_paths) {
#if defined(OS_WIN)
static const base::FilePath::CharType* const kUnversionedFiles[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use base::FILE_EXE instead of hard-coding brave.exe with the path

<< ", signature_path=" << brave_framework_sig_path.value();
cdm_host_file_paths->push_back(
content::CdmHostFilePath(brave_framework_path, brave_framework_sig_path));
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

@posix4e you'll need something here for linux

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Code is compiling, but I get an error when trying to run create_dist unfortunately. Changes look good though

@bridiver bridiver merged commit fedad02 into master Aug 30, 2017
bridiver added a commit that referenced this pull request Aug 30, 2017
Verify CDM host files
bridiver added a commit that referenced this pull request Aug 31, 2017
Verify CDM host files
bbondy pushed a commit that referenced this pull request Aug 31, 2017
bbondy pushed a commit that referenced this pull request Aug 31, 2017
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.

Netflix Streaming Error
4 participants