Skip to content

Add Rake task for inserting pre-generated access token into database #368

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lfdebrux
Copy link
Member

@lfdebrux lfdebrux commented Nov 14, 2023

What problem does this pull request solve?

Currently our method for creating access tokens for developers on the team is to rely on there being a developer around who already has API access [1].

However, at the moment there aren't very many developers already in the access tokens database, which creates a bit of a single point of failure problem.

This PR adds an alternate method; if the developer who wants access already has access to the support role in production (which is more likely than them already having an API key), they can use this task to insert the hash of a key they pre-generate into the forms-api database.

To use this to add a token to the development app, for example, you can run in your shell:

TOKEN="$(ruby -e 'require "securerandom"; puts "forms_#{SecureRandom.uuid}"')"
TOKEN_DIGEST="$(ruby -e "require 'digest'; puts Digest::SHA256.hexdigest('$TOKEN')")"

gds aws forms-dev-support -- forms run_task -a api -c "access_tokens:insert[$USER, $TOKEN_DIGEST]"

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Has all relevant documentation been updated?

@lfdebrux lfdebrux marked this pull request as draft November 14, 2023 06:47
@lfdebrux lfdebrux force-pushed the ldeb-add-access-tokens-task branch 2 times, most recently from 6f68f7b to 34f3891 Compare November 14, 2023 07:01
@lfdebrux lfdebrux force-pushed the ldeb-add-access-tokens-task branch from 34f3891 to 7293c37 Compare November 22, 2023 08:40
@lfdebrux lfdebrux marked this pull request as ready for review November 22, 2023 08:40
Copy link
Contributor

@jamie-o-wilkinson jamie-o-wilkinson left a comment

Choose a reason for hiding this comment

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

I think this a good idea, it makes sense to be able to generate access tokens by authorising with AWS, rather than needing to get an access token from another team member. It also consolidates how we give permissions to individuals which is nice.

@lfdebrux lfdebrux added this pull request to the merge queue Nov 29, 2023
@lfdebrux lfdebrux removed this pull request from the merge queue due to a manual request Nov 29, 2023

access_token = AccessToken.create!(
description: "inserted with `rails access_tokens:insert`",
owner: ENV["USER"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking dev where I presume you've run this and the user is listed as ruby. The user is intended to be the name of the developer (or app such as forms-runner) who owns the key and I think that's important to maintain.

bash-3.2$ forms data_api -d forms-api -s "select DISTINCT owner from access_tokens where description LIKE 'inserted with%';" | jq '.records[]' -r
{
  "owner": "ruby"
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point. I wasn't sure how to get the user information in, I thought that maybe it didn't matter so much, but if you think it does I'll have another think about it!

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah the reasons it's important are that we use the owner in our logs to log who made the API call (and it'll be important for general access auditing):

payload[:requested_by] = "#{@access_token.owner}-#{@access_token.id}" if @access_token.present?

And we'll need to use it for management of those keys (for example when someone leaves) and to notify people for key rotation etc

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed the task slightly to add the owner as a parameter as well as the token digest.

I did spend a while looking into whether it was possible to get the AWS user who started the EC2 task out of the ECS metadata somehow, but I couldn't find anything straightforward.

@lfdebrux lfdebrux marked this pull request as draft December 1, 2023 15:21
@lfdebrux lfdebrux force-pushed the ldeb-add-access-tokens-task branch 2 times, most recently from 904e4f0 to bd2bc48 Compare December 15, 2023 11:51
@lfdebrux lfdebrux marked this pull request as ready for review December 15, 2023 11:52
@lfdebrux lfdebrux mentioned this pull request Dec 15, 2023
This task allows inserting a token into the access tokens table without
already having an API token.

To use this, generate a UUID or other random string, and then pass the
hex encoded sha256 digest of that to the `insert` task:

```bash
TOKEN="$(ruby -e 'require "securerandom"; puts "forms_#{SecureRandom.uuid}"')"
TOKEN_DIGEST="$(ruby -e "require 'digest'; puts Digest::SHA256.hexdigest('$TOKEN')")"

bin/rails "access_tokens:insert[$USER, $TOKEN_DIGEST]"
```

Then don't forget to save the token somewhere safe!
@lfdebrux lfdebrux force-pushed the ldeb-add-access-tokens-task branch from bd2bc48 to 9fc4db7 Compare December 15, 2023 13:17
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@aliuk2012
Copy link
Contributor

What is the need to pre-generate an access token? I'm not a huge fan or pre-generating because its then left done to the dev to create or pass in whatever they like and potentially expose our system with a weak token. We already have a method on the AccessToken model which will generate the token for us, https://github.com/alphagov/forms-api/blob/main/app/models/access_token.rb#L6.

I understand the issue of devs potentially being locked out of a service because there hasn't been an initial token generated. This is because api endpoints require a valid token but if we only need this to create new tokens then I think we should still be letting the model define that the token for that use is.

Comment on lines +7 to +12
access_token = AccessToken.create!(
description: "inserted with `rails access_tokens:insert`",
owner: args[:owner],
token_digest: args[:token_digest],
)
pp(access_token)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rough example of how we might want to do it without leaving it down to the dev to generate/use a strong token

Suggested change
access_token = AccessToken.create!(
description: "inserted with `rails access_tokens:insert`",
owner: args[:owner],
token_digest: args[:token_digest],
)
pp(access_token)
access_token = AccessToken.new(
description: "inserted with `rails access_tokens:insert`",
owner: args[:owner])
access_token.generate_token
if access_token.save
pp(access_token)
end

@lfdebrux
Copy link
Member Author

lfdebrux commented Dec 18, 2023

What is the need to pre-generate an access token? I'm not a huge fan or pre-generating because its then left done to the dev to create or pass in whatever they like and potentially expose our system with a weak token. We already have a method on the AccessToken model which will generate the token for us, https://github.com/alphagov/forms-api/blob/main/app/models/access_token.rb#L6.

I understand the issue of devs potentially being locked out of a service because there hasn't been an initial token generated. This is because api endpoints require a valid token but if we only need this to create new tokens then I think we should still be letting the model define that the token for that use is.

@aliuk2012 I did think about how to use the existing token generation code, however the problem I was concerned about is that any output from running a task is logged to AWS CloudWatch. So the token that is generated would be readable to anyone with access to AWS.

@aliuk2012
Copy link
Contributor

@lfdebrux interesting, both approaches could open up our service to attack. Lets have a chat about it as a team and see how much risk we are prepared to take on.

@lfdebrux lfdebrux marked this pull request as draft December 18, 2023 11:36
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.

4 participants