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

Add an auth command #62

Merged
merged 2 commits into from
Mar 23, 2024
Merged

Add an auth command #62

merged 2 commits into from
Mar 23, 2024

Conversation

rneatherway
Copy link
Owner

@rneatherway rneatherway commented Mar 17, 2024

To be used like this:

eval `gh slack auth`

Then you will have SLACK_TOKEN and SLACK_COOKIES set in the environment and can (with rneatherway/slack#3) use other slack commands as you wish.

The main use is during development you don't have to keep unlocking the MacOS keychain. "Always allow" doesn't help because the binary changes on every compile.

@nobe4 we discussed this a while back. What do you think?

@nobe4
Copy link
Contributor

nobe4 commented Mar 19, 2024

This looks great! I wonder if it would be good to add a mention that the Token and Cookies should be treated as secrets. AFAIK, if you have the team, token and cookies, you can basically impersonate someone on Slack.

Comment on lines 14 to 15
Short: "Prints authentication information for the Slack API",
Long: `Prints authentication information for the Slack API.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a simple update like this would be enough?

Suggested change
Short: "Prints authentication information for the Slack API",
Long: `Prints authentication information for the Slack API.`,
Short: "Prints authentication information for the Slack API (treat those as secrets)",
Long: `Prints authentication information for the Slack API (treat those as secrets).`,

{{.CommandPath}}{{end}}{{if gt (len .Aliases) 0}}
Aliases:
{{.NameAndAliases}}{{end}}{{if .HasExample}}

Copy link
Contributor

Choose a reason for hiding this comment

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

But I'd like to have a longer message explaining the criticality of this in the description as well. Maybe this?

Suggested change
Security:
Treat those values as secrets and don't share them with anyone.
If someone get access to them, they will be able to impersonate you.

Copy link
Contributor

@nobe4 nobe4 Mar 19, 2024

Choose a reason for hiding this comment

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

I also am wondering if there's a way to reset the cookie so that we can add a section saying

In case of leak, run `slack auth renew`

And if it's not possible, then to add

Those values can't be manually rotated, so be careful

Copy link
Owner Author

Choose a reason for hiding this comment

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

You can invalidate the token and cookies by logging out of the Slack app. That does seem worth mentioning 👍. Will update.

@rneatherway
Copy link
Owner Author

I'm pretty happy with this now except for one thing. I'm a little concerned that the environment variable names output here and accepted at https://github.com/rneatherway/slack/pull/3/files#diff-2feccbe0109db2f6b93af0c4afed26af571a61438266ebd8690bf669c75d2874R19-R20 are too generic in case someone already has SLACK_TOKEN in their environment. I was trying to think of an alternative. GH_SLACK_* is kind of natural but then https://github.com/rneatherway/slack/ doesn't have anything to do with gh and I might want to ship this repo also as a standalone client rather than a gh plugin anyway. Any ideas?

@rneatherway
Copy link
Owner Author

I'll go ahead with this, it's always possible to change if it causes problems.

@rneatherway rneatherway merged commit 05d68ed into main Mar 23, 2024
2 checks passed
@rneatherway rneatherway deleted the authcmd branch March 23, 2024 13:21
@nobe4
Copy link
Contributor

nobe4 commented Apr 8, 2024

@rneatherway could you create a new release with this included? I just realized that 0.0.23 does not include it 🙏

@rneatherway
Copy link
Owner Author

Sure, done 😁

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.

None yet

2 participants