Add support for storing OAuth2.0 tokens in AWS Secrets Manager

[michaelstepner]

Closes #33, which features a substantive discussion about the design of this feature.

Objective

Provides an option to store tokens remotely in AWS Secrets Manager. By storing the authorization tokens remotely, this facilitates the administration of the proxy in a server-based configuration:

• individual servers can be replaced without reauthorizing: a new server can fetch the remote tokens and remain authenticated as long as the tokens haven't expired
• authenticating on a local computer can provide tokens to a headless server

Implementation

This implementation endeavours to minimize the complexity added to the proxy:

1. It adds an optional account-level configuration parameter, aws_secret:

[[email protected]]
permission_url = https://login.microsoftonline.com/common/oauth2/v2.0/authorize
token_url = https://login.microsoftonline.com/common/oauth2/v2.0/token
oauth2_scope = https://outlook.office365.com/IMAP.AccessAsUser.All https://outlook.office365.com/SMTP.Send offline_access
redirect_uri = http://localhost
client_id = *** your client id here ***
client_secret = *** your client secret here ***
aws_secret = *** ARN or name of secret in AWS Secrets Manager ***

2. The only dependency is boto3, and it is only loaded if one or more accounts have the aws_secret option specified in their config. This new dependency is documented in requirements-aws-secrets.txt

3. It adds new methods to the AppConfig class to handle the option of loading/saving OAuth 2.0 tokens from the remote AWS Secrets Manager.

   • These methods are only called from AppConfig._load() and AppConfig.save() if an account has the aws_secret option specified, which minimizes the risk that this new feature affects existing functionality.
   • These new methods are all named with the prefix aws_secrets.

4. If the proxy fails to access the AWS Secrets Manager, it logs an error but continues functioning normally: without fetching the remote secret, or without storing the secret remotely.

Manual Testing

I've tested this new feature quite carefully using the Terraform config in PR #1 of michaelstepner/email-oauth2-proxy-aws.

I've also verified that the proxy will seamlessly create the specified AWS Secret if it does not yet exist, using the Terraform config in PR #2 of michaelstepner/email-oauth2-proxy-aws.

This lists the various cases that are handled, along with the associated debug log error messages (--debug enabled) tested manually on commit 15f049c:

• ✅ Proxy uses AWS-SM (AWS Secrets Manager) when secret is already created and populated
• ✅ Proxy uses AWS-SM when secret does not yet exist, is specified with name, and IAM has permission to create it
• ✅ Warning: boto3 is not installed

  2023-01-09 11:33:02: Failed to load AWS SDK (boto3) - "aws_secret" is specified in config file for at least one account; have you installed dependencies from `requirements-aws-secrets.txt`?
  2023-01-09 11:33:02: AWS SDK load error: ModuleNotFoundError("No module named 'boto3'")
  2023-01-09 11:33:02: Unable to get AWS client; cannot fetch OAuth 2.0 tokens from AWS Secrets Manager
  2023-01-09 11:33:02: Starting SMTP server at IP:PORT (TLS) proxying smtp.office365.com:587 (STARTTLS)
  2023-01-09 11:33:02: Initialised Email OAuth 2.0 Proxy - listening for authentication requests. Connect your email client to begin
  

• ✅ Warning: IAM user does not have secretsmanager:GetSecretValue permissions

  2023-01-09 11:39:07: Fetching AWS Secret "test1" - access denied: does the IAM user have the "secretsmanager:GetSecretValue" permission?
  2023-01-09 11:39:07: AccessDeniedException: User: arn:aws:iam::000000000000:user/email_oauth2_proxy is not authorized to perform: secretsmanager:GetSecretValue on resource: test1 because no identity-based policy allows the secretsmanager:GetSecretValue action
  2023-01-09 11:39:07: Starting SMTP server at IP:PORT (TLS) proxying smtp.office365.com:587 (STARTTLS)
  2023-01-09 11:39:07: Initialised Email OAuth 2.0 Proxy - listening for authentication requests. Connect your email client to begin
  

• ✅ Warning: secret does not exist, cannot create a secret with a specific ARN

  2023-01-09 13:50:40: Fetching AWS Secret "arn:aws:secretsmanager:us-east-1:000000000000:secret:MySecret-a1b2c3d4e5" - secret does not exist or does not contain a secret value
  2023-01-09 13:50:40: ResourceNotFoundException: Secrets Manager can't find the specified secret.
  [...]
  2023-01-09 13:52:10: Failed to store OAuth 2.0 tokens in AWS Secret "arn:aws:secretsmanager:us-east-1:000000000000:secret:MySecret-a1b2c3d4e5" - the secret does not exist, and it is not possible to create a secret with a specific ARN
  2023-01-09 13:52:10: ResourceNotFoundException: Secrets Manager can't find the specified secret.
  

• ✅ Warning: secret does not exist, IAM user does not have secretsmanager:CreateSecret permissions

  2023-01-09 11:47:35: Fetching AWS Secret "test2" - secret does not exist or does not contain a secret value
  2023-01-09 11:47:35: ResourceNotFoundException: Secrets Manager can't find the specified secret.
  [...]
  2023-01-09 11:47:56: AWS Secret "test2" does not exist - attempting to create it
  2023-01-09 11:47:56: ResourceNotFoundException: Secrets Manager can't find the specified secret.
  2023-01-09 11:47:56: Failed to store OAuth 2.0 tokens in AWS Secret "test2" - access denied: does the IAM user have the "secretsmanager:CreateSecret" permission?
  2023-01-09 11:47:56: AccessDeniedException: User: arn:aws:iam::000000000000:user/email_oauth2_proxy is not authorized to perform: secretsmanager:CreateSecret on resource: test2 because no identity-based policy allows the secretsmanager:CreateSecret action
  

• ✅ Warning: secret does not exist, IAM user does not have secretsmanager:PutSecretValue permissions

  2023-01-09 13:55:51: Fetching AWS Secret "test2" - secret does not exist or does not contain a secret value
  2023-01-09 13:55:51: ResourceNotFoundException: Secrets Manager can't find the specified secret.
  [...]
  2023-01-09 13:57:38: Failed to store OAuth tokens in AWS Secret "test2" - access denied: does the IAM user have the "secretsmanager:PutSecretValue" permission?
  2023-01-09 13:57:38: AccessDeniedException: User: arn:aws:iam::000000000000:user/email_oauth2_proxy is not authorized to perform: secretsmanager:PutSecretValue on resource: test2 because no identity-based policy allows the secretsmanager:PutSecretValue action
  

• ✅ Warning: secret exists, IAM user does not have secretsmanager:PutSecretValue permissions

  2023-01-09 11:45:27: Failed to store OAuth tokens in AWS Secret "test1" - access denied: does the IAM user have the "secretsmanager:PutSecretValue" permission?
  2023-01-09 11:45:27: AccessDeniedException: User: arn:aws:iam::000000000000:user/email_oauth2_proxy is not authorized to perform: secretsmanager:PutSecretValue on resource: test1 because no identity-based policy allows the secretsmanager:PutSecretValue action
  

[simonrob]

Thanks for this contribution! I've had a quick look and added some initial thoughts/suggestions.

[michaelstepner]

Thanks for this contribution! I've had a quick look and added some initial thoughts/suggestions.

@simonrob thanks for the careful review and thoughtful feedback! I plan to iteratively revise & respond to each point over the holidays, and will tag you in a comment when I've finished the first round of revisions.

[michaelstepner]

@simonrob I did a pass through all your comments. Thanks again for the feedback!

• I fixed a few minor issues and marked them resolved.
• I asked a few clarifying questions for you.
• I did some more research about error handling with boto3 and learned that the messiness of my first draft is not just a case of my inexperience. It's quite poorly designed and widely complained about.
  • I'm going to make some revisions after testing, but it's not going to end up as tidy as one would hope.

I will await your feedback on the clarifying questions before tackling the items that require more extensive testing with the AWS SDK.

[simonrob]

Here I've suggested a minor reorganisation of the requirements file to include the core no-GUI requirements so that this file can be relied upon to install all dependencies.

[michaelstepner]

@simonrob I've substantially refactored the code in commits 5b7d63b and 2c9c6be in order to:

• Address all of your feedback in the code review thus far
• Eliminate the proposed --aws-secrets argument, which was no longer necessary, so removing it simplifies configuration for the user.
• Isolate the new code which implements the secrets manager within clearly-named methods in the AppConfig class
  • This feature only interacts with existing code via simple checks in AppConfig._load() and AppConfig.save() to see if AWS Secrets Manager functions should be called.
• Provide more informative error messages

I'm much happier with this new design: I believe the code is more robust and more maintainable.

It's ready for a new round of code review when you have a chance.

[simonrob]

Thanks for the update!

I think this is very nearly ready to go – I went through line by line this time and made lots of very minor edits just for consistency with the rest of the proxy's code style and linting settings. There is one question about the debug log method, and I also made a separate commit to add a global list of tokens that should be stored in a secrets manager.

Could you check that this updated version works and I haven't inadvertently introduced any new issues?

[michaelstepner]

@simonrob thank you for putting in another round of detailed review and feedback! I am accepting all of your suggestions and will make a couple of my own.

[simonrob]

One thing we haven't covered so far is what would/should happen if the required keys are not present (either in the secret, or in the proxy's cache).

I don't know if/how this could happen from the AWS side, but the proxy can (and does) delete values when they become invalid, which would cause this method to fail.

One simple solution here would be to use a fallback value in get() and skip keys if they have no value (and entire accounts if there are no valid keys). What do you think?

[simonrob]

Another related question: my assumption is that token renewal is handled via a local copy of the proxy - i.e., you receive an authentication failure when using your remote server, which prompts you to open a local copy and authenticate, updating the AWS secret. Is that correct? If so, how do you trigger a token update from AWS Secrets Manager on the remote server?

[michaelstepner]

I addressed the first question about required keys in a review below.

Another related question: my assumption is that token renewal is handled via a local copy of the proxy - i.e., you receive an authentication failure when using your remote server, which prompts you to open a local copy and authenticate, updating the AWS secret. Is that correct? If so, how do you trigger a token update from AWS Secrets Manager on the remote server?

I think there are 3 options:

1. handle token renewal locally and use SIGHUP to reload config on the remote server.
2. handle token renewal locally and restart the remote server's proxy to reload config
3. handle the token renewal by ssh-ing into the remote server.

[simonrob]

I was wondering whether it is worth triggering AppConfig._aws_secrets_fetch() (probably via a more generic method in the AppConfig class) after a login failure for an account using AWS Secrets. This way, a login failure on the server would trigger checking for updated credentials that have been provided via a local login, and there would be no need to log in to the server at all.

[michaelstepner]

Thanks @simonrob! Happy to test that out, although looking at my work week I'm not sure how soon I'll get to it this week.

Since we've completed the core functionality, do you think it would be worthwhile to merge this PR and split the reload functionality into a new PR? Or would you prefer to keep it in a single PR?

[michaelstepner]

Looking at this made me reconsider the allow_catch_all_accounts advanced configuration option. For consistency, having an aws_secret value in any parent account (e.g., the global [@], or, say [@gmail.com]) should use that secret to store values for all child accounts within that group. I'll think about how is best to address this.

The simplest brute solution would be to only support aws_secret functionality for fully-specified accounts, and return a warning & ignore the aws_secret if it's specified for a catch-all account. Up to you how valuable it would be to get these two features to work together, but I don't foresee using that functionality personally.

[michaelstepner]

On second thought, I think the functionality already implemented so far will work fine for catch-all accounts (unless I'm mistaken). It's the reload step that is trickier.

[simonrob]

I think the catch-all functionality is quite valuable, but it should be relatively straightforward to incorporate that into the existing PR. I'll take a look when I get chance.

How did you get on with the reloading suggestion?

[michaelstepner]

I've had a busy week and haven't had a chance to test the reloading approach you suggested, but I look forward to testing it soon—perhaps this weekend.

Re: catch-all with AWS Secrets, I agree it will be best to integrate! I haven't used catch-all accounts before because I only use the proxy for a single account, but I suppose I can define a catch-all that applies to that single account in order to test.

[michaelstepner]

@simonrob I've finished manually testing the code in commit 15f049c, stepping through each of the scenarios covered by our error handling logic. I've pasted the associated logged errors in the description at the top of this PR. All is working correctly in my tests.

I will now address the new comment you opened above re: what would/should happen if the required keys are not present. Once that's resolved, I think this PR is ready to merge.

[michaelstepner]

One thing we haven't covered so far is what would/should happen if the required keys are not present (either in the secret, or in the proxy's cache).

I don't know if/how this could happen from the AWS side, but the proxy can (and does) delete values when they become invalid, which would cause this method to fail.

One simple solution here would be to use a fallback value in get() and skip keys if they have no value (and entire accounts if there are no valid keys). What do you think?

I manually tested what would happen if I edited the secret value in AWS Secrets Manager to remove a key (e.g. remove token_salt). As you predicted, this causes a crash because we don't have a fallback value.

I agree that a missing key should not cause a crash. The proxy should have the same behaviour as it would if the local config had a missing key.

I've proposed two changes that would set a default value of an empty string '' when fetching/storing the token keys.

I also posed a question below about whether we need to add additional error handling
logic around remove_option.

[michaelstepner]

@simonrob based on @thiagomacieira's question in #33, I wonder if you'd like to adjust the way that we're specifying this feature in the config, in order to provide future flexibility for alternative token storage strategies.

Right now, we're enabling AWS Secrets Manager token storage by adding the following to config:

aws_secret = *** ARN or name of secret in AWS Secrets Manager ***

We could potentially modify the config syntax to "future-proof" it, ex:

token_storage = { "type": "aws_secrets_manager", "id": "*** ARN or name of secret in AWS Secrets Manager ***" }

In the future, if for example someone implemented token storage in an alternative local file, it could use the same config key, ex:

token_storage = { "type": "local", "file": "path/to/local/file" }

[simonrob]

That's an interesting idea, but I wonder whether it is a little overly complex. Looking at the current implementation, is there actually any need to have a separate AWS Secrets Manager ID for each account entry, or could this be a global setting?

If not, both the AWS feature and the ability to store tokens in a local cache file could as you point out be implemented in a very similar way. I'd probably make this a little more generic, and have the AppConfig.save() method strip out everything except the key tokens from a ConfigParser deep copy, and then just pass this object to whichever saving method is configured. As it happens, this would also remove any issues with catch-all accounts, which makes things simpler in the AWS case too.

What do you think – could this all be handled via a global configuration parameter? (Or perhaps a command-line option – e.g., --cache-location aws://id or --cache-location file:///path/to/file)

[michaelstepner]

Although I don't personally store the credentials for different accounts in different locations, that's because I have only one account and am using the proxy in a single-user setting. I implemented it as an account-specific parameter because it seemed plausible to me that one might like to have separate access controls for the different accounts.

Suppose I'm running a remote server for two different users, each of whom I'd like to empower to update their own credentials from their local computers. If it's a global config parameter, then each user must have read/write access to each other's tokens. If it's an account-specific config parameter, then each user could have individual access controls to their own secrets.

Do you think the JSON value for token_storage is especially complex? It seems like it would be a very small change to the current code, although it allows complete flexibility for the configuration of other token storage options in the future. (Which I suppose could become complex, although they could remain simple.)

[simonrob]

I think it becomes overly complex once additional external token stores come into the picture. I'm also not sure how often the sort of situation you've outlined would actually happen - remember each user would also have to have their own copy of the proxy to re-authenticate periodically and then synchronise with the server. Is that likely?

I think it's probably more common that someone running that setup would either be an individual (like yourself) or an organisation that uses one of the O365 methods that removes the need for local OAuth.

If this is made a global setting then everything becomes much simpler (and easier to maintain) - if you want to add a new token store you are simply provided with a ConfigParser object, and you write all of its contents to whatever storage you like. There are no issues with any other proxy features either (except possibly client secret encryption, but it's unlikely you'd be using that in this case).

[michaelstepner]

I'm also not sure how often the sort of situation you've outlined would actually happen - remember each user would also have to have their own copy of the proxy to re-authenticate periodically and then synchronise with the server. Is that likely?

Not sure! In this case we're entirely in the realm of speculation regarding future use cases that neither of us will personally face.

If this is made a global setting then everything becomes much simpler (and easier to maintain) - if you want to add a new token store you are simply provided with a ConfigParser object, and you write all of its contents to whatever storage you like. There are no issues with any other proxy features either (except possibly client secret encryption, but it's unlikely you'd be using that in this case).

Ultimately you're the maintainer of the package and I will gladly defer to you on the tradeoffs regarding flexibility vs complexity/maintainability.

Do you want to refactor the current PR to make the token storage a global setting?

[simonrob]

I spent some time just now thinking about how to balance the complexity, multi-account inheritance, desire for other external token caches and ongoing maintainability. 2e6b641 is what I ended up with – this adds a new --cache-store script parameter, removes the ability to specify a different AWS Secret per account, and also adds a new requirement to use a prefix if specifying a name rather than an ARN. But it means that other cache stores can be added far more easily, and in my opinion is a bit more maintainable going forward.

What do you think?

[simonrob]

@michaelstepner I'm pretty happy with this new version, but it'd be useful to get your feedback if you have time.

[simonrob]

@michaelstepner any thoughts about this?

[simonrob]

I plan to merge this next week, so if you have any feedback or issues please do let me know before then.

[simonrob]

Thanks @michaelstepner for all the work on this – I think it's time to merge.

[michaelstepner]

@simonrob I'm so grateful that you implemented the improved version of this idea with the --cache-store argument! I regret that I wasn't there to help see it over the finish line, and I'm glad you went ahead and merged it.

I just updated my Terraform AWS configuration email-oauth2-proxy-aws to use the --cache-store argument. In my own testing, it works like a charm 🚀

Thanks again!

[simonrob]

It wouldn't have happened at all without your idea and contribution, so no worries at all. Really glad to hear it works well with your tool; thanks again for the work on this.