[PATCH RFC wayland-protocols v3 1/2] Add secure output protocol
Scott Anderson
scott.anderson at collabora.com
Mon Feb 4 03:11:59 UTC 2019
On 2/02/19 1:16 am, Pekka Paalanen wrote:
> On Mon, 3 Dec 2018 19:14:50 +1300
> scott.anderson at collabora.com wrote:
>
>> From: Scott Anderson <scott.anderson at collabora.com>
>>
>> This protocol allows a client to ask the compositor to only allow it to
>> be displayed on a "secure" output.
>>
>> This is based on a chromium protocol of the same name [1].
>>
>> This protocol is mostly useful for closed systems, where the client can
>> trust the compositor, such as set-top boxes. This is not a way to
>> implement any kind of Digital Rights Management on desktops. The
>> protocol deliberately doesn't define what a "secure output" is, and the
>> compositor would be free to lie to the client anyway.
>>
>> The protocol has been designed to be extendable with other protocols to
>> implement more specific types of security that a client may care about,
>> e.g. HDCP. This extensibility is based on the concept of a "role" that a
>> wl_surface has.
>>
>> Signed-off-by: Scott Anderson <scott.anderson at collabora.com>
>
> Hi Scott,
>
> the commit message is very much to the point. I like how it underlines
> "closed systems" and "not rights management".
>
> My comments below are necessarily colored by the discussions I had with
> Ankit and Ramalingam quite a while ago.
>
>>
>> [1] https://chromium.googlesource.com/chromium/src/+/master/third_party/wayland-protocols/unstable/secure-output/secure-output-unstable-v1.xml
>> ---
>> Makefile.am | 1 +
>> unstable/secure-output/README | 4 +
>> .../secure-output-unstable-v1.xml | 189 ++++++++++++++++++
>> 3 files changed, 194 insertions(+)
>> create mode 100644 unstable/secure-output/README
>> create mode 100644 unstable/secure-output/secure-output-unstable-v1.xml
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 345ae6a..4d94747 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -23,6 +23,7 @@ unstable_protocols = \
>> unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \
>> unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \
>> unstable/primary-selection/primary-selection-unstable-v1.xml \
>> + unstable/secure-output/secure-output-unstable-v1.xml \
>> $(NULL)
>>
>> stable_protocols = \
>> diff --git a/unstable/secure-output/README b/unstable/secure-output/README
>> new file mode 100644
>> index 0000000..3251af9
>> --- /dev/null
>> +++ b/unstable/secure-output/README
>> @@ -0,0 +1,4 @@
>> +Secure output protocol
>> +
>> +Maintainers:
>> +David Reveman <reveman at chromium.org>
>
> Would Reveman really be the only maintainer? I don't recall seeing any
> email from him on this mailing list for a long time. Would he respond
> to any questions and change suggestions that might arise?
>
That's a good point. I just copied this from the chromium protocol which
I originally based this on, and didn't really put much thought into
updating it.
>> diff --git a/unstable/secure-output/secure-output-unstable-v1.xml b/unstable/secure-output/secure-output-unstable-v1.xml
>> new file mode 100644
>> index 0000000..867a03c
>> --- /dev/null
>> +++ b/unstable/secure-output/secure-output-unstable-v1.xml
>> @@ -0,0 +1,189 @@
>> +<?xml version="1.0" encoding="UTF-8"?>
>> +<protocol name="secure_output_unstable_v1">
>> +
>> + <copyright>
>> + Copyright 2016 The Chromium Authors.
>> + Copyright 2018 Collabora, Ltd.
>> +
>> + Permission is hereby granted, free of charge, to any person obtaining a
>> + copy of this software and associated documentation files (the "Software"),
>> + to deal in the Software without restriction, including without limitation
>> + the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + and/or sell copies of the Software, and to permit persons to whom the
>> + Software is furnished to do so, subject to the following conditions:
>> +
>> + The above copyright notice and this permission notice (including the next
>> + paragraph) shall be included in all copies or substantial portions of the
>> + Software.
>> +
>> + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + DEALINGS IN THE SOFTWARE.
>> + </copyright>
>> +
>> + <description summary="Protocol for providing secure output">
>> + This protocol specifies a set of interfaces used to prevent surface
>> + contents from appearing in screenshots or from being visible on non-secure
>> + outputs.
>> +
>> + In order to prevent surface contents from appearing in screenshots or from
>> + being visible on non-secure outputs, a client must first bind the global
>> + interface "wp_secure_output" which, if a compositor supports secure output,
>> + is exposed by the registry. Using the bound global object, the client uses
>> + the "get_security" request to instantiate an interface extension for a
>> + wl_surface object. This extended interface will then allow surfaces
>> + to be marked as only visible on secure outputs.
>> +
>> + Warning! The protocol described in this file is experimental and backward
>> + incompatible changes may be made. Backward compatible changes may be added
>> + together with the corresponding interface version bump. Backward
>> + incompatible changes are done by bumping the version number in the protocol
>> + and interface names and resetting the interface version. Once the protocol
>> + is to be declared stable, the 'z' prefix and the version number in the
>> + protocol and interface names are removed and the interface version number is
>> + reset.
>> + </description>
>> +
>> + <interface name="zwp_secure_output_v1" version="1">
>> + <description summary="secure output">
>> + The global interface exposing secure output capabilities is used
>> + to instantiate an interface extension for a wl_surface object.
>> + This extended interface will then allow surfaces to be marked as
>> + as only visible on secure outputs.
>> + </description>
>> +
>> + <request name="destroy" type="destructor">
>> + <description summary="unbind from the secure output interface">
>> + Informs the server that the client will not be using this
>> + protocol object anymore. This does not affect any other objects,
>> + security objects included.
>> + </description>
>> + </request>
>> +
>> + <enum name="error">
>> + <entry name="security_exists" value="0"
>> + summary="the surface already has a security object associated"/>
>> + <entry name="implementation" value="1"
>> + summary="the wp_security already has an implementation"/>
>> + </enum>
>> +
>> + <request name="get_security">
>> + <description summary="extend surface interface for security">
>> + Instantiate an interface extension for the given wl_surface to
>> + provide surface security. If the given wl_surface already has
>> + a security object associated, the security_exists protocol error
>> + is raised.
>> + </description>
>> + <arg name="id" type="new_id" interface="zwp_security_v1"
>> + summary="the new security interface id"/>
>> + <arg name="surface" type="object" interface="wl_surface"
>> + summary="the surface"/>
>
> Being tied to a wl_surface is an excellent design, I think it is
> mandatory.
>
>> + </request>
>> +
>> + <request name="set_default">
>> + <description summary="set wp_security implementation">
>> + This sets the implementation of a wp_security object to the server's
>> + default. See the description of wp_security for the details of security
>> + implementations.
>> +
>> + This provides no extra interfaces to the wp_security object and would be
>> + used by clients that don't care about the details of how they're secured.
>> +
>> + The default implementation is considered a distinct implementation from
>> + all other implementations, even if they're implemented in the same way.
>> + </description>
>> + <arg name="security" type="object" interface="zwp_security_v1"
>> + summary="the wp_security object"/>
>> + </request>
>> + </interface>
>> +
>> + <interface name="zwp_security_v1" version="1">
>> + <description summary="security interface to a wl_surface">
>> + An additional interface to a wl_surface object, which allows the
>> + client to specify that a surface should not appear in screenshots
>> + or be visible on non-secure outputs.
>> +
>> + In order for a security object to take effect, the security object must
>> + have an implementation set for it.
>> +
>> + A security object can only have one implementation at a time. Initially
>> + the security object has no implementation. Once the implementation is
>> + set, it is set permanently set for the whole lifetime of the security
>> + object. Setting the same interface again is allowed, unless explicitly
>> + forbidden by the relevant interface specification.
>> +
>> + Security implementations are set by the requests in other interfaces
>> + such as wp_secure_output.set_default. The request should explicitly
>> + mention that the request sets the wp_security's implementation.
>> + Often, this request also creates a new protocol object that represents
>> + the implementation and adds additional functionality to the wp_security.
>> + When a client wants to destroy a wp_security, they must destroy this
>> + 'implementation object' before the wp_security.
>
> This role based model feels over-enginereed to me. There is the HDCP
> case, but do we have any other real cases?
>
> The reason wl_surface went with roles, is that the roles change the
> behaviour and the allowed protocol sequences on the wl_surface
> radically. I don't see such diversity here.
>
> Here a straw-man:
>
> Protection level is an enum. At first it would contain three entries:
> - UNPROTECTED
> - HDCP_TYPE_0
> - HDCP_TYPE_1
>
> A client tags a wl_surface with one value from the enums, the
> compositor attempts to reach that level or higher, and the compositor
> keeps the client informed on what level of protection the wl_surface is
> currently having.
>
> Adding new values to the enum for new protection types is trivial. If
> some new protection types are parameterised, it is equally easy to add
> a request to pass the parameters. This can all be done
> backward-compatibly.
You're probably right about being over-engineered. The intention was to
keep it decoupled from HDCP and allow a client to take a more general "I
don't care about how the security is implemented" stance.
I'd be fine with the roles being removed and this just being a single
protocol instead of being split in two.
>> +
>> + If the wl_surface associated with the security object is destroyed,
>> + the security object becomes inert.
>> +
>> + If the security object is destroyed, the security state is removed
>> + from the wl_surface. The change will be applied on the next
>> + wl_surface.commit.
>> + </description>
>> +
>> + <enum name="error">
>> + <entry name="no_implementation" value="0"
>> + summary="wp_security has no implementation"/>
>> + </enum>
>> +
>> + <request name="destroy" type="destructor">
>> + <description summary="remove security from the surface">
>> + The associated wl_surface's security state is removed.
>> + The change is applied on the next wl_surface.commit.
>> + </description>
>> + </request>
>> +
>> + <request name="set_constrain_outputs">
>> + <description summary="set the only visible on secure output state">
>> + Constrain visibility of wl_surface contents to secure outputs.
>> + See wp_secure_output for the description.
>> +
>> + A 'no_implementation' protocol error is raised if this is called
>> + while the wp_security has no implementation.
>> +
>> + The constrain outputs state is double-buffered, and will be applied
>> + on the next wl_surface.commit.
>> + </description>
>> + <arg name="constrain" type="uint"
>> + summary="wl_surface is constrained to secure outputs"/>
>
> This seems like a good idea. A client can choose if it can tolerate a
> protection level below its demands. Some clients may be happy to show a
> few frames of high quality content on unprotected outputs until they
> themselves react and downgrade the quality. Other clients do not want
> even a single frame to be shown without the required protection level.
>
> Obviously, if the strict constraint is set, the compositor needs to
> replace the protected content with a placeholder on outputs where the
> protection is not sufficient.
>
>> + </request>
>> +
>> + <event name="status">
>> + <description summary="security status changed">
>> + This event will be emitted when there is a change in the security
>> + status.
>> +
>> + If status is zero, then the content was unable to be displayed
>> + securely. If status is one, then the content was able to be displayed
>> + securely.
>> +
>> + These events will not be sent until the wp_security has an
>> + implementation, and the client should assume that the content cannot
>> + be shown securely until this event is first sent.
>> +
>> + If the content cannot be shown securely, then it is compositor-defined
>> + for what will be displayed for the client's content.
>> +
>> + A possible reason the security status changes is due to a new connector
>> + being added or the client attaching a buffer too large for what the
>> + security may protect. However, it is not limited to these reasons.
>
> A buffer too large, really? How can that happen?
>
HDCP has some rules about content resolution and such, depending on the
version, which I don't really understand the nuances of.
An example situation would be that a secure client is displaying 4K
content, a new connector is added, and the maximum security now only
provides up to 2K content, so the client needs to know and potentially
change its resolution.
>> +
>> + A client may want to listen to this event and lower the resolution of
>> + their content until it can successfully be shown securely.
>> + </description>
>> + <arg name="status" type="uint" summary="the security status"/>
>> + </event>
>> + </interface>
>> +
>> +</protocol>
>
> I won't comment on the patch 2 of this series yet until there is a
> consensus if the role based model is even needed. Right now, all it
> really does is add the two enum values HDCP_TYPE_0 and HDCP_TYPE_1.
>
>
> Thanks,
> pq
Considering that this is probably going to go forward just being part of
weston, I'll keep future discussions of this on the various weston MRs
and issues.
Scott
More information about the wayland-devel
mailing list