Skip to content

Make it possible for Go agents to use auth tokens from HTTP header#10237

Merged
mshaposhnik merged 8 commits intomasterfrom
agents_headers
Jul 3, 2018
Merged

Make it possible for Go agents to use auth tokens from HTTP header#10237
mshaposhnik merged 8 commits intomasterfrom
agents_headers

Conversation

@mshaposhnik
Copy link
Copy Markdown
Contributor

@mshaposhnik mshaposhnik commented Jul 2, 2018

What does this PR do?

For now Go agents is able to extract aithentication tokens only from request query parameters,
which is inconvenient for proxy authetication and has some other disadvantages.
This PR makes possible to pass tokens via Authorization header and refactors readiness/liveness probes to use them.

What issues does this PR fix or reference?

Release Notes

N/A

Docs PR

N/A

@mshaposhnik mshaposhnik requested a review from garagatyi as a code owner July 2, 2018 07:13
@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 2, 2018
token := req.URL.Query().Get("token")
if token == "" {
header := req.Header.Get("Authorization")
if header != "" && strings.HasPrefix(strings.ToLower(header), "bearer") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's impossible in our authentication implementation but still, you check bearer word and cut bearer what if there will be a token that starts with bearer that is not type but just part of token like bearer9SUcJXjsdenx. Maybe it would be better to check if token is prefixed with bearer

Copy link
Copy Markdown
Contributor Author

@mshaposhnik mshaposhnik Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bearer word indicates type of token, there also can be Basic or Digest which is not supported. So we cut it away to have pure token. Or do i understood your question wrong ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown helps me to confuse you =)
Let me rephrase, you check bearer word without space and cut bearer with space (7 characters) Maybe it would be better to check if token is prefixed with **bearer ** with space.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok


public ExecServerLivenessProbeConfigFactory(int successThreshold) {
@Inject
public ExecServerLivenessProbeConfigFactory(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are able not to change current approach with public available liveness probes.
@skabashnyuk WDYT? Should we revert it here and create an issue for implementing exclude in JWT proxy?

Copy link
Copy Markdown

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@mshaposhnik mshaposhnik merged commit 29eef88 into master Jul 3, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jul 3, 2018
@benoitf benoitf added this to the 6.8.0 milestone Jul 3, 2018
@mshaposhnik mshaposhnik deleted the agents_headers branch July 9, 2018 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants