mattlevan commented 6 years ago

Problem

The code (likely some part of %eyre, I suppose) which ++post-quay of arvo/lib/oauth2 relies on to send POST requests is not attaching headers correctly.

Background

As part of my arvo docs work, I have been going through the security driver tutorial here, which guides the reader through configuring OAuth2 for use with the GitHub API v3.

Issue Reproduction

Run the instructions from the OAuth2 section of my arvo-finish branch of the docs. The only things I've changed are the URLs to enter into GitHub when you make an OAuth app and the access_token API URL (from https://github.com/login/oauth/api/access_token to the correct https://github.com/login/oauth/access_token).

Network Trace

The reader will notice things running smoothly: |init-oauth2 performs as expected, etc. Upon reaching the part in which the reader is instructed to run +https://api.github.com/user in the dojo, he will receive an appropriately formatted GitHub authorization URL (https://github.com/login/oauth/authorize?state&client_id=&redirect_uri=&scope=client%20admin). The reader's network activity log from the developer console in his browser will look similar to this:

oauth2-github

%bad-json

He will also be met with a response from his urbit, still in the browser, which indicates "bad json":

eyre: execute [%se p=%receive-auth-response q=[~. <|com github|>]]
%need
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[36 3].[36 40]>
~
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[35 3].[36 40]>
[ %bad-json
  [ ~
    [ p=78
        q
      'access_token=<ommitted>&scope=&token_type=bearer'
    ]
  ]
]
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[34 3].[36 40]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[108 3].[108 44]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[107 15].[108 44]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[198 29].[198 43]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[198 22].[198 43]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[198 16].[198 44]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[198 5].[198 45]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[195 5].[198 45]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[194 5].[198 45]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[193 5].[198 45]>
  /~hopfen-binsec-rosnub-dalhes--marbet-sabwyd-ticdeb-darrul/home/0/lib/oauth2
  <[192 17].[198 45]>

We're not receiving JSON from the GitHub API. That's not surprising-- GitHub API v3 requires us to send the header Accept: application/json if we want the response in JSON.

Adding to lib/oauth2

So I started to explore the correct place to add this header. It seems appropriate to me to add this to ++post-quay of lib/oauth2.hoon, so that the new version reads as such:

++  post-quay
  |=  {a/purl b/quay}  ^-  hiss
  =.  b  (quay:hep-to-cab b)
  =-  [a %post - ?~(b ~ (some (tact +:(tail:earn b))))]
  %^  my  
      :+  %accept
        'application/json'
      ~   
      :+  %content-type
        'application/x-www-form-urlencoded'
      ~   
      ~   
::

Please note that HTTP header field names are case insensitive. From what I've gathered, this code should tack a new header onto the POST request and we should accordingly receive JSON back from GitHub. But, it doesn't work!

hiss and httr

Last thing I did was ~& the hiss and httr (I've noted the arms from which they are being printed too):

'request-token hiss'
[ p=[p=[p=%.y q=~ r=[%.y p=<|com github|>]] q=[p=~ q=<|login oauth access_token|>] r=~]
    q
  [ p=%post
    q={[p='accept' q=<|application/json|>] [p='content-type' q=<|application/x-www-form-urlencoded|>]}
      r
    [ ~
      [ p=216
          q
        'code=<actual code ommitted>&client_id=<id ommitted>&client_secret=<secret ommitted>&redirect_uri=http%3A%2F%2Flocalhost%3A8443%2F~%2Fac%2Fgithub.com%2F~.%2Fin&grant_type=authorization_code'
      ]
    ]
  ]
]

'bak-save-token httr'
[ p=200
    q
  ~[
    [p='X-GitHub-Request-Id' q='BF78:6DF8:400EB20:692524F:59EE16EF']
    [p='X-Runtime-rack' q='0.030523']
    [p='X-XSS-Protection' q='1; mode=block']
    [p='X-Frame-Options' q='deny']
    [p='X-Content-Type-Options' q='nosniff']
    [ p='Public-Key-Pins'
        q
      'max-age=5184000; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; pin-sha256="RRM1dGqnDFsCJXBTHky16vi1obOlCgFFn/yOhI/y+ho="; pin-sha256="k2v657xBsOVe1PQRwOsHsw3bsGT2VzIqz5K+59sNQws="; pin-sha256="K87oWBWM9UZfyddvDfoxL+8lpNyoUB2ptGtn0fv6G2Q="; pin-sha256="IQBnNBEiFuhj+8x6X8XLgh01V9Ic5/V3IRQLNFFc7v4="; pin-sha256="iie1VXtL7HzAMF+/PVPR9xzT80kQxdZeJ+zduCB3uj0="; pin-sha256="LvRiGEjRqfzurezaWuj8Wie2gyHMrW5Q06LspMnox7A="; includeSubDomains'
    ]
    [p='Strict-Transport-Security' q='max-age=31536000; includeSubdomains; preload']
    [ p='Content-Security-Policy'
        q
      'default-src \'none\'; base-uri \'self\'; block-all-mixed-content; child-src render.githubusercontent.com; connect-src \'self\' uploads.github.com status.github.com collector.githubapp.com api.github.com www.google-analytics.com github-cloud.s3.amazonaws.com github-production-repository-file-5c1aeb.s3.amazonaws.com github-production-upload-manifest-file-7fdce7.s3.amazonaws.com github-production-user-asset-6210df.s3.amazonaws.com wss://live.github.com; font-src assets-cdn.github.com; form-action \'self\' github.com gist.github.com; frame-ancestors \'none\'; img-src \'self\' data: assets-cdn.github.com identicons.github.com collector.githubapp.com github-cloud.s3.amazonaws.com *.githubusercontent.com; media-src \'none\'; script-src assets-cdn.github.com; style-src \'unsafe-inline\' assets-cdn.github.com'
    ]
    [p='Expect-CT' q='max-age=2592000, report-uri="https://api.github.com/_private/browser/errors"']
    [p='X-Runtime' q='0.026072']
    [p='X-Request-Id' q='a6d0a3b9f96ff8f9e76aa4c6b2050cb0']
    [p='X-UA-Compatible' q='IE=Edge,chrome=1']
    [p='Vary' q='X-PJAX']
    [p='Cache-Control' q='no-cache']
    [p='Status' q='200 OK']
    [p='Transfer-Encoding' q='chunked']
    [p='Content-Type' q='application/x-www-form-urlencoded; charset=utf-8']
    [p='Date' q='Mon, 23 Oct 2017 16:21:03 GMT']
    [p='Server' q='GitHub.com']
  ]
  r=[~ [p=78 q='access_token=<token ommitted>&scope=&token_type=bearer']]
]

It looks like our hiss is okay here, complete with our newly added accept: application/json header. But, we still don't receive JSON back from GitHub upon retry!

So, I decided to go trawling through arvo/eyre.hoon and that's where I'm fairly well stuck.

Commentary

Tagging @ohAitch on this one since I've heard rumors in meta that this is your work! Perhaps you can help. Also tagging @joemfb since he was so helpful in meta. And @keatondunsford because the docs are squarely in his domain! I am just learning what's going on here (editing the docs has been a great way to learn), so please let me know if I'm mistaken anywhere or if I'm missing some crucial knowledge. Looking forward to hearing back so I can finish my edits of security-driver.md and api.md in the docs. Thanks!

belisarius222 commented 6 years ago

@mattlevan I don't think %application/json is valid syntax. I can't type it into the dojo on the master branch, at least. Is there a reason it's structured that way and not surrounded by single quotes like 'application/x-www-form-urlencoded'?

mattlevan commented 6 years ago

@belisarius222 Thanks, my typo there. Same results with 'application/json'.

joemfb commented 6 years ago

I've spent some time reproducing this. It seems that %hiss headers are not actually being used -- not sure if the bug is in %eyre or cttp.c:

Save as %/gen/hiss/hoon (secrets elided):

|=  *
:^    (need (de-purl:html 'https://github.com/login/oauth/authorize'))
    %post
  (my content-type+['application/x-www-form-urlencoded']~ accept+['application/json']~ ~)
%-  some
:-  216
'code=123&client_id=456&client_secret=789&redirect_uri=http%3A%2F%2Flocalhost%3A8443%2F~%2Fac%2Fgithub.com%2F~.%2Fin&grant_type=authorization_code'

> |start %curl
> :curl +hiss ~

Voila

Note: I tried to test locally with netcat, but kept hitting urbit/urbit#784 and urbit/urbit#838. urbit/urbit#882 seemed to be an issue as well.

joemfb commented 6 years ago

I added some printfs to _cttp_heds_list(), and the headers are definitely making it that far. I'm officially out of my depth ...

cgyarvin commented 6 years ago

Very interesting this is. Joe, I take it that you'd call yourself completely confident that this is a cttp bug?

On Tue, Oct 24, 2017 at 8:31 PM, Joe Bryan notifications@github.com wrote:

I added some printfs to _cttp_heds_list(), and the headers are definitely making it that far. I'm officially out of my depth ...

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/arvo/issues/442#issuecomment-339203603, or mute the thread https://github.com/notifications/unsubscribe-auth/AALyAb3ngIYJXUgWpREBf7ArSfJeyfpoks5svquGgaJpZM4QDMyf .

joemfb commented 6 years ago

To be completely confident, I'd need to print the headers in cttp.c and observe the absence of headers in a request to an http server I control. I haven't gotten that to work on localhost -- at least not with netcat. I keep hitting the issues i linked above.

I could add a name to /etc/hosts and test with that, or stand up a server on another machine. I can give it another spin this evening.

joemfb commented 6 years ago

The headers are being passed through, and are part of the request. But there are duplicates:

> =headers (my content-type+['application/x-www-form-urlencoded']~ ['Accept' ['application/json']~] ~)
> :curl [[p=[p=%.n q=[~ u=10.101] r=[%.y p=['localhost' ~]]] q=[p=~ q=~] r=~] %get headers ~]

$ nc -k -l 10101
GET / HTTP/1.1
User-Agent: urbit/vere.0.2
Accept: */*
Connection: Keep-Alive
Host: localhost
content-type: application/x-www-form-urlencoded
Accept: application/json

Confirmed on retrofit and maint.

Duplicate headers are generally frowned upon, but I don't know what real-world usage actually looks like. Maybe github has started parsing requests more strictly? Or merely changed it's default response type?

mattlevan commented 6 years ago

It could very well be that GitHub reads the first accept header (Accept: */*) and says, "Hey looks like he's cool with any format!" and proceeds to send the access token in the urlencoded form. @joemfb That */* header must be hard-coded somewhere, do you have a good idea where or some clues so I can search for where?

BTW, yes, the GitHub default response type is urlencoded form. I've checked this with curl.

joemfb commented 6 years ago

Yeah, just comment this out: https://github.com/urbit/urbit/blob/retrofit/vere/cttp.c#L1512

I just confirmed that was the issue, I've successfully authorized my token and gotten JSON from github.

joemfb commented 6 years ago

That line has been there for awhile ... urbit/urbit@3400b6bacb857301f988ad82a7384ceabaac9f6b

Update: make that all the way back urbit/urbit@cc255a4c496f39818be6346292e63c1229c2c31a

cgyarvin commented 6 years ago

So good to get this fixed! Can you drop a PR?

mattlevan commented 6 years ago

Successfully reproduced @joemfb's findings with urbit retrofit branch and belisarius222/sec-fixes arvo branch as well. Changes to urbit/urbit and urbit/arvo are reflected respectively here: urbit/urbit#890 and urbit/arvo#446.

Here are the working parts:

`cttp.c`

//  _cttp_ccon_fire_str(coc_u, "Accept: */*\r\n"); // line 1512 commented out

After commenting that line out, recompile urbit, boot it with retrofit arvo, and ensure lib/oauth2.hoon and sec/github.hoon are as below and GitHub OAuth2 should now work.

`lib/oauth2.hoon`

::  Test url +https://api.github.com/user
::
::::  /hoon/github/com/sec
  ::
/+    oauth2
!:
::::
  ::
|_  {bal/(bale:eyre keys:oauth2) tok/token:oauth2}  :: note the bale:eyre here, important
++  scopes                          ::  comment out scopes to taste
  :~  'user'  'user:email'  'user:follow'  'public_repo'  'repo'
      'repo_deployment'  'repo:status'  'delete_repo'  'notifications'
      'gist'  'read:repo_hook'  'write:repo_hook'  'admin:repo_hook'
      'admin:org_hook'  'read:org'  'write:org'  'admin:org'
      'read:public_key'  'write:public_key'  'admin:public_key'
  ==
::  ++aut is a "standard oauth2" core, which implements the
::  most common handling of oauth2 semantics. see lib/oauth2 for more details,
::  and examples at the bottom of the file.
++  aut  (~(standard oauth2 bal tok) . |=(tok/token:oauth2 +>(tok tok)))
++  filter-request
  %^  out-add-query-param:aut  'access_token'
    scope=~[%client %admin]
  oauth-dialog='https://github.com/login/oauth/authorize'
::
++  receive-auth-query-string
  %-  in-code-to-token:aut
  url='https://github.com/login/oauth/api/access_token'
++  receive-auth-response  bak-save-token:aut
--

`sec/github.hoon`

::  Test url +https://api.github.com/user
::
::::  /hoon/github/com/sec
  ::  
/+    oauth2
!:
::::
  ::  
|_  {bal/(bale:eyre keys:oauth2) tok/token:oauth2}
++  scopes                          ::  comment out scopes to taste
  :~  'user'  'user:email'  'user:follow'  'public_repo'  'repo'
      'repo_deployment'  'repo:status'  'delete_repo'  'notifications'
      'gist'  'read:repo_hook'  'write:repo_hook'  'admin:repo_hook'
      'admin:org_hook'  'read:org'  'write:org'  'admin:org'
      'read:public_key'  'write:public_key'  'admin:public_key'
  ==  
::  ++aut is a "standard oauth2" core, which implements the
::  most common handling of oauth2 semantics. see lib/oauth2 for more details,
::  and examples at the bottom of the file.
++  aut  (~(standard oauth2 bal tok) . |=(tok/token:oauth2 +>(tok tok)))
++  filter-request
  %^  out-add-query-param:aut  'access_token'
    scope=~[%client %admin]
  oauth-dialog='https://github.com/login/oauth/authorize'
::
++  receive-auth-query-string
  %-  in-code-to-token:aut
  url='https://github.com/login/oauth/access_token' :: '/api' removed
++  receive-auth-response  bak-save-token:aut

urbit / arvo

%eyre via lib/oauth2 not sending headers in post requests? #442

Problem

Background

Issue Reproduction

Network Trace

%bad-json

Adding to lib/oauth2

hiss and httr

Commentary

`cttp.c`

`lib/oauth2.hoon`

`sec/github.hoon`