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

OAuth Slack Token Logged In Clear #944

Closed
christophercutajar opened this issue Nov 9, 2020 · 8 comments · Fixed by #1003
Closed

OAuth Slack Token Logged In Clear #944

christophercutajar opened this issue Nov 9, 2020 · 8 comments · Fixed by #1003
Labels
bug Something isn't working
Milestone

Comments

@christophercutajar
Copy link
Contributor

Describe the bug
The OAuth base64 Slack token used by the Slack trigger is logged in the clear.

To Reproduce
Steps to reproduce the behavior:

  1. Follow the steps to create a Slack App and a Slack trigger.
  2. Fire Slack trigger
  3. Check the slack webhook sensor logs.
sensor-qrlrb-58d497d595-q9dmh main 2020-11-09T10:11:50.147597554Z slack-go/slack2020/11/09 10:11:50 chat.go:217: Sending request: channel=random&text=webhook+triggered%21&token=xoxb-xxxx-xxx-xxx

Expected behavior
OAuth Slack token not logged in the clear in the log files.

Screenshots
Not Applicable.

Environment (please complete the following information):

  • Kubernetes: 1.15.12-gke.20
  • Argo: v2.11.6
  • Argo Events: v1.0.0

Message from the maintainers:

If you wish to see this enhancement implemented please add a 👍 reaction to this issue! We often sort issues this way to know what to prioritize.

@christophercutajar christophercutajar added the bug Something isn't working label Nov 9, 2020
@whynowy
Copy link
Member

whynowy commented Nov 9, 2020

This seems to be an issue of slack-go/slack, could you check if there's a way to disable it, or raise an issue in slack-go/slack repo?

@christophercutajar
Copy link
Contributor Author

I'll check the code how this is used in Argo Events and if this an issue with slack-go/slack, I'll move ahead and raise the issue with them.

@whynowy
Copy link
Member

whynowy commented Nov 9, 2020

Thanks @christophercutajar !

@christophercutajar
Copy link
Contributor Author

christophercutajar commented Nov 9, 2020

@whynowy from Argo, the api.PostMessage in

channelID, timestamp, err := api.PostMessage(channel, slack.MsgOptionText(message, false))
is used which then triggers SengMessageContext function from chat.go in https://github.com/slack-go/slack/blob/686c209f9525a78313cfe54cbc07cd86bd677384/chat.go#L200.

From Argo point-of-view there is no safer api function that can be used within slack-go/slack. I'll open a ticket with them an link this issue with that.

@christophercutajar
Copy link
Contributor Author

christophercutajar commented Nov 9, 2020

Looking this in more detail, this line of log is only logged if api.Debug() is true in chat.go. From Argo this can be avoided if slack.OptionDebug(true) is changed to slack.OptionDebug(false) in

api := slack.New(slackToken, slack.OptionDebug(true))

@whynowy
Copy link
Member

whynowy commented Nov 9, 2020

Looking this in more detail, this line of log is only logged if api.Debug() is true in chat.go. From Argo this can be avoided if slack.OptionDebug(true) is changed to slack.OptionDebug(false) in

api := slack.New(slackToken, slack.OptionDebug(true))

Thanks! A PR to fix it?

@christophercutajar
Copy link
Contributor Author

Yeah @whynowy, will work on it and perform some testing to make sure that setting it to false will actually solve the issue.

@whynowy whynowy added this to the v1.2 milestone Nov 10, 2020
whynowy added a commit to whynowy/argo-events that referenced this issue Dec 26, 2020
whynowy added a commit that referenced this issue Dec 30, 2020
@christophercutajar
Copy link
Contributor Author

Thanks @whynowy for taking this and apologies for not solving this earlier.

juliev0 pushed a commit to juliev0/argo-events that referenced this issue Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants