-
Notifications
You must be signed in to change notification settings - Fork 63
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
auth support for Tron APIs #1005
base: master
Are you sure you want to change the base?
Conversation
53e9e03
to
9f35622
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of a bummer that there's so much code dupe between paasta and tron, but a solution to address that (create another open source package for the common code) would be worse :)
9f35622
to
ca480c2
Compare
@@ -37,9 +37,22 @@ class RequestError(ValueError): | |||
} | |||
|
|||
|
|||
def get_sso_auth_token() -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one difference with tron is that there's also a webui - are we expecting to only protect endpoints that are ever called by the CLI or is there some sort of way to get the same auth headers populated by any of our internal proxy mechanisms (or by doing the whole SAML SSO dance)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jfong was asking the same thing in TRON-2374. The plan is to only enforce authz on management actions (e.g. start / stopping jobs), and leave anything that just reads data open, and since as far as I know tronweb is read-only, there shouldn't be problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha - there have been some requests to enable mutations from the UI (and it was something we were considering)
we were debating trying to add this during our current unhack, but we decided to punt on it in favor of potentially re-using this auth feature - but it sounds like this feature might not be re-usable for this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call it "not re-usable", it'll just need a bit more work on top. More than happy to help brainstorm a way to end up with a properly authenticated tronweb :)
This is a carbon copy of Yelp/paasta#3985, as eventually we'll want to have authentication on these APIs too.