Add PPAPI interfaces for platform verification.

Add PPAPI interfaces for platform verification.

Interfaces only at this point.  Implementation to follow.  API mirrors
that which is provided to the CDMs in http://crrev.com/221019

CanChallengePlatform() can be synchronous since it will just check a
command line flag passed to the process.

BUG=270294
TEST=none
R=ddorwin@chromium.org, dmichael@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222113

[DaleCurtis, 2013-09-04 22:29:07]

The generator ended up creating lines > 80 characters for this interface;
presumably that's going to block landing this via CQ. I'll take a look and see
if there's a quick generator fix for that.

[ddorwin, 2013-09-06 05:58:28]

IDL LG overall.

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
File ppapi/api/private/ppb_platform_verification_private.idl (right):

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
ppapi/api/private/ppb_platform_verification_private.idl:20: * untrusted platform
is a Chromebook in developer mode.
I think we should give an example of where we might get an affirmative answer:
s/un//
s/developer/verified boot/

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
ppapi/api/private/ppb_platform_verification_private.idl:52: PP_Bool
CanChallengePlatform();
Maybe this was discussed elsewhere, but "CanChallenge" doesn't exactly convey
"should pass platform challenge". I guess maybe you "can't"/shouldn't challenge
it if the platform knows it will fail.

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
ppapi/api/private/ppb_platform_verification_private.idl:63: * @param[out]
signed_data A <code>PP_Var</code> of type
I'm not clear on this one. Let's discuss.

[DaleCurtis, 2013-09-06 21:49:31]

PTAL.  I've also fixed the generator to limit lines to 80 chars.

+tinskip FYI.

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
File ppapi/api/private/ppb_platform_verification_private.idl (right):

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
ppapi/api/private/ppb_platform_verification_private.idl:20: * untrusted platform
is a Chromebook in developer mode.
On 2013/09/06 05:58:28, ddorwin wrote:
> I think we should give an example of where we might get an affirmative answer:
> s/un//
> s/developer/verified boot/

Done.

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
ppapi/api/private/ppb_platform_verification_private.idl:52: PP_Bool
CanChallengePlatform();
On 2013/09/06 05:58:28, ddorwin wrote:
> Maybe this was discussed elsewhere, but "CanChallenge" doesn't exactly convey
> "should pass platform challenge". I guess maybe you "can't"/shouldn't
challenge
> it if the platform knows it will fail.

It was discussed on the the CDM.h CL. CanChallengePlatform() conveys enough of
that meaning without being overly verbose. The description also mentions that it
will only return true if the challenge is expected to pass.

https://codereview.chromium.org/23569005/diff/3001/ppapi/api/private/ppb_plat...
ppapi/api/private/ppb_platform_verification_private.idl:63: * @param[out]
signed_data A <code>PP_Var</code> of type
On 2013/09/06 05:58:28, ddorwin wrote:
> I'm not clear on this one. Let's discuss.

Done. I've removed the implementation details.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
File ppapi/api/private/ppb_platform_verification_private.idl (right):

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:61: *
<code>PP_VARTYPE_ARRAY_BUFFER</code> that contains the challenge data.
One PPAPI Q I have is whether it's worth having these as array buffers or if
there's a more efficient type for their size -- they'll probably range in length
from 256-512 bytes of non-ascii data.

[ddorwin, 2013-09-07 04:31:43]

lgtm with .py suggestions

https://codereview.chromium.org/23569005/diff/13001/ppapi/generators/idl_c_pr...
File ppapi/generators/idl_c_proto.py (right):

https://codereview.chromium.org/23569005/diff/13001/ppapi/generators/idl_c_pr...
ppapi/generators/idl_c_proto.py:613: if 'http' in line:
minor issue: Should we create a new line if 'http' is not the first non-comment
character?

// http://...
  does not make a new line, but 
// See spec: http://
  should

https://codereview.chromium.org/23569005/diff/13001/ppapi/generators/idl_c_pr...
ppapi/generators/idl_c_proto.py:613: if 'http' in line:
Should we combine this if with the one above?

If that's too messy, does Python support else if? Either way, it's clearer that
this block is unrelated to the rest of the "else" and we get rid of the
continue.

[dmichael (off chromium), 2013-09-09 15:54:20]

