Apply HMAC signatures to upstream requests #147

mbland · 2015-09-29T22:23:27Z

Closes #146.

This still a work-in-progress, as I need to add tests to verify that the actual signing of requests by the proxy is implemented properly. I will remove the "[WIP]" from the title and ping when that's done.

However, the signature package itself and the updating of options to include the new signature_key and upstream_keys configuration values is tested and complete.

mbland · 2015-10-01T00:36:16Z

OK @jehiah, this is ready for a look-see. I haven't squashed down the commits yet, just in case I end up wanting/needing to tweak something. Also, the last commit is a broader change to allow for more algorithms besides sha1; all the commits before that hardcode sha1.

Note that signature.ValidateRequest should be immediately usable by Go programs wishing to validate signed requests (I think go get will do the right thing). Also, all of StringToSign, RequestSignature, and ValidateRequest should be easy to implement in other languages; I plan to write a Ruby gem and an npm if and when this change makes it in.

mbland · 2015-10-02T00:56:15Z

I've gotten the jump on the Ruby and Node implementations; as expected, they were more or less straight ports of signature/signature.go:

https://rubygems.org/gems/oauth2_proxy_authentication
https://www.npmjs.com/package/oauth2-proxy-authentication

I'll get on the Python version soon. Of course, I stand ready to update these and share maintainership based on the outcome of this PR.

mbland · 2015-10-02T15:33:44Z

So I went on a tear, and figured out how generic this HMAC signature logic could be. I've extracted the former signature package into github.com/18F/hmacauth, and also created:

https://rubygems.org/gems/hmac_authentication
https://www.npmjs.com/package/hmac-authentication

So now the headers could even be configurable between instances.

jehiah · 2015-10-02T15:57:59Z

@mbland I'm excited for this PR. It certainly adds a few new configuration options for oauth2proxy.

Some general feedback on a first read:

I'm not convinced there is a compelling need to have separate signatures/secrets for each potential upstream. It feels like it would be much simpler (in implementation and usage) to just have --signature-"sha1:s3kr3t" and apply that to all upstream requests. Thoughts?
The signature in github.com/a18F/hmacauth is calculated with header.Get() but we need all the headers of a particular header in case they are repeated. I think this also implies we need output the header key as part of the signature to disambiguate where values came from.
The signature has the full URL and i'm pretty sure that means it's requires that you must use -pass-host-header=true (the default). I think this is ok, but it's worth documenting as otherwise the upstream wouldn't have enough information to re-construct the signature.
Header order. Would it be useful to just define that the headers are signed in sorted order?

mbland · 2015-10-02T17:34:48Z

Hey @jehiah:

I've removed the support for per-upstream secret keys. I've saved a patch in case we ever want to add it back.
Looking more closely at the documentation for Header.Get(), I see now that it says it will return the "first" value. I'll update the 18F/hmacauth package.
Actually, the intent is to pass just the path + query, so pass-host-header shouldn't be necessary. Again, I'll update 18F/hmacauth.
So you're saying just sort all the headers by name and hash them all? Interesting approach that wouldn't even require a configuration of which headers to pick. Can't think of a downside offhand.

As a government employee, I can't solicit effort, but I'll copy you on the 18F/hmacauth PR(s). 😉

jehiah · 2015-10-02T17:40:56Z

oops, no i wasn't implying to sort all headers, i was implying we sort the order of the headers we are hashing. english--

mbland · 2015-10-02T17:41:54Z

Ah, got it. I'll keep the header config bit, then.

@jehiah

Per @jehiah's suggestion in bitly/oauth2_proxy#147. Also adds a test to ensure that the query string of the URL is factored in.

mbland · 2015-10-05T15:52:28Z

FYI, I managed to get a generic proxy server together based on the 18F/hmacauth package. Check it out: https://github.com/18F/hmacproxy/

However, I don't know why the build is breaking; it somehow can't find 18F/hmacauth, even though it's public and specified in the Godeps file. Any hints, @jehiah?

mbland · 2015-10-06T03:03:20Z

What d'ya know, it's passing again. :-) @jehiah So long as you're comfortable with the implementation of github.com/18F/hmacauth, this may be ready for another look.

But to pick up two points from before:

I've implemented the hashing of multiply-defined headers in the hmacauth package, as well as in the Node.js HMAC auth implementation, but apparently that's a discouraged practice, and Ruby unilaterally selects the value from the last instance of a header (see the notes in the README from the Ruby HMAC auth implementation.
I've not sorted the headers for hashing, but I'm really on the fence about that. If you think it's really worth it, I can update hmacauth (and the other implementations) accordingly.

mbland · 2015-10-10T14:44:16Z

I just pushed a commit to simulate a request body buffer consumed by Read(), which is why the test is currently failing.

Using fakeNetConn in the test exposes a bug in 18F/hmacauth when handling POST requests, addressed by 18F/hmacauth#4. The bug was that the strings.Reader does not consume its buffer contents the same way that a net.Conn does. So the test would pass because its request body would still be intact after signing, but during live testing, the request body would be consumed by HmacAuth.requestSignature().

It also happened to expose a subsequent 18F/hmacauth bug addressed in 18F/hmacauth#5. It turns out that checking Request.ContentLength is an unreliable way of detecting that a body is present, and checking body != nil is sufficient.

18F/hmacauth#4 is already merged; when 18F/hmacauth#5 is in, I'll tag 18F/hmacauth v1.0.1 and update the Godeps to use that version, at which point the test should pass.

cc: @dlapiduz @jcscottiii

mbland · 2015-10-13T13:35:25Z

OK, hmacauth bumped to v1.0.1 and the test is passing again.

jehiah · 2015-10-13T13:37:38Z

@mbland 😄 I think this is in good shape now! Sorry i've been silent here over the past week. I should be able to help wrap this up today

mbland · 2015-10-13T13:50:33Z

No worries, and no rush! 😄

mbland · 2015-11-09T16:12:10Z

@jehiah Also rebased this branch after #164.

Using fakeNetConn in the test exposes a bug in 18F/hmacauth when handling POST requests, addressed by 18F/hmacauth#4. The bug was that the strings.Reader does not consume its buffer contents the same way that a net.Conn does. So the test would pass because its request body would still be intact after signing, but during live testing, the request body would be consumed by HmacAuth.requestSignature(). It also happened to expose a subsequent 18F/hmacauth bug addressed in 18F/hmacauth#5. It turns out that checking Request.ContentLength is an unreliable way of detecting that a body is present, and checking body != nil is sufficient. 18F/hmacauth#4 is already merged; when 18F/hmacauth#5 is in, I'll tag 18F/hmacauth v1.0.1 and update the Godeps to use that version, at which point the test should pass.

v1.0.1 contains 18F/hmacauth#4 and 18F/hmacauth#5, needed to make TestRequestSignaturePostRequest pass again.

mbland · 2015-11-09T16:16:42Z

...and after #153. 😄

jehiah · 2015-11-16T03:09:57Z

merged with some slight readme changes as e4626c1

mbland · 2015-11-17T13:13:03Z

Ah, thanks, @jehiah!

Updates readme and help Adds azure to the providers. Fixes race condition Sometimes, during tests, a race condition occurs. Using `break` instead of `return` fixes this for me. Tries to read mail address Tries to read mail address from the Graph API. Currently this has not been tested properly. Adds resource parameter Uses to gain access to protected resources when redeeming the token. Gets the mail address from the graph *: rename Url to URL everywhere Go coding style says that acronyms should be all lower or all upper. Fix Url to URL. oauthproxy: rename Uri to URI Be consistent with Go coding style for acroynyms. *: rename Oauth to OAuth Be consistent with Go capitalization styling and use a single way of spelling this across the tree. Add /auth endpoint to support Nginx's auth_request Closes bitly#152. Extract Authenticate for Proxy, AuthenticateOnly Add nginx auth_request config to README Sign Upstream requests with HMAC. closes bitly#147 Renames var ending on Url to URL Simplifies configuration for brevity

mbland force-pushed the hmac-signatures branch 6 times, most recently from 50ab403 to e44dfad Compare September 30, 2015 20:23

mbland changed the title ~~[WIP] Apply HMAC signatures to upstream requests~~ Apply HMAC signatures to upstream requests Sep 30, 2015

mbland force-pushed the hmac-signatures branch 3 times, most recently from 16832c7 to 71931f2 Compare October 1, 2015 00:25

mbland pushed a commit to 18F/hmacauth that referenced this pull request Oct 2, 2015

Handle headers that are defined more than once

fc29040

Per @jehiah's suggestion in bitly/oauth2_proxy#147. Also adds a test to ensure that the query string of the URL is factored in.

mbland mentioned this pull request Oct 2, 2015

Handle headers that are defined more than once 18F/hmacauth#1

Merged

jehiah mentioned this pull request Oct 5, 2015

travis: custom install steps to fix build #151

Closed

mbland force-pushed the hmac-signatures branch from 69c4bc4 to 116a68e Compare November 9, 2015 16:10

Mike Bland added 2 commits November 9, 2015 11:13

Signature package for SHA1 HMAC request signatures

99cffc1

Add signature flag options

bae6136

Mike Bland added 18 commits November 9, 2015 11:13

Sign requests before sending upstream

30cc2bf

Extract NewTestProvider

7c4fa19

Tests to ensure upstream can validate signature

f9e1888

Update README with request signature information

43d1788

Allow for other signature algorithms than sha1

db8bc93

Use extracted 18F/hmacauth package

dae3719

Remove support for per-upstream secret keys

32be942

Signature header as a ValidateRequest parameter

ff08ca5

Use hmacauth.HmacAuth type

ecdf593

Use HmacAuth.SignRequest()

25cb80b

Better SIGNATURE_HEADERS decl; eliminate init()

0bcc464

Update Godeps with 18F/hmacauth v0.0.0-pr-1

2bedf21

Update to hmacauth v0.0.1

98af10f

Bump github.com/18F/hmacauth to v1.0.0

e08b9d8

s/Validate/Authenticate/g

742e3fc

Bump hmacauth to v1.0.1

8a0dacf

v1.0.1 contains 18F/hmacauth#4 and 18F/hmacauth#5, needed to make TestRequestSignaturePostRequest pass again.

README update

47696e0

mbland force-pushed the hmac-signatures branch from 116a68e to 47696e0 Compare November 9, 2015 16:15

jehiah closed this in e4626c1 Nov 16, 2015

mbland deleted the hmac-signatures branch November 17, 2015 13:13

stepanstipl pushed a commit to stepanstipl/oauth2_proxy that referenced this pull request Jan 16, 2016

Sign Upstream requests with HMAC. closes bitly#147

22d9b78

ruta-goomba pushed a commit to ruta-goomba/oauth2_proxy that referenced this pull request Jan 21, 2016

Sign Upstream requests with HMAC. closes bitly#147

0e7fea1

mbland mentioned this pull request Jan 29, 2016

Replace MyUSA auth with hmac proxy 18F/dolores-landingham-slack-bot#65

Closed

2 tasks

mbland mentioned this pull request Feb 24, 2016

OAUTH2_PROXY_SIGNATURE_KEY env var, README update #219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply HMAC signatures to upstream requests #147

Apply HMAC signatures to upstream requests #147

mbland commented Sep 29, 2015

mbland commented Oct 1, 2015

mbland commented Oct 2, 2015

mbland commented Oct 2, 2015

jehiah commented Oct 2, 2015

mbland commented Oct 2, 2015

jehiah commented Oct 2, 2015

mbland commented Oct 2, 2015

mbland commented Oct 5, 2015

mbland commented Oct 6, 2015

mbland commented Oct 10, 2015

mbland commented Oct 13, 2015

jehiah commented Oct 13, 2015

mbland commented Oct 13, 2015

mbland commented Nov 9, 2015

mbland commented Nov 9, 2015

jehiah commented Nov 16, 2015

mbland commented Nov 17, 2015

Apply HMAC signatures to upstream requests #147

Apply HMAC signatures to upstream requests #147

Conversation

mbland commented Sep 29, 2015

mbland commented Oct 1, 2015

mbland commented Oct 2, 2015

mbland commented Oct 2, 2015

jehiah commented Oct 2, 2015

mbland commented Oct 2, 2015

jehiah commented Oct 2, 2015

mbland commented Oct 2, 2015

mbland commented Oct 5, 2015

mbland commented Oct 6, 2015

mbland commented Oct 10, 2015

mbland commented Oct 13, 2015

jehiah commented Oct 13, 2015

mbland commented Oct 13, 2015

mbland commented Nov 9, 2015

mbland commented Nov 9, 2015

jehiah commented Nov 16, 2015

mbland commented Nov 17, 2015