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.

Sign up for GitHub

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

Jump to bottom

Warn when the token query param is used for auth #16009

Merged
merged 8 commits into from
Jan 24, 2023
Merged

Warn when the token query param is used for auth #16009

merged 8 commits into from
Jan 24, 2023

Conversation

roncodingenthusiast
Copy link
Contributor

@roncodingenthusiast roncodingenthusiast commented Jan 18, 2023 •
edited by jkirschner-hashicorp
Loading

Description

  • Emit a warning log message every time the token query param is used
  • Update docs to reflect the deprecation of the token query param

Makes progress on #11917. Remaining work: fully remove in a future release.

Testing & Reproduction steps

Manual testing steps

  • Ran consul locally (make dev...)
  • Tried a request with header
curl \
    --header "X-Consul-Token: <my token>" \
    "http://127.0.0.1:8500/v1/agent/members"

## No logs
  • Tried a request with token without the header
curl \
    "http://127.0.0.1:8500/v1/agent/members?token=<my token>"

## log message
2023-01-18T15:37:36.989-0500 [WARN]  agent.http: This request is using a token query parameter; This is deprecated and will be removed in future Consul versions.: logUrl=/v1/agent/members?token=<hidden>
  • Tried a request with token and the header present
curl \
    --header "X-Consul-Token: <my token>" \
    "http://127.0.0.1:8500/v1/agent/members?token=<my token>"

## log message
2023-01-18T15:48:18.764-0500 [WARN]  agent.http: This request is using a token query parameter; This is deprecated and will be removed in future Consul versions.: logUrl=/v1/agent/members?token=<hidden>

Still todo

  • update tests that still use token query params to use header instead
  • Generate change-log

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@github-actions github-actions bot added the type/docs Documentation needs to be created/updated/clarified label Jan 18, 2023
@roncodingenthusiast roncodingenthusiast changed the title [NET-1728] warn when use of token query param [NET-1728] warn when the token query param is used for auth Jan 18, 2023
@roncodingenthusiast roncodingenthusiast changed the title [NET-1728] warn when the token query param is used for auth WIP [NET-1728] warn when the token query param is used for auth Jan 18, 2023
@roncodingenthusiast roncodingenthusiast mentioned this pull request Jan 18, 2023
Closed
2 tasks
jkirschner-hashicorp
jkirschner-hashicorp reviewed Jan 19, 2023
View reviewed changes
agent/http.go Outdated Show resolved Hide resolved
@roncodingenthusiast roncodingenthusiast force-pushed the NET-1728/warn-when-use-of-token-query-param branch from f3beff3 to ab533e2 Compare January 19, 2023 23:25
@roncodingenthusiast roncodingenthusiast changed the title WIP [NET-1728] warn when the token query param is used for auth [NET-1728] warn when the token query param is used for auth Jan 19, 2023
@roncodingenthusiast roncodingenthusiast marked this pull request as ready for review January 19, 2023 23:35
@roncodingenthusiast roncodingenthusiast requested a review from a team as a code owner January 19, 2023 23:35
kisunji
kisunji reviewed Jan 20, 2023
View reviewed changes
.changelog/16009.txt Outdated Show resolved Hide resolved
agent/http.go Outdated Show resolved Hide resolved
@roncodingenthusiast roncodingenthusiast changed the title [NET-1728] warn when the token query param is used for auth Warn when the token query param is used for auth Jan 20, 2023
kisunji
kisunji approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

:shipit:

im2nguyen
im2nguyen approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@im2nguyen im2nguyen left a comment

Choose a reason for hiding this comment

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

a couple nits, but pre-approving so you're not blocked!

website/content/api-docs/api-structure.mdx Outdated Show resolved Hide resolved
website/content/docs/upgrading/upgrade-specific.mdx Outdated Show resolved Hide resolved
jkirschner-hashicorp
jkirschner-hashicorp reviewed Jan 20, 2023
View reviewed changes
agent/http.go Outdated Show resolved Hide resolved
cthain
cthain reviewed Jan 20, 2023
View reviewed changes
Copy link
Contributor

@cthain cthain left a comment

Choose a reason for hiding this comment

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

Looking really good. I had a few minor comments/suggestions.

.changelog/16009.txt Outdated Show resolved Hide resolved
.changelog/16009.txt Outdated Show resolved Hide resolved
agent/agent_endpoint_test.go Outdated Show resolved Hide resolved
website/content/docs/upgrading/upgrade-specific.mdx Outdated Show resolved Hide resolved
@cthain cthain added theme/api Relating to the HTTP API interface theme/auth Authentication Issues labels Jan 20, 2023
cthain
cthain approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@cthain cthain left a comment

Choose a reason for hiding this comment

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

Great work on this PR Ronald 👍

@jkirschner-hashicorp
Copy link
Contributor

jkirschner-hashicorp commented Jan 20, 2023 •
edited
Loading

Awesome work :) Looks like engineering and education have both approved. If you agree you're done resolving all the comments, merge at will!

(Well, I guess the branch needs to be re-updated from main first / the CI pipeline has to finish first)

jkirschner-hashicorp
jkirschner-hashicorp requested changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

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

Placing a block on this until we discuss something I just left a comment on. (Still writing the comment.)

agent/http.go Show resolved Hide resolved
jkirschner-hashicorp
jkirschner-hashicorp approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

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

My previous review comment has been addressed 👍

@kisunji kisunji force-pushed the NET-1728/warn-when-use-of-token-query-param branch from dd412a3 to d951eb2 Compare January 24, 2023 15:57
@kisunji
Copy link
Contributor

kisunji commented Jan 24, 2023 •
edited
Loading

@roncodingenthusiast You'll have to OAuth into CircleCI with Github for the CI to run in this PR

edit: Oh never mind, maybe my rebase kicked it off

@kisunji kisunji enabled auto-merge (squash) January 24, 2023 16:13
@kisunji kisunji merged commit 6167aef into main Jan 24, 2023
@kisunji kisunji deleted the NET-1728/warn-when-use-of-token-query-param branch January 24, 2023 16:21
skpratt pushed a commit that referenced this pull request Jan 25, 2023
@roncodingenthusiast @skpratt
Warn when the token query param is used for auth (#16009)
8dcdc39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@im2nguyen im2nguyen im2nguyen approved these changes

@kisunji kisunji kisunji approved these changes

@cthain cthain cthain approved these changes

@jkirschner-hashicorp jkirschner-hashicorp jkirschner-hashicorp approved these changes

Assignees
No one assigned
Labels
theme/api Relating to the HTTP API interface theme/auth Authentication Issues type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@roncodingenthusiast @jkirschner-hashicorp @kisunji @im2nguyen @cthain