Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jan 16, 2019

Conversation

Projects
None yet
5 participants
@laanwj
Copy link
Member

commented Jan 14, 2019

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.

laanwj added some commits Jan 14, 2019

contrib: Detailed reporting for http errors in github-merge
Print detailed error, this makes it easier to diagnose github API issues.
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).
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.

@laanwj laanwj force-pushed the laanwj:2019_01_ghmerge branch to f1bd219 Jan 14, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Jan 14, 2019

@@ -119,7 +119,25 @@ Configuring the github-merge tool for the bitcoin repository is done in the foll

git config githubmerge.repository bitcoin/bitcoin
git config githubmerge.testcmd "make -j4 check" (adapt to whatever you want to use for testing)
git config --global user.signingkey mykeyid (if you want to GPG sign)
git config --global user.signingkey mykeyid

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 14, 2019

Author Member

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.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

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).

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

@gkrizek

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

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.

@gkrizek

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

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?

@gkrizek

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

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.

@promag

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

utACK

@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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).

👍

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

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.

@laanwj laanwj merged commit f1bd219 into bitcoin:master Jan 16, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 16, 2019

Merge #15165: contrib: Allow use of github API authentication in gith…
…ub-merge

f1bd219 contrib: Allow use of github API authentication in github-merge (Wladimir J. van der Laan)
a4c5bbf contrib: Add support for http[s] URLs in github-merge (Wladimir J. van der Laan)
059a3cf contrib: Detailed reporting for http errors in github-merge (Wladimir J. van der Laan)

Pull request description:

  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.

Tree-SHA512: ca8ae1874a787263e49d915d7cf31c0c0f50aba229c9440265bf1fda69f7e00641d1492512b93d76c17ff1766859283d640d37770acb120898736ad97efbd5c2
@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

post-merge utACK bcdd31f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.