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

Permission prompt spec #94

Closed
wants to merge 0 commits into from
Closed

Permission prompt spec #94

wants to merge 0 commits into from

Conversation

iVanlIsh
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@johnathan79717 johnathan79717 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 updating the spec. I have some questions and found some spelling errors.

Also, I don't think you should commit the .DS_Store file?

index.src.html Outdated
Comment on lines 508 to 510
The <a http-header>`Private-Network-Access-Name`</a> attempt to provide users
a human friendly name in the [=private network access permission prompt=] (will
simpliy use the content permission prompt below).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The <a http-header>`Private-Network-Access-Name`</a> attempt to provide users
a human friendly name in the [=private network access permission prompt=] (will
simpliy use the content permission prompt below).
The <a http-header>`Private-Network-Access-Name`</a> attempts to provide users
a human friendly name in the [=private network access permission prompt=] (will
simply use the content permission prompt below).

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to the edits. Also I don't fully understand what you mean by "will simply use the content permission prompt below". Do you mean to say that the header value is displayed in the permission prompt described below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean that in this document "permission prompt" specifically refers to "private network access permission prompt".

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think instead of giving it a shorter name, it's probably clearer to just use the longer time consistently?

index.src.html Outdated
Comment on lines 591 to 592
However, private devices which is not able to migrate with secure context would
be able to use a [=permission prompt=] to relax mixed-content check.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
However, private devices which is not able to migrate with secure context would
be able to use a [=permission prompt=] to relax mixed-content check.
However, private devices which are not able to migrate with secure context would
be able to use a [=permission prompt=] to relax mixed-content checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest explaining a bit more why we suddently mention mixed content. How about:

Suggested change
However, private devices which is not able to migrate with secure context would
be able to use a [=permission prompt=] to relax mixed-content check.
Mixed content checks [[MIXED-CONTENT-2]] prevent secure contexts from making
requests over HTTP, so this restriction would seem to require that local network
servers migrate to HTTPS. This is often difficult to impossible. A new
[=permission prompt=] is introduced to allow secure contexts to make requests
over HTTP to the local network anyway, given user consent.

index.src.html Outdated
@@ -615,7 +622,15 @@ <h4 id="secure-context-restriction">Secure context restriction</h4>

1. If |connection|'s [=connection/IP address space=] is
[=IP address space/less public=] than |clientAddressSpace|, then
return a [=network error=].
show a [=permission prompt=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we always block clients in insecure contexts, and only show a permission prompt to clients in secure contexts when the "connection" is in HTTP.

The explainer says

This would mean that websites that wish to speak to private network devices must be served over HTTPS. The target device, however, would not have to serve HTTPS. It would only need to respond correctly to CORS preflights, and maybe to some kind of simple identification request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, you'll want to add an else branch to this "if client is a non-secure context", or invert the clause:

if secure context:
  ... do permission stuff ...
else:
  compare address spaces, maybe return error

index.src.html Outdated
@@ -615,7 +622,15 @@ <h4 id="secure-context-restriction">Secure context restriction</h4>

1. If |connection|'s [=connection/IP address space=] is
[=IP address space/less public=] than |clientAddressSpace|, then
return a [=network error=].
show a [=permission prompt=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't always need to show a permission prompt, right, because the permission state can be cached, can't it? We should then maybe define when we should show the prompt based on the permission state?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably define a "obtain user permission" algorithm or something similar, and use it here.

index.src.html Outdated
Comment on lines 625 to 627
show a [=permission prompt=].

1. If |permission|'s result is:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the "permission" variable is not defined before it's used.
Also I feel like "permission's result" is a little awkward.
Maybe something like below:

Suggested change
show a [=permission prompt=].
1. If |permission|'s result is:
show a <a data-link-type="dfn" href="#private-network-access-permission-prompt" id="ref-for-private-network-access-permission-prompt②">permission prompt</a> and ask for the user's <dfn ...>permission</dfn>.</p>
<li data-md>
<p>If <var>permission</var> is:</p>

index.src.html Outdated

1. If |permission|'s result is:

1. [Deniel](https://www.w3.org/TR/permissions/#:~:text=following%20states%3A-,Denied,-%3A),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
1. [Deniel](https://www.w3.org/TR/permissions/#:~:text=following%20states%3A-,Denied,-%3A),
1. [Denied](https://www.w3.org/TR/permissions/#:~:text=following%20states%3A-,Denied,-%3A),

Copy link
Collaborator

Choose a reason for hiding this comment

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

There must be a better way of linking to that: one of the primary purposes of bikeshed, the spec generation tool, is that it allows cross-linking between specs.

Given that denied is a definition in the permissions spec (https://www.w3.org/TR/permissions/#dfn-denied), I think we can probably write [=denied=]? Bikeshed might complain that it is ambiguous, in which case we will likely want to add a an entry for it in <pre class="link-defaults"> near the top of the file.

index.src.html Outdated

<em>This section is not normative.</em>

According to the discutions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
According to the discutions
According to the discussions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
According to the discutions
Following the discussions in

index.src.html Outdated
Comment on lines 881 to 883
The overarching goal is to provide a way for private devices which are not able
and not welling to directly talk to any public servers including TLS certificate
authorities and not able to apply secure contexts to relax mixed-content checks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The overarching goal is to provide a way for private devices which are not able
and not welling to directly talk to any public servers including TLS certificate
authorities and not able to apply secure contexts to relax mixed-content checks.
The overarching goal is to provide a way for private devices which are not able
or not willing to directly talk to any public servers including TLS certificate
authorities or the ones which are not able to apply secure contexts to relax mixed-content checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really unclear to me. How about:

Suggested change
The overarching goal is to provide a way for private devices which are not able
and not welling to directly talk to any public servers including TLS certificate
authorities and not able to apply secure contexts to relax mixed-content checks.
The goal of the permission is to allow communication from public websites to local
network servers over HTTP, which would otherwise be prevented by the
<a href="#secure-context-restriction">secure context restriction</a> and mixed
content checks. Migrating local network servers to HTTPS has indeed proven to
be often difficult, even sometimes impossible.

index.src.html Outdated
Comment on lines 891 to 911
<h3 id="permission-states">States</h3>

The states of the private network access permission can be one of the three:

* <dfn export>Denied</dfn>:

* <dfn export>Granted</dfn>:

* <dfn export>Prompt</dfn>:

<h3 id="permission-store">Store</h3>

The user agent maintains a single permission store which is a list of permission
store entries. Each particular entry denoted by its descriptor and key can only
appear at most once in the list.

The |key| for the entries should be the IP address of local devices.

The |discriptor| woule contains the list of origin which gained user permission
for the key IP address, the optional name for the entry and the optional id for
the entry.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we link these back to the permission spec instead of repeating it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. In the to-be-written "obtain local network access permission" algorithm, we should probably get and set permission store entries (using the appropriate algorithms from the permissions spec) with the right keys. We can explain the keying schema in plain English for readability too.

Copy link
Collaborator

@letitz letitz 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 sending this, and apologies for the very long wait. We're on the right track!

A bunch of comments below, some editorial, some more important. In particular, I think this is entirely missing how we obtain permission. We probably want to define an "obtain local network access permission" algorithm or similar that takes the target origin and IP address space as arguments. It will likely need to call get the permission state and prompt the user to choose from the permissions spec. It will also need to define how we use the new header. In fact, the structure of the spec will need to be changed a bit, because now we will need to define the infrastructure for preflights in the secure context restriction section. Right now, the spec is structured as two steps (1: secure context restriction, 2: preflights), but it seems to me that we need to revisit that.

Otherwise, as @johnathan79717 said, let's not commit .DS_Store.

Finally, we'll also want to write "local network" instead of "private network" everywhere - since this PR will likely land after #97.

index.src.html Outdated
Comment on lines 508 to 510
The <a http-header>`Private-Network-Access-Name`</a> attempt to provide users
a human friendly name in the [=private network access permission prompt=] (will
simpliy use the content permission prompt below).
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to the edits. Also I don't fully understand what you mean by "will simply use the content permission prompt below". Do you mean to say that the header value is displayed in the permission prompt described below?

index.src.html Outdated
Comment on lines 591 to 592
However, private devices which is not able to migrate with secure context would
be able to use a [=permission prompt=] to relax mixed-content check.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest explaining a bit more why we suddently mention mixed content. How about:

Suggested change
However, private devices which is not able to migrate with secure context would
be able to use a [=permission prompt=] to relax mixed-content check.
Mixed content checks [[MIXED-CONTENT-2]] prevent secure contexts from making
requests over HTTP, so this restriction would seem to require that local network
servers migrate to HTTPS. This is often difficult to impossible. A new
[=permission prompt=] is introduced to allow secure contexts to make requests
over HTTP to the local network anyway, given user consent.

index.src.html Outdated
@@ -615,7 +622,15 @@ <h4 id="secure-context-restriction">Secure context restriction</h4>

1. If |connection|'s [=connection/IP address space=] is
[=IP address space/less public=] than |clientAddressSpace|, then
return a [=network error=].
show a [=permission prompt=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, you'll want to add an else branch to this "if client is a non-secure context", or invert the clause:

if secure context:
  ... do permission stuff ...
else:
  compare address spaces, maybe return error

index.src.html Outdated
@@ -615,7 +622,15 @@ <h4 id="secure-context-restriction">Secure context restriction</h4>

1. If |connection|'s [=connection/IP address space=] is
[=IP address space/less public=] than |clientAddressSpace|, then
return a [=network error=].
show a [=permission prompt=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably define a "obtain user permission" algorithm or something similar, and use it here.

index.src.html Outdated

1. If |permission|'s result is:

1. [Deniel](https://www.w3.org/TR/permissions/#:~:text=following%20states%3A-,Denied,-%3A),
Copy link
Collaborator

Choose a reason for hiding this comment

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

There must be a better way of linking to that: one of the primary purposes of bikeshed, the spec generation tool, is that it allows cross-linking between specs.

Given that denied is a definition in the permissions spec (https://www.w3.org/TR/permissions/#dfn-denied), I think we can probably write [=denied=]? Bikeshed might complain that it is ambiguous, in which case we will likely want to add a an entry for it in <pre class="link-defaults"> near the top of the file.

index.src.html Outdated

According to the discutions
[[Issue#23](https://github.com/WICG/private-network-access/issues/23)], the
<dfn export local-lt="permission prompt">private network access permission
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not define a spec term in a non-normative section, I think. Instead, I think we should specify somewhere above (perhaps in the secure context restriction section) a new "local network access" permission (not prompt), and perhaps a "obtain local network access permission" algorithm. I'm not sure how much we can specify the prompt itself. I think we'll have to say that the permission is obtained in a UA-defined manner? @engedy probably knows best how to specify permissions and their UI.

index.src.html Outdated
Comment on lines 881 to 883
The overarching goal is to provide a way for private devices which are not able
and not welling to directly talk to any public servers including TLS certificate
authorities and not able to apply secure contexts to relax mixed-content checks.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really unclear to me. How about:

Suggested change
The overarching goal is to provide a way for private devices which are not able
and not welling to directly talk to any public servers including TLS certificate
authorities and not able to apply secure contexts to relax mixed-content checks.
The goal of the permission is to allow communication from public websites to local
network servers over HTTP, which would otherwise be prevented by the
<a href="#secure-context-restriction">secure context restriction</a> and mixed
content checks. Migrating local network servers to HTTPS has indeed proven to
be often difficult, even sometimes impossible.

index.src.html Outdated
Comment on lines 891 to 899
<h3 id="permission-states">States</h3>

The states of the private network access permission can be one of the three:

* <dfn export>Denied</dfn>:

* <dfn export>Granted</dfn>:

* <dfn export>Prompt</dfn>:
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 think we need to redefine the states defined in the permissions spec.

index.src.html Outdated
Comment on lines 885 to 889
<h3 id="permission-header">Additional CORS Headers</h3>

The <dfn export http-header>`Private-Network-Access-Name`</dfn> attempt to
provide users a human friendly name in the private network access permission
prompt.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a repeat of the CORS header modification above? Also, we should not define things in non-normative sections.

index.src.html Outdated
Comment on lines 891 to 911
<h3 id="permission-states">States</h3>

The states of the private network access permission can be one of the three:

* <dfn export>Denied</dfn>:

* <dfn export>Granted</dfn>:

* <dfn export>Prompt</dfn>:

<h3 id="permission-store">Store</h3>

The user agent maintains a single permission store which is a list of permission
store entries. Each particular entry denoted by its descriptor and key can only
appear at most once in the list.

The |key| for the entries should be the IP address of local devices.

The |discriptor| woule contains the list of origin which gained user permission
for the key IP address, the optional name for the entry and the optional id for
the entry.
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1. In the to-be-written "obtain local network access permission" algorithm, we should probably get and set permission store entries (using the appropriate algorithms from the permissions spec) with the right keys. We can explain the keying schema in plain English for readability too.

@johnathan79717
Copy link
Collaborator

Don't forget to replace all "private network" with "local network" too.

@yelhouti
Copy link

yelhouti commented Apr 6, 2023

In section 6.2 it is mentioned that:

The CORS restrictions added by the proposal in this document do not obviate mixed content checks [MIXED-CONTENT-2]. Device consent obtained through a CORS preflight request is necessary but not sufficient.

I have a use case where a legacy hardware device (payment terminal) is in my private network. I would like my public webapp to be able to communicate with it using websocket but it doesn't support wss:// . What I do today is run a proxy locally that forwards request back and fort, this is not ideal as you need to manually install the proxy on the device. Ideally the prompt should take priority over CORS here.

@letitz
Copy link
Collaborator

letitz commented Apr 6, 2023

@yelhouti thanks for the feedback! This is actually the point of the prompt: we want to allow secure websites to bypass mixed content restrictions given user permission. The device would still have to respond to preflight requests with new headers, but it would not need to be served over HTTPS. Would that satisfy your use case?

@yelhouti
Copy link

yelhouti commented Apr 6, 2023

The device would still have to respond to preflight requests with new headers, but it would not need to be served over HTTPS.

@letitz Maybe I wasn't clear, but I have no control over the payment terminal or it's firmware. So it won't be able to answer the preflight request as expected.

@letitz
Copy link
Collaborator

letitz commented Apr 6, 2023

Ah, that's unfortunate. PNA as currently envisioned does not allow for such accesses, as they are indistinguishable from attacks.

Browsers are free to introduce mechanisms allowing users to disable PNA selectively on certain websites, though. Maybe that's enough of an escape hatch for your purposes?

@yelhouti
Copy link

yelhouti commented Apr 6, 2023

PNA as currently envisioned does not allow for such accesses, as they are indistinguishable from attacks.

Well I disagree.
The app running in the client could prompt for a permission to access a specific device on the network.

Browsers are free to introduce mechanisms allowing users to disable PNA selectively on certain websites, though. Maybe that's enough of an escape hatch for your purposes?

That is too big of an escape hatch/risk. as a user I would want the app to be able to talk to only one device but not my router...

@letitz
Copy link
Collaborator

letitz commented Apr 6, 2023

PNA as currently envisioned does not allow for such accesses, as they are indistinguishable from attacks.

Well I disagree. The app running in the client could prompt for a permission to access a specific device on the network.

Right, but one design principle we've held fast to in PNA is that users are ill-positioned to make that kind of judgment call. In other words, we do not believe that only introducing a prompt would meaningfully protect users from attacks.

Browsers are free to introduce mechanisms allowing users to disable PNA selectively on certain websites, though. Maybe that's enough of an escape hatch for your purposes?

That is too big of an escape hatch/risk. as a user I would want the app to be able to talk to only one device but not my router...

That's true! I think the way we're seeing this is that the device should allow the fetch, in that case.

@yelhouti
Copy link

yelhouti commented Apr 6, 2023

Right, but one design principle we've held fast to in PNA is that users are ill-positioned to make that kind of judgment call. In other words, we do not believe that only introducing a prompt would meaningfully protect users from attacks.

@letitz thanks for the clarification, I kind of agree with the statement. The threat model here if I understand it correctly is that the webapp is malicious or compromised right ?

Maybe I compromise could to prompte the user and ask to manually enter the IP he wants to allow instead of just clicking allow. Maybe doing all this in a different page (ex: chrome://pna-allow-list ) with a clear warning:

That's true! I think the way we're seeing this is that the device should allow the fetch, in that case.

End users do not have control over the firmware of all their devices, specially the ones built/designed in the pre-cloud era.

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.

4 participants