contrib: Allow use of github API authentication in github-merge #15165

1. 
   laanwj commented at 3:40 PM on January 14, 2019: member

   Three commits I had locally for github-merge.py:

   • Detailed reporting for http errors in github-merge: Print detailed error, this makes it easier to diagnose github API issues.
   • Add support for http[s] URLs in github-merge: Sometimes it can be useful to use github-merge with read-only access (say, for reviewing and testing from untrusted VMs).
   • Allow use of github API authentication in github-merge: The API request limit for unauthenticated requests is quite low. I started running into rate limiting errors. The limit for authenticated requests is much higher. This patch adds an optional configuration setting user.ghtoken that, when set, is used to authenticate requests to the API.
2. contrib: Detailed reporting for http errors in github-merge

   Print detailed error, this makes it easier to diagnose github API issues.

   059a3cffdf
3. contrib: Add support for http[s] URLs in github-merge

   Sometimes it can be useful to use github-merge with read-only access
   (say, for reviewing and testing).

   a4c5bbfcd3
4. laanwj added the label Scripts and tools on Jan 14, 2019
5. contrib: Allow use of github API authentication in github-merge

   The API request limit for unauthenticated requests is quite low.
   I started running into rate limiting errors. The limit
   for authenticated requests is much higher.
   
   This patch adds an optional configuration setting `user.ghtoken`
   that, when set, is used to authenticate requests to the API.

   f1bd219a5b
6. laanwj force-pushed on Jan 14, 2019
7. 
   MarcoFalke commented at 3:50 PM on January 14, 2019: member

   Concept ACK. I have the same local patches, but probably did it less clean. Will take a look later.

8. MarcoFalke added this to the milestone 0.18.0 on Jan 14, 2019
9. in contrib/devtools/README.md:122 in f1bd219a5b

    118 | @@ -119,7 +119,25 @@ Configuring the github-merge tool for the bitcoin repository is done in the foll
    119 |  
    120 |      git config githubmerge.repository bitcoin/bitcoin
    121 |      git config githubmerge.testcmd "make -j4 check" (adapt to whatever you want to use for testing)
    122 | -    git config --global user.signingkey mykeyid (if you want to GPG sign)
    123 | +    git config --global user.signingkey mykeyid
   

   ---

   ---

   

   laanwj commented at 3:55 PM on January 14, 2019:

   Removed the (if you want to GPG sign) intentionally here. I know it's unrelated to this patch, but GPG signing has been mandatory for forever and I don't think this one-line doc change warrants another commit.

10. 
    laanwj commented at 4:26 PM on January 14, 2019: member

    FWIW: There was some discussion on IRC about storing authentication tokens in the git configuration. According to @promag this is not a good idea (I guess because some people have these files under version control?). He suggests using an environment variable instead. I'm not sure if this is a good idea because environment variables 'leak' through /proc/<pid>/environ, and if you set it in .bashrc well that has exactly the same potential issue.

    What I use myself is

    [include]
        path = ~/.gitsecrets
    

    Then the secrets files (with very restricted permissions) contains the ghtoken, and SMTP authentication for sending mail (for sending Linux patches).

11. 
    MarcoFalke commented at 4:45 PM on January 14, 2019: member

    It is a read-only token, so the worst thing that could happen is that someone can get you rate-limited?

12. 
    gkrizek commented at 4:50 PM on January 14, 2019: contributor

    utACK f1bd219 @MarcoFalke It's whatever perms you set on the token. It seems like GitHub allows you to create permission-less tokens. So it shouldn't even have read access, if you create it as such. So in that case, yes. The worst thing is someone uses up all your requests and you get rate limited again.

13. 
    gkrizek commented at 4:56 PM on January 14, 2019: contributor

    Link to GitHub documentation on this: https://developer.github.com/v3/#rate-limiting

    Very small nit: you could also set User-Agent here. GitHub requires a User-Agent to be set for all API requests. I don't know of a case where urllib doesn't set one by default, but it might be nice to be explicit about it. https://developer.github.com/v3/#user-agent-required

14. 
    laanwj commented at 5:33 PM on January 14, 2019: member

    It's whatever perms you set on the token

    I think it's wise to set as little privileges on the token as possible. This is why in the documentation I suggest creating a token without extra privileges.

    I don't know of a case where urllib doesn't set one by default, but it might be nice to be explicit about it.

    urllib (in Python 3.x, which is the only version supported) sets the User Agent to Python-urllib/3.x. I think this is good enough?

15. 
    gkrizek commented at 5:44 PM on January 14, 2019: contributor

    token without extra privileges

    Yes, totally. I wasn't trying to say to do anything different. Just saying that if you screw up and give the token full access, your attack surface is full access.

    think this is good enough?

    Yeah, that will definitely work and I don't see that changing. Just making a comment about User-Agent requirements if we wanted to be explicit about setting it.

16. 
    promag commented at 5:49 PM on January 14, 2019: member

    utACK

17. 
    fanquake commented at 11:52 PM on January 14, 2019: member

    Concept ACK

    Add support for http[s] URLs in github-merge: Sometimes it can be useful to use github-merge with read-only access (say, for reviewing and testing from untrusted VMs).

    👍

18. 
    laanwj commented at 8:42 AM on January 15, 2019: member

    Yeah, that will definitely work and I don't see that changing. Just making a comment about User-Agent requirements if we wanted to be explicit about setting it.

    I think one argument for setting a custom User Agent here, other than the default python one, would be so that github knows what is using their API—simply by googling the UA string. But that starts to be relevant for scripts that generate a lot of traffic.

19. laanwj merged this on Jan 16, 2019
20. laanwj closed this on Jan 16, 2019

    ---

21. laanwj referenced this in commit bcdd31f265 on Jan 16, 2019
22. 
    fanquake commented at 11:14 AM on January 16, 2019: member

    post-merge utACK bcdd31f

23. Munkybooty referenced this in commit d51c3e340e on Aug 21, 2021
24. Munkybooty referenced this in commit 49a4fb0e44 on Aug 23, 2021
25. Munkybooty referenced this in commit 7d1d07108c on Aug 24, 2021
26. Munkybooty referenced this in commit 8bed044ac8 on Aug 24, 2021
27. Munkybooty referenced this in commit 68886087c5 on Aug 24, 2021
28. UdjinM6 referenced this in commit 64243d5417 on Aug 24, 2021
29. Munkybooty referenced this in commit 6e81d04ecc on Aug 24, 2021
30. DrahtBot locked this on Dec 16, 2021

Milestone
0.18.0