mostly comments on comments, lgtm

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
File ppapi/api/private/ppb_platform_verification_private.idl (right):

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:19: * services to verify
that the underlying platform is trusted. An example of
Would it be worthwhile to expand on what you mean by trusted? My understanding
is it isn't really a judgement of quality of the platform or anything, just a
question of whether the platform provides storage (and maybe other services)
that are private to the app. Or something like that.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:20: * a trusted platform
is a Chromebook in verified boot mode.
s/a Chromebook/ChromeOS
?
since not all ChromeOS devices are Chromebooks

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:51: */
nit: Please use the same comment format as the other methods.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:52: PP_Bool
CanChallengePlatform([in] PP_Resource instance);
This doesn't seem strictly necessary. Is this basically an optimization so you
don't have to bother creating a challenge?

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:55: * Requests a
platform challenge for a given service id.
Do you have any public documentation giving more detail about the challenge to
which you can refer?

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:61: *
<code>PP_VARTYPE_ARRAY_BUFFER</code> that contains the challenge data.
On 2013/09/06 21:49:32, DaleCurtis wrote:
> One PPAPI Q I have is whether it's worth having these as array buffers or if
> there's a more efficient type for their size -- they'll probably range in
length
> from 256-512 bytes of non-ascii data.
tl;dr:  ArrayBuffer vars seem like the right choice here. 

- PPB_Buffer_Dev would work, but always uses shared memory. That would be
wasteful for such small buffers (wasting handles, requiring creation to go to
the browser to get the shared memory). ArrayBuffer will send the data directly
for small sizes (I can't remember the cutoff...  it was picked through empirical
testing, but it's a lot bigger than the sizes you're talking about).

- String Var would also be a bad choice, since it's required to be valid UTF-8.

- The only reasonable alternative to an ArrayBuffer var would be just a byte
array and size. We use that for interfaces like FileIO where you might already
have a buffer that you want to read from or write to, and allocating and copying
to a separate ArrayBuffer would be a waste. ChallengePlatform isn't called
frequently, and using ArrayBuffer shouldn't add extra copies or allocations
anyway.

[DaleCurtis, 2013-09-09 20:49:14]

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
File ppapi/api/private/ppb_platform_verification_private.idl (right):

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:19: * services to verify
that the underlying platform is trusted. An example of
On 2013/09/09 15:54:21, dmichael wrote:
> Would it be worthwhile to expand on what you mean by trusted? My understanding
> is it isn't really a judgement of quality of the platform or anything, just a
> question of whether the platform provides storage (and maybe other services)
> that are private to the app. Or something like that.

I believe the current text is sufficient. I'm hesitant to add too many details
which might be platform specific.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:20: * a trusted platform
is a Chromebook in verified boot mode.
On 2013/09/09 15:54:21, dmichael wrote:
> s/a Chromebook/ChromeOS
> ?
> since not all ChromeOS devices are Chromebooks

Done.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:51: */
On 2013/09/09 15:54:21, dmichael wrote:
> nit: Please use the same comment format as the other methods.

Done.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:52: PP_Bool
CanChallengePlatform([in] PP_Resource instance);
On 2013/09/09 15:54:21, dmichael wrote:
> This doesn't seem strictly necessary. Is this basically an optimization so you
> don't have to bother creating a challenge?

Yes, kind of, it avoids both the cost of creating a challenge as well as a
network round trip if challenges are expected to pass.  Additionally the
information necessary to construct the challenge may not be available at the
time a client needs to check if platform challenges can succeed.

Essentially it will allow us to avoid a noticeable playback delay in the most
common case where platform challenges are expected to succeed. It also allows
graceful fallback to alternate protection paths for dev mode machines.

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:55: * Requests a
platform challenge for a given service id.
On 2013/09/09 15:54:21, dmichael wrote:
> Do you have any public documentation giving more detail about the challenge to
> which you can refer?

For ChromeOS it's just n bits of nonce data; which is what I originally wrote
here.  In the previous patch set ddorwin asked for its removal in order to avoid
platform specific implementation details in the interface.

In a broader sense this is the only public documentation I can find currently:
http://www.chromium.org/developers/design-documents/tpm-usage#TOC-Attesting-D...

https://codereview.chromium.org/23569005/diff/13001/ppapi/api/private/ppb_pla...
ppapi/api/private/ppb_platform_verification_private.idl:61: *
<code>PP_VARTYPE_ARRAY_BUFFER</code> that contains the challenge data.
On 2013/09/09 15:54:21, dmichael wrote:
> On 2013/09/06 21:49:32, DaleCurtis wrote:
> > One PPAPI Q I have is whether it's worth having these as array buffers or if
> > there's a more efficient type for their size -- they'll probably range in
> length
> > from 256-512 bytes of non-ascii data.
> tl;dr:  ArrayBuffer vars seem like the right choice here. 
> 
> - PPB_Buffer_Dev would work, but always uses shared memory. That would be
> wasteful for such small buffers (wasting handles, requiring creation to go to
> the browser to get the shared memory). ArrayBuffer will send the data directly
> for small sizes (I can't remember the cutoff...  it was picked through
empirical
> testing, but it's a lot bigger than the sizes you're talking about).
> 
> - String Var would also be a bad choice, since it's required to be valid
UTF-8.
> 
> - The only reasonable alternative to an ArrayBuffer var would be just a byte
> array and size. We use that for interfaces like FileIO where you might already
> have a buffer that you want to read from or write to, and allocating and
copying
> to a separate ArrayBuffer would be a waste. ChallengePlatform isn't called
> frequently, and using ArrayBuffer shouldn't add extra copies or allocations
> anyway.

Thanks for the detailed answer.

https://codereview.chromium.org/23569005/diff/13001/ppapi/generators/idl_c_pr...
File ppapi/generators/idl_c_proto.py (right):

https://codereview.chromium.org/23569005/diff/13001/ppapi/generators/idl_c_pr...
ppapi/generators/idl_c_proto.py:613: if 'http' in line:
On 2013/09/07 04:31:43, ddorwin wrote:
> minor issue: Should we create a new line if 'http' is not the first
non-comment
> character?
> 
> // http://...
>   does not make a new line, but 
> // See spec: http://
>   should

There are a lot of cases here that aren't handled; i.e. http may not even be in
a comment.  Comments may be block or inline too, so that makes detection tricky.
 I'd rather leave such cases alone until they need to be supported.

IMHO, Indent() should be removed in favor of clang-format integration. 
Obviating the need for whats sure to always be a partial subset of correct
indentation.

https://codereview.chromium.org/23569005/diff/13001/ppapi/generators/idl_c_pr...
ppapi/generators/idl_c_proto.py:613: if 'http' in line:
On 2013/09/07 04:31:43, ddorwin wrote:
> Should we combine this if with the one above?
> 
> If that's too messy, does Python support else if? Either way, it's clearer
that
> this block is unrelated to the rest of the "else" and we get rid of the
> continue.

Done; though now space_break is always computed.

[DaleCurtis, 2013-09-09 21:02:43]

I'm pretty sure the CQ is going to reject this due to:

** Presubmit ERRORS **
Missing PPAPI IDL for stable interface:

***************
  ppapi/c/ppb_file_system.h
***************

dmichael: Is this something that I should add? Fix the PRESUBMIT.  Just dcommit?

[dmichael (off chromium), 2013-09-09 21:06:51]

If I were you, I'd probably dcommit it. But if you want to roll up your
sleeves and fix the PRESUBMIT (maybe turn it in to a WARNING if something
in ppapi/generators changed)...  I support that, too.


On Mon, Sep 9, 2013 at 3:02 PM, <dalecurtis@chromium.org> wrote:

> I'm pretty sure the CQ is going to reject this due to:
>
> ** Presubmit ERRORS **
> Missing PPAPI IDL for stable interface:
>
> ***************
>   ppapi/c/ppb_file_system.h
> ***************
>
> dmichael: Is this something that I should add? Fix the PRESUBMIT.  Just
> dcommit?
>
>
>
https://codereview.chromium.**org/23569005/<https://codereview.chromium.org/2...
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
chromium-reviews+unsubscribe@**chromium.org<chromium-reviews%2Bunsubscribe@ch...
> .
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-09 21:59:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23569005/20001

[DaleCurtis, 2013-09-09 22:07:09]

Whoops I should upload first. PRESUBMIT changed to a warning.

[commit-bot: I haz the power, 2013-09-09 22:09:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23569005/38001

[dmichael (off chromium), 2013-09-09 22:18:25]

https://chromiumcodereview.appspot.com/23569005/diff/38001/ppapi/PRESUBMIT.py
File ppapi/PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/23569005/diff/38001/ppapi/PRESUBMIT.py...
ppapi/PRESUBMIT.py:251: output_api.PresubmitPromptWarning(
I meant to make this a warning _only_ in the case where a file in
ppapi/generators has changed. We should probably just do that change separately.


Sorry, I unflipped the CQ bit, because this should stay an Error most of the
time. I'll slap together a PRESUBMIT change.

[DaleCurtis, 2013-09-09 22:20:37]

Looks like it still failed the CQ anyways since it uses the old PRESUBMIT, hah!

How about I just remove the PRESUBMIT change and dcommit?

[dmichael (off chromium), 2013-09-09 22:25:59]

On Mon, Sep 9, 2013 at 4:20 PM, <dalecurtis@chromium.org> wrote:

> Looks like it still failed the CQ anyways since it uses the old PRESUBMIT,
> hah!
>
> How about I just remove the PRESUBMIT change and dcommit?

sgtm/lgtm


>
>
>
https://chromiumcodereview.**appspot.com/23569005/<https://chromiumcodereview...
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
chromium-reviews+unsubscribe@**chromium.org<chromium-reviews%2Bunsubscribe@ch...
> .
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2013-09-09 22:36:24]

Committed patchset #3 manually as r222113.