HTTP signatures #4146

Gargron · 2017-07-10T19:35:27Z

Spec: https://tools.ietf.org/html/draft-cavage-http-signatures-06

Access control on Atom resource URLs can be implemented with this spec.

TODO:

Class to sign requests (Request)
Parsing and verifying signatures in controllers (SignatureVerification)
Replace direct usage of HTTP with new Request class
Test positive cases
Test cases when request has been tampered with (signature does not match)

Potential uses in the future:

Fix private toot delivery to instances where LOCAL_DOMAIN != WEB_DOMAIN, by authenticating PuSH subscriptions and storing the signed account's "domain" along the callback_url (Fix PuSH to instances where WEB_DOMAIN != LOCAL_DOMAIN #2739 (comment))
General use in ActivityPub when fetching entities

Spec: https://tools.ietf.org/html/draft-cavage-http-signatures-06

Accidentally fix lease_seconds not being set and sent properly, and change the new minimum subscription duration to 1 day

ClearlyClaire

Worried about how signed date HTTP headers are handled, and about handling of multiple headers with the same fields (although it seems like Mastodon-originating requests won't have multiple occurrences of the same header field). Seems ok otherwise.

ClearlyClaire · 2017-07-13T13:35:10Z

app/controllers/concerns/signature_verification.rb

+      if signed_header == Request::REQUEST_TARGET
+        "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}"
+      elsif signed_header == 'date'
+        "date: #{Time.now.utc.httpdate}"


Wait, am I missing something? How can there be any chance that Time.now.utc matches the request's Date field? I don't think this should be special-cased at all.

How else are you supposed to prevent replay attacks? You need to check that a request that says it comes at a specific time did come at the specific time. This is an honest question - the spec itself is extremely blurry about this, though it does mandate the "date" header for some kind of verification.

The Date header could indeed be used to avoid replay attacks, but it is completely out of this draft's scope (the Date header is actually a SHOULD, not a MUST). It is very unlikely that the request's Date header matches the time it is processed on the server down to the second.

If you are worried about replay attacks, you could verify that the difference between the time of the Date header and the time it is processed does not exceed some reasonable bound. I'm not sure what a reasonable bound would be, as many servers are slightly out of date and do not run ntp.

So signing the date header makes sense to make every signature different, but verifying its value is not important, is that right? We could give it like a minute drift window, I think that's what TOTP does.

Well, I guess the reason they recommend signing Date is to do that kind of verification, but it's a separate step.

ClearlyClaire · 2017-07-13T13:41:10Z

app/controllers/concerns/signature_verification.rb

+      elsif signed_header == 'date'
+        "date: #{Time.now.utc.httpdate}"
+      else
+        "#{signed_header}: #{request.headers[to_header_name(signed_header)]}"


I am not confident you are handling “duplicate” headers correctly.

Cf. point 2.3.2. of https://tools.ietf.org/html/draft-cavage-http-signatures-06

ClearlyClaire · 2017-07-13T13:42:21Z

app/controllers/concerns/signature_verification.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+module SignatureVerification


The whole module should probably be documented with the spec you are implementing, especially since it is not standard. In the review, I will assume you are implementing https://tools.ietf.org/html/draft-cavage-http-signatures-06

ClearlyClaire · 2017-07-13T13:45:44Z

app/lib/request.rb

+  end
+
+  def signed_headers
+    @headers.keys.join(' ').downcase


While I can't see any particular issue with that, I am not convinced signing all headers is useful.

I mean, by default we only have user-agent, date and host headers. Places that make use of add_headers only add headers that are required by e.g. spec. So I don't think there is an issue here.

I see no use for stuff like user-agent to be signed, but I have no real opposition to it either, so I'm fine with this.

ClearlyClaire · 2017-07-13T13:55:37Z

app/workers/pubsubhubbub/confirmation_worker.rb

-    HTTP.headers(user_agent: 'Mastodon/PubSubHubbub')
-        .timeout(:per_operation, write: 20, connect: 20, read: 50)
-        .get(subscription.callback_url, params: callback_params)
+    Request.new(:get, subscription.callback_url, params: callback_params).perform


You are effectively changing the timeout parameters here. Not sure if it's a problem, though.

I am unifying them in one place, which I think is the primary benefit of this. Also, letting a request stall for 90 seconds wasn't good behaviour, I think. We'd rather fail and retry later.

ClearlyClaire · 2017-07-13T13:55:59Z

app/workers/pubsubhubbub/delivery_worker.rb

-    HTTP.timeout(:per_operation, write: 50, connect: 20, read: 50)
-        .headers(headers)
-        .post(subscription.callback_url, body: payload)
+    request = Request.new(:post, subscription.callback_url, body: payload)


Same thing about timeouts.

Gargron · 2017-07-14T00:00:19Z

@ThibG Addressed date issue and documented draft. Don't think duplicate headers should occur in our use case.

ClearlyClaire · 2017-07-14T07:57:27Z

@Gargron I'm still worried a 30s window is much too tight: I routinely see time discrepancies of the order of minutes between Mastodon instances. Furthermore, I think at least in the case of PuSH subscriptions, replay attacks are a non-issue. Can't really say for ActivityPub, though.

Regarding multiple headers for the same field, it will not be an issue between Mastodon instances, but this means you don't actually verify signatures according to the spec, and any software that would sign multiple headers of the same name for whatever reason won't correctly interoperate with Mastodon.

Gargron · 2017-07-14T09:19:51Z

@ThibG I am worried that if someone gets their hands on such requests in real time, they could replay them and the server would genuinely respond to them, within that time window. If we want to use this for access control, that's risky. That being said, SSL should protect the requests from being caught by a 3rd party, so I don't know if the risk is real.

ClearlyClaire · 2017-07-14T15:08:01Z

@Gargron well, to be honest, there are things I don't get with the spec itself: why doesn't it cover signing the request data, but only headers? That doesn't really protect you from MITM attacks that would just change the request data while keeping the request headers intact.

Regarding replay attacks, as you said, TLS should protect the requests from being caught by a 3rd party. And if not, you have worse problems (namely, the aforementioned one, or tampering with the key exchange in the first place, etc.)

Gargron · 2017-07-14T15:17:31Z

why doesn't it cover signing the request data, but only headers?

@ThibG It delegates that to the Digest header from a different spec

ClearlyClaire · 2017-07-14T15:27:53Z

@Gargron thanks, I missed that, that makes sense! So, assuming you follow RFC 3230 and sign the Digest header, replay attacks are just replay attacks. How much of an issue they are depends on the kind of messages you're dealing with, and I would argue they are not much of an issue for things like PuSH subscriptions. I think I would make the validity window an optional parameter, make it default to something like a few minutes, and specifically log requests that fail validating because of this.

Gargron · 2017-07-14T15:56:24Z

I think I would make the validity window an optional parameter, make it default to something like a few minutes, and specifically log requests that fail validating because of this.

@ThibG I'd like to leave it as-is. Nothing right now actually makes use of the value yet, and even if verification fails, all we get is the status quo, it's not like the request gets denied. We'll have time to figure this out in future PRs.

ClearlyClaire · 2017-07-14T17:21:53Z

@Gargron I would still argue it should be an optional parameter, and such failures should be logged. As for my other gripe with this PR, the handling of multiple headers sharing the same field-name… it seems to me it would be quite complicated to get it right with RoR, which joins such values with “, ” (which is sound with regards to the HTTP specs, but is a problem for the HTTP Signature one). That should probably be explained somewhere in this module.

Gargron · 2017-07-14T17:57:01Z

app/services/subscribe_service.rb

+      'hub.callback': api_subscription_url(@account.id),
+      'hub.verify': 'async',
+      'hub.secret': @account.secret,
+      'hub.lease_seconds': 30.days.seconds,


This is actually a change, I think, because through a bug it was 7 days previously. I think we might want to keep it to 7 days instead of 30, because while 30 is more efficient, 7 means less "black hole" time if somebody messes up and loses their DB with all subscription data.

Gargron · 2017-07-14T17:59:59Z

@ThibG

it seems to me it would be quite complicated to get it right with RoR, which joins such values with “, ”

From the spec:

If there are multiple instances of the same header field, all header field values associated with the header field MUST be concatenated, separated by a ASCII comma and an ASCII space , , and used in the order in which they will appear in the transmitted HTTP message

So it works out of the box?

ClearlyClaire · 2017-07-14T18:10:09Z

@Gargron ah yes, sorry, indeed, it should work out of the box then! So the only change I'm suggesting now is the time window as parameter, and some logging when it doesn't match the window.

as PuSH subscription duration (which was previous default due to a bug)

Gargron · 2017-07-14T18:37:08Z

@ThibG Fixed!

* Add Request class with HTTP signature generator Spec: https://tools.ietf.org/html/draft-cavage-http-signatures-06 * Add HTTP signature verification concern * Add test for SignatureVerification concern * Add basic test for Request class * Make PuSH subscribe/unsubscribe requests use new Request class Accidentally fix lease_seconds not being set and sent properly, and change the new minimum subscription duration to 1 day * Make all PuSH workers use new Request class * Make Salmon sender use new Request class * Make FetchLinkService use new Request class * Make FetchAtomService use the new Request class * Make Remotable use the new Request class * Make ResolveRemoteAccountService use the new Request class * Add more tests * Allow +-30 seconds window for signed request to remain valid * Disable time window validation for signed requests, restore 7 days as PuSH subscription duration (which was previous default due to a bug)

Gargron added api REST API, Streaming API, Web Push API requires in depth work in progress Not to be merged, currently being worked on activitypub Protocol-related changes, federation labels Jul 10, 2017

Gargron added 5 commits July 13, 2017 00:39

Add Request class with HTTP signature generator

b19c876

Spec: https://tools.ietf.org/html/draft-cavage-http-signatures-06

Add HTTP signature verification concern

2bd8b0e

Add test for SignatureVerification concern

ed31dc4

Add basic test for Request class

c0fb574

Make PuSH subscribe/unsubscribe requests use new Request class

d2dbb4d

Accidentally fix lease_seconds not being set and sent properly, and change the new minimum subscription duration to 1 day

Gargron force-pushed the feature-http-signatures branch from b1775c4 to d2dbb4d Compare July 12, 2017 23:28

Gargron added 2 commits July 13, 2017 01:57

Make all PuSH workers use new Request class

de36982

Make Salmon sender use new Request class

0eb1957

Gargron force-pushed the feature-http-signatures branch from 8310a56 to 0eb1957 Compare July 13, 2017 00:13

Gargron added 5 commits July 13, 2017 02:26

Make FetchLinkService use new Request class

c23818a

Make FetchAtomService use the new Request class

4978f86

Make Remotable use the new Request class

0385165

Make ResolveRemoteAccountService use the new Request class

9ec5943

Add more tests

9386ac6

Gargron removed the work in progress Not to be merged, currently being worked on label Jul 13, 2017

Gargron requested a review from ClearlyClaire July 13, 2017 00:57

ClearlyClaire requested changes Jul 13, 2017

View reviewed changes

Allow +-30 seconds window for signed request to remain valid

52da940

Gargron commented Jul 14, 2017

View reviewed changes

Disable time window validation for signed requests, restore 7 days

3058437

as PuSH subscription duration (which was previous default due to a bug)

ClearlyClaire approved these changes Jul 14, 2017

View reviewed changes

Gargron merged commit 1618b68 into master Jul 14, 2017

Gargron deleted the feature-http-signatures branch July 14, 2017 18:41

Gargron mentioned this pull request Jul 15, 2017

Proposal for improving private toot security #4208

Closed

clworld mentioned this pull request Sep 20, 2017

Measures to time out when waiting for startup of an instance running on Passenger. #5024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP signatures #4146

HTTP signatures #4146

Gargron commented Jul 10, 2017 •

edited

Loading

ClearlyClaire left a comment

ClearlyClaire Jul 13, 2017

Gargron Jul 13, 2017

ClearlyClaire Jul 13, 2017

Gargron Jul 13, 2017

ClearlyClaire Jul 13, 2017

ClearlyClaire Jul 13, 2017

ClearlyClaire Jul 13, 2017

ClearlyClaire Jul 13, 2017

Gargron Jul 13, 2017

ClearlyClaire Jul 13, 2017

ClearlyClaire Jul 13, 2017

Gargron Jul 13, 2017

ClearlyClaire Jul 13, 2017

ClearlyClaire Jul 13, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

		@@ -0,0 +1,77 @@
		# frozen_string_literal: true

		module SignatureVerification

HTTP signatures #4146

HTTP signatures #4146

Conversation

Gargron commented Jul 10, 2017 • edited Loading

ClearlyClaire 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

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

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Choose a reason for hiding this comment

Gargron commented Jul 14, 2017

ClearlyClaire commented Jul 14, 2017

Gargron commented Jul 14, 2017

Gargron commented Jul 10, 2017 •

edited

Loading