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 support to the client (CLI, http client) for sending additional basic auth credentials (aka MFA) #5152

Merged
merged 12 commits into from
Mar 27, 2022

Conversation

Kami
Copy link
Member

@Kami Kami commented Feb 13, 2021

This pull request adds support for new credentials.basic_auth = username:password StackStorm CLI config option and -basic-auth CLI argument.

This argument allows client to use additional set of basic auth credentials when talking to the StackStorm API endpoints (api, auth, stream) - that is, in addition to the token / api key native StackStorm auth.

Background, Context

I'm running a StackStorm instance which I want to put behind additional auth wall (think second factor auth). I could probably achieve that using SSO and handling multi factor auth on the SSO side, but I'm not using SSO nor want to use it (and even with SSO used, if there is a bug in StackStorm auth code for SSO, dual factor auth on SSO side would not help in this case while having additional auth in another layer like nginx would).

The easiest way to add second factor to any kind of application which runs behind some kind of http proxy (nginx, Apache, etc.) is to simply utilize existing basic auth primitives of the proxy - this PR does exactly that.

Notes, Limitations

  1. Current CLI and http client code is very messy and hard to work it. We don't have good abstractions for it (a lot of kwargs hacks all over the place) so it's a PITA to update it and work with it (something which has been a known issue for a long time already).

  2. If this second factor auth is enabled and user wants to use webhook integrations, they will also need to send basic auth credentials with webhooks (which shouldn't be an issue since most 3rd party service support that and if they don't, they could also add exceptions in http proxy layer to not require it for webhooks API endpoint or similar).

  3. Currently /v1/auth/tokens API endpoint also utilizes basic auth for authentication which means you should disable proxy based basic auth wall for /v1/auth/tokens API endpoint. This is of course not ideal since it means actually auth endpoint won't be behind MFA wall (aka user may be able to auth and obtain a valid st2 auth token without additional basic auth auth, but other operations and endpoints won't work if valid basic auth header is not provided), but all other endpoints will be.

In nginx proxy, this would look something like this:

...
# Require additional basic auth credentials for all StackStorm API endpoints except /auth/tokens one
location / {
   auth_basic "off";
   auth_basic	"Auth required";
   auth_basic_user_file	/etc/nginx/htpasswd/foo.bar.example;
  ...
}
...

location /auth/tokens {
   auth_basic "off";
  ...
}

TODO

  • Unit tests
  • Verify it works with all the endpoints (so far I verified all the basic one + event stream aka execution tail one)
  • Verify st2web (may require changes in case the current client doesn't simply send existing basic auth credentials browser has cached)

endpoints (api, auth, stream) using additional basic auth credentials in
addition to the standard auth token / api key auth.

This should come handy in situation where SSO is not used, but MFA is
desired or similar
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Feb 13, 2021
endpoint since this one also utilizes basic auth based authentication
mechanism.

This approach is not ideal, but it's a comprompose - in case additional
proxy based basic auth is enabled, user will still need to provide those
additional basic auth credentials header for rest of the API operations
(just not the /auth/tokens one).
@Kami Kami force-pushed the client_and_cli_basic_auth_support branch from 74f940d to 2f39b06 Compare February 13, 2021 17:31
@Kami Kami added this to the 3.5.0 milestone Feb 15, 2021
@Kami Kami changed the title [WIP] [RFC] Add support to the client (CLI, http client) for sending additional basic auth credentials (aka MFA) Add support to the client (CLI, http client) for sending additional basic auth credentials (aka MFA) Apr 5, 2021
@blag
Copy link
Contributor

blag commented Apr 22, 2021

If this second factor auth is enabled and user wants to use webhook integrations, they will also need to send basic auth credentials with webhooks (which shouldn't be an issue since most 3rd party service support that and if they don't, they could also add exceptions in http proxy layer to not require it for webhooks API endpoint or similar).

I haven't ever run across a third party service that supports basic auth for outgoing webhooks. Jira doesn't, GitHub doesn't. So allowing the base webhook endpoint, and all children, to bypass this would definitely be required. Edit: I'm wrong, they both do.

I'm not hugely happy with this. I think putting our efforts into an SSO backend that handles MFA itself would be better.

Also, I'm afraid that this change would give users more undeserved confidence in running StackStorm exposed to the internet, which I don't think has been our security model/mindset so far. I'm already aware of one such instance, and I hope it's the only one.

@Kami
Copy link
Member Author

Kami commented Apr 22, 2021

@blag

I haven't ever run across a third party service that supports basic auth for outgoing webhooks. Jira doesn't, GitHub doesn't. So allowing the base webhook endpoint, and all children, to bypass this would definitely be required.

I'm pretty sure Github and lot of other services support https://<credentials>@url notation for the webhook URLs and handle credentials correctly.

But if that's not possible in some situations, user can also handle that in nginx config level (similar as tokens auth endpoint) so that can be documented.

I'm not hugely happy with this. I think putting our efforts into an SSO backend that handles MFA itself would be better.
Also, I'm afraid that this change would give users more undeserved confidence in running StackStorm exposed to the internet, which I don't think has been our security model/mindset so far. I'm already aware of one such instance, and I hope it's the only one.

Security is all about layers and I think it's good to give users a choice of adding additional layer on top StackStorm auth (+optionally SSO with MFA).

SSO + MFA is definitely a good choice, but for some more security conscious users, it's better to have another layer somewhere outside of the app layer.

There were definitely cases in the past where it was possible to bypass login + mfa due to the bug in the app code.

st2client/st2client/client.py Outdated Show resolved Hide resolved
@blag
Copy link
Contributor

blag commented Apr 22, 2021

I'm pretty sure Github and lot of other services support https://@url notation for the webhook URLs and handle credentials correctly.

Oh, right, yes they do, nevermind.

@Kami Kami force-pushed the client_and_cli_basic_auth_support branch from e67d40e to 566f1ba Compare May 2, 2021 11:10
@amanda11 amanda11 modified the milestones: 3.5.0, 3.6.0 Jun 29, 2021
@amanda11 amanda11 added this to To Do in StackStorm v3.6.0 via automation Jun 29, 2021
@amanda11 amanda11 moved this from To Do to In Progress in StackStorm v3.6.0 Jul 1, 2021
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021

CLA assistant check
All committers have signed the CLA.

@cognifloyd cognifloyd added the status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. label Oct 3, 2021
@arm4b arm4b modified the milestones: 3.6.0, 3.7.0 Oct 13, 2021
@arm4b arm4b removed this from In Progress in StackStorm v3.6.0 Oct 13, 2021
@arm4b arm4b added this to In progress in StackStorm v3.7.0 via automation Oct 13, 2021
@cognifloyd cognifloyd removed the status:needs cla A manual tag to note PRs that are otherwise ready, but the CLA bot says the CLA has not been signed. label Nov 12, 2021
@nzlosh
Copy link
Contributor

nzlosh commented Mar 14, 2022

@Kami Would you mind rebasing the branch against master so we can merge it for the 3.7 release?

@Kami
Copy link
Member Author

Kami commented Mar 27, 2022

@nmaludy I've added changelog entry and synced it up with latest master. Will go ahead and merge it once CI passes - thanks.

@Kami Kami merged commit 2907475 into master Mar 27, 2022
@Kami Kami deleted the client_and_cli_basic_auth_support branch March 27, 2022 13:08
StackStorm v3.7.0 automation moved this from In progress to Done Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI component:st2client size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants