Remove GET requests #445

bifurcation · 2018-08-30T15:24:05Z

This is one option for resolving the privacy concerns raised in Adam's and Ben's AD reviews.

felixfontein · 2018-08-30T17:38:54Z

draft-ietf-acme-acme.md

+send a POST request with a JWS body as described above, with the
+following differences:
+
+* The JWS Protected Header MUST contain a "typ" field with the value


Shouldn't it be "type", not "typ"?

I noticed that there exists a JWS header called "typ", but that's supposed to be a media type. Is it ok to redefine "typ" for ACME?

https://tools.ietf.org/html/rfc7515#section-4.1.9

@adamroach pointed out to me that the typ header parameter is tied to media types, though, so we'll have to pick a different signal here. Because application/GET isn't a thing.

felixfontein · 2018-08-30T17:40:14Z

draft-ietf-acme-acme.md

@@ -790,7 +806,7 @@ externalAccountRequired (optional, boolean):
 new-account requests include an "externalAccountBinding" field associating the
 new account with an external account.

-Clients access the directory by sending a GET request to the directory URL.
+Clients access the directory by sending a POST-as-GET request to the directory URL.


This should be GET, as this is about the directory (which is explicitly mentioned as GET above).

Yep. Overzealous search-and-replace :)

jsha · 2018-08-30T23:59:41Z

I think we don't need to require a "typ" field. I think we just need to update the semantics for each resource so that POSTs are always allowed, and a POST with the literal contents {} is a request to echo the current contents of that resource. This is what we already do for retrieving account objects, so we should mirror that.

kaduk · 2018-08-31T00:54:31Z

draft-ietf-acme-acme.md

+send a POST request with a JWS body as described above, with the
+following differences:
+
+* The JWS Protected Header MUST contain a "typ" field with the value


https://tools.ietf.org/html/rfc7515#section-4.1.9

kaduk · 2018-08-31T00:57:01Z

draft-ietf-acme-acme.md

+following differences:
+
+* The JWS Protected Header MUST contain a "typ" field with the value
+  "GET"


Don't we need IANA considerations to register this header parameter?

No; in fact, I chose typ because it was already registered. However, as noted above, that'll need to change.

Somehow I thought we had a registry of values that the "typ" field could take. Which, I guess we do, if it's media types, but I was clearly confused about it.

kaduk · 2018-08-31T00:58:03Z

draft-ietf-acme-acme.md

+| Poll for status       | POST-as-GET order          | 200            |
+| Finalize order        | POST order finalize        | 200            |
+| Poll for status       | POST-as-GET order          | 200            |
+| Download certificate  | POST order certificate     | 200            |


I'm feeling particularly dense today (jet lag), but why is this POST and not POST-as-GET?

cpu · 2018-08-31T13:36:09Z

draft-ietf-acme-acme.md

+    "alg": "ES256",
+    "kid": "https://example.com/acme/acct/1",
+    "nonce": "uQpSjlRb4vQVCjVYAyyUWg",
+    "url": "https://example.com/acme/new-authz",


This "url" header value is not correct for a POST to /acme/cert/asdf

cpu · 2018-08-31T13:36:50Z

draft-ietf-acme-acme.md

+    "alg": "ES256",
+    "kid": "https://example.com/acme/acct/1",
+    "nonce": "uQpSjlRb4vQVCjVYAyyUWg",
+    "url": "https://example.com/acme/new-authz",


This "url" header value is not correct for a POST to /acme/authz/1234

cpu · 2018-08-31T13:37:26Z

draft-ietf-acme-acme.md

+    "alg": "ES256",
+    "kid": "https://example.com/acme/acct/1",
+    "nonce": "uQpSjlRb4vQVCjVYAyyUWg",
+    "url": "https://example.com/acme/new-authz",


This "url" header value is not correct for a POST to /acme/authz/1234

cpu · 2018-08-31T13:51:11Z

I think we don't need to require a "typ" field. I think we just need to update the semantics for each resource so that POSTs are always allowed, and a POST with the literal contents {} is a request to echo the current contents of that resource. This is what we already do for retrieving account objects, so we should mirror that.

@jsha the POST to initiate a challenge throws a 🔧 into this plan. Specifically the server needs a way to differentiate a POST to an authorization's challenge as a POST to initiate the challenge validation process or a POST-AS-GET to poll the challenge. Both types of POST would be using a {} body and without the typ header or equivalent I don't think it would be possible to know which was intended.

One option is to change the "start the challenge" POST to include a marker field in the body instead of using a "typ" header in the JWS and reserving the empty body as a marker for POST-AS-GET.

cpu

Few more comments.

I think "7.3.7. Account Deactivation" needs to be updated to make it clear that under this new proposed authenticated POST-as-GET scheme you would lose access to all authorization, challenge, order and certificate information associated with the account that is deactivated. Previously you could continue to GET all of these resources post-deactivation and this may be a surprising outcome for folks that aren't aware of it before deactivating (especially since ACME does not provide a mechanism to "re-activate").

cpu · 2018-08-31T13:54:48Z

draft-ietf-acme-acme.md

+
+* The JWS Protected Header MUST contain a "typ" field with the value
+  "GET"
+* The payload MUST be an empty JSON object ({})


What should the server return to the client if this isn't true? A urn:ietf:params:acme:error:unauthorized type problem? urn:ietf:acme:error:malformedRequest?

cpu · 2018-08-31T13:55:16Z

draft-ietf-acme-acme.md

+
+We will refer to these as "POST-as-GET" requests. On receiving such
+a request, the server MUST authenticate the sender and verify any
+access control rules.  Otherwise, the server MUST treat this request


What should the server return to the client if the POST-as-GET authentication fails? A urn:ietf:params:acme:error:unauthorized type problem? urn:ietf:acme:error:malformedRequest?

cpu · 2018-08-31T14:03:17Z

draft-ietf-acme-acme.md

+Otherwise, if a client wishes to fetch a resource from
+the server (which would otherwise be done with a GET), then it MUST
+send a POST request with a JWS body as described above, with the
+following differences:


What should the server return if it receives a GET to a resource? SHOULD (MUST?) return a urn:ietf:acme:error:malformedRequest problem with status 405?

cpu

@bifurcation Thanks for the iteration. It's looking better. Few small things to flag.

cpu · 2018-08-31T16:13:59Z

draft-ietf-acme-acme.md

+Otherwise, the server MUST treat this request as having the same
+semantics as a GET request for the same resource.
+
+The server MUST allow GET requests for the following resources(see


typo: resources(see) -> resources (see)

cpu · 2018-08-31T16:14:35Z

draft-ietf-acme-acme.md

+
+The server MUST allow GET requests for the following resources(see
+{{resources}}, in addition to POST-as-GET requests for these
+resources:


nit: I think the second "these resources" in this sentence is redundant.

cpu · 2018-08-31T16:14:56Z

draft-ietf-acme-acme.md

+* The newNonce resource
+* Certificate resources
+
+Allowing GET requests for the directory and newNonce resourcse


typo: "resourcse" -> "resource"

cpu · 2018-08-31T16:15:43Z

draft-ietf-acme-acme.md

@@ -1616,7 +1639,9 @@ orders or authorization transactions based on a change of account key.

 A client can deactivate an account by posting a signed update to the server with
 a status field of "deactivated." Clients may wish to do this when the account
-key is compromised or decommissioned.
+key is compromised or decommissioned. A deactivated account an no longer request


typo: "an no longer" -> "can no longer"

cpu · 2018-08-31T16:16:14Z

draft-ietf-acme-acme.md

+    "url": "https://example.com/acme/authz/1234",
+    "typ": "GET"
+  }),
+  "payload": base64url({}),


Shouldn't this be "payload": null, now since its replacing a GET to this authz with a POST-as-GET?

I think this should actually be "payload": "", since JSON distinguishes between an empty string (or equivalently, zero-length payload) and the null value.

cpu · 2018-08-31T16:17:02Z

draft-ietf-acme-acme.md

+    "url": "https://example.com/acme/authz/1234",
+    "typ": "GET"
+  }),
+  "payload": base64url({}),


ditto here RE: the null payload.

This is an experimental implementation of the acme draft protocol change proposed in ACME PR 445[0]. When Pebble is run with `-strict` it will no longer allow unauthenticated GET requests to Orders, Authorizations or Challenges. Support is added for "POST-as-GET" requests as an authenticated alternative. [0] - ietf-wg-acme/acme#445

felixfontein · 2018-09-01T13:13:51Z

draft-ietf-acme-acme.md

+certificate chain.  (See {{?I-D.ietf-acme-star}} for more advanced
+cases.)  A server that allows GET requests for certificate resources
+can still provide them a degree of access control by assigning them
+capability URLs them capability URLs


"them capability URLs" is there twice.

Also please add a reference for "capability URLs", https://www.w3.org/TR/capability-urls/ (the same reference that's listed below).

Suggest to add: The server MAY choose to allow GET requests to certain certificate resources but not to others. The server can base this decision on out-of-band knowledge (e.g., to allow GET requests to certificates owned by a certain account) or on order-specific information, such as the extension defined in {{?I-D.ietf-acme-star}}.

yaronf · 2018-09-01T18:04:51Z

draft-ietf-acme-acme.md

+| Poll for status       | POST-as-GET order          | 200            |
+| Finalize order        | POST order finalize        | 200            |
+| Poll for status       | POST-as-GET order          | 200            |
+| Download certificate  | GET  order certificate     | 200            |


Should the "Download certificate" row be "GET or POST-as-GET"?

yaronf · 2018-09-01T18:06:46Z

draft-ietf-acme-acme.md

+
+For example, suppose that the CA uses highly structured URLs that
+with several low-entropy fields:
+


The "For example" sentence above is incomplete.

draft-ietf-acme-acme.md

bifurcation · 2018-09-06T15:21:03Z

@yaronf Which case do you mean? GETs are clearly allowed for certificate resources, just at the MAY level.

yaronf · 2018-09-06T15:34:29Z

I was referring to the "Downloading the Certificate" section. After rereading the whole thing again, I realize that GET is still allowed. But still, for clarity, we should mention this fact again in this section.

…

On 06/09/18 18:21, Richard Barnes wrote: @yaronf <https://github.com/yaronf> Which case do you mean? GETs are clearly allowed for certificate resources, just at the MAY level. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#445 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABNP5IyUMtu68wsuHkgdhKUOFD9gUJtwks5uYT1_gaJpZM4WTsq7>.

richsalz · 2018-09-06T15:48:33Z

I think we need to give the WG a chance to object to this. Yes there has been some discussion, but I'll do a formal 'call' email. Don't let that delay merging, we can always revert. :)

cpu · 2018-09-14T16:00:54Z

draft-ietf-acme-acme.md

+for these resources.  This enables clients to bootstrap into the
+ACME authentication system.
+
+The server MAY allow GET requests for certificate resources, in


minor nit: I don't think the comma after "resources" is required here.

cpu · 2018-09-14T16:01:24Z

draft-ietf-acme-acme.md

+process, e.g., the web server that will use the referenced
+certificate chain.  (See {{?I-D.ietf-acme-star}} for more advanced
+cases.)  A server that allows GET requests for certificate resources
+can still provide them a degree of access control by assigning them


nit: "can still provide them a degree" -> "can still provide a degree"

cpu · 2018-09-14T16:04:35Z

draft-ietf-acme-acme.md

-key is compromised or decommissioned.
+key is compromised or decommissioned. A deactivated account can no longer request
+certificate issuance or access resources related to the account, such as orders
+or authorizations.


Can you add one more sentence that explicitly says "If a server receives a POST or POST-as-GETs from a deactivated account it MUST return a urn:ietf:params:acme:error:unauthorized problem"?

kaduk · 2018-09-21T22:29:57Z

draft-ietf-acme-acme.md

 order to allow certificates to be fetched by a lower-privileged
 process, e.g., the web server that will use the referenced
 certificate chain.  (See {{?I-D.ietf-acme-star}} for more advanced
 cases.)  A server that allows GET requests for certificate resources
-can still provide them a degree of access control by assigning them
+can still provide a degree of access control by assigning them
 capability URLs them capability URLs {{?W3C.WD-capability-urls-20140218}}.


"them capability URLs" they really are a-happen-ing ... twice.

This is an implementation of the acme draft protocol change added in ACME PR 445[0]. When Pebble is run with `-strict` it will no longer allow unauthenticated GET requests to Orders, Authorizations or Challenges. Support is added for "POST-as-GET" requests as an authenticated alternative. [0] - ietf-wg-acme/acme#445

Remove GET requests

64f8fb2

felixfontein reviewed Aug 30, 2018

View reviewed changes

kaduk reviewed Aug 31, 2018

View reviewed changes

cpu suggested changes Aug 31, 2018

View reviewed changes

cpu reviewed Aug 31, 2018

View reviewed changes

Revisions in response to WG comments

8afc0a3

This was referenced Aug 31, 2018

Adjusting example to match new text about potentially sensitive resources #444

Closed

Comments from Adam Roach's and Ben Kaduk's IESG reviews #443

Closed

cpu suggested changes Aug 31, 2018

View reviewed changes

cpu mentioned this pull request Aug 31, 2018

Remove unauthenticated GET in -strict. letsencrypt/pebble#162

Merged

bifurcation added 2 commits August 31, 2018 17:59

Make GET optional for certs; add URL construction guidance

eeff10e

Merge branch 'master' into no-get

42f7dbb

felixfontein reviewed Sep 1, 2018

View reviewed changes

Couple of minor bugs noted by @cpu

5d56e38

yaronf reviewed Sep 1, 2018

View reviewed changes

draft-ietf-acme-acme.md Show resolved Hide resolved

bifurcation added 2 commits September 6, 2018 10:59

Revisions in response to WG discussion

ec48bca

Typo

9589939

cpu suggested changes Sep 14, 2018

View reviewed changes

bifurcation added 2 commits September 21, 2018 18:25

Further clarifications

ecf58c6

Merge branch 'master' into no-get

8d34888

Merge branch 'master' into no-get

a703f71

kaduk reviewed Sep 21, 2018

View reviewed changes

One more typo

e46cc3d

cpu approved these changes Sep 22, 2018

View reviewed changes

bifurcation merged commit aca1171 into master Sep 24, 2018

bifurcation mentioned this pull request Oct 2, 2018

Randomize URLs in examples #455

Merged


		For example, suppose that the CA uses highly structured URLs that
		with several low-entropy fields:

Remove GET requests #445

Remove GET requests #445

Conversation

bifurcation commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsha commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Aug 31, 2018 • edited Loading

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaronf Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bifurcation commented Sep 6, 2018

yaronf commented Sep 6, 2018 via email

richsalz commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpu commented Aug 31, 2018 •

edited

Loading

cpu Aug 31, 2018 •

edited

Loading

yaronf Sep 1, 2018 •

edited

Loading