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

Allow authentication to grpc APIs with oauth bearer tokens #6581

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Oct 2, 2018

No description provided.

@illicitonion illicitonion requested review from ity , blorente and dotordogh Oct 2, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Oct 2, 2018

Depends on #6584

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/remoting/auth branch 2 times, most recently from 018fe8a to d9c6f90 Oct 2, 2018

@illicitonion illicitonion changed the title Support secure grpc connections Allow authentication to grpc APIs with oauth bearer tokens Oct 2, 2018

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/remoting/auth branch 3 times, most recently from 738ff41 to 9681ff2 Oct 3, 2018

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/remoting/auth branch from 9681ff2 to b6bc1a0 Oct 3, 2018

@stuhood

stuhood approved these changes Oct 4, 2018

@@ -291,6 +294,10 @@ def register_bootstrap_options(cls, register):
help='Path to a PEM file containing CA certificates used for verifying secure '
'connections to --remote-execution-server and --remote-store-server. '
'If not specified, TLS will not be used.')
register('--remote-oauth-bearer-token-path', advanced=True,
help='Path to a file containing an oauth token to use for grpc connections to '

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

Would it be worth writing up a very quick README about how to use these options together to hit GCP/RBE? It's not clear whether they are mutually exclusive.

This comment has been minimized.

@benjyw

benjyw Oct 5, 2018

Contributor

Yes, and in particular how that token end up in that file.

It may be worth breaking oauth support out into some subsystem (I have a draft change for this for other reasons) and have this name a provider instead of a file? But I'm fine with making that change later and deprecating this at that time.

@@ -636,7 +636,12 @@ fn main() {
.takes_value(true)
.long("root-ca-cert-file")
.required(false)
).arg(
).arg(clap::Arg::with_name("oauth-bearer-token-file")
.help("Path to file containing oauth bearer token. If not set, not authorization will be provided to remote servers.")

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

s/not authorization/no authorization/

@@ -93,6 +93,13 @@ fn main() {
.long("execution-root-ca-cert-file")
.required(false)
)
.arg(
Arg::with_name("execution-oauth-bearer-token-path")
.help("Path to file containing oauth bearer token for communication with the execution server. If not set, not authorization will be provided to remote servers.")

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

not/no

@@ -106,6 +113,13 @@ fn main() {
.long("cas-root-ca-cert-file")
.required(false)
)
.arg(
Arg::with_name("cas-oauth-bearer-token-path")
.help("Path to file containing oauth bearer token for communication with the CAS server. If not set, not authorization will be provided to remote servers.")

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

ditto

@stuhood stuhood requested review from benjyw and removed request for blorente and dotordogh Oct 4, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Oct 5, 2018

Shard 3 is the only failure here (fixed by #6600). Merging.

@stuhood stuhood merged commit 22a5936 into pantsbuild:master Oct 5, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@stuhood stuhood deleted the twitter:dwagnerhall/remoting/auth branch Oct 5, 2018

stuhood added a commit to twitter/pants that referenced this pull request Oct 6, 2018

stuhood added a commit that referenced this pull request Oct 6, 2018

Fix merge conflict between two PRs. (#6602)
Fix a merge conflict between #6589 and #6581.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment