Build: use scoped credentials for interacting with S3

[stsewd]

The main difference from the other credentials, is that we now need to pass an additional "sesstion_token" value (which django storages calls security_token :_)

This also brings a little more of things to do if you want to actually test it locally, as there isn't an STS service for minio.

We need to create a role with the proper policy for production as described in the dev docs, I guess we need to that from the ops repos? Not sure how to translate that to salt/terraform, I was able to create things for dev using the UI.

And, there may be a little more of work involved to remove the storage credentials from the builder settings, since builders are basically a full working django app, it's expecting to have access to static resources somewhere... dealing with settings :_

Closes https://github.com/readthedocs/readthedocs-ops/issues/1599

[Copilot]

Copilot reviewed 13 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (3)

• docs/dev/aws-temporary-credentials.rst: Language not supported
• docs/dev/index.rst: Language not supported
• docs/dev/settings.rst: Language not supported

Comments suppressed due to low confidence (2)

readthedocs/storage/s3_storage.py:47

• The parameter 'secret_acces_key' is likely misspelled. It should be corrected to 'secret_access_key' to match naming conventions.

secret_acces_key=self.secret_key,

readthedocs/storage/rclone.py:173

• The parameter 'secret_acces_key' appears to be a typo; consider renaming it to 'secret_access_key' to maintain consistency.

secret_acces_key,

[ericholscher]

This is a great step. It's another place I'd like to get another set of eyes on before shipping, but overall makes sense. I don't love that we're integrating more deeply with AWS here, since in general our builds historically have been pretty vendor neutral, but it seems necessary and worth the trade off. I'm also mildly scared of dynamically generating IAM policies, but it's better than * :)

[stsewd]

Had to do some changes in order to support temporary credentials for build_tools, as that also requires access to our buckets (and while they are public, AWS requires you to use valid credentials).

[humitos]

Looks good to me!

Makes sense to put this behind a feature flag since the change it quite big.

I left a few suggestions that I would like them to be considered before merging.

[humitos]

This changes the current semantic of the RTD_ prefix.

We have been using RTD_ prefix for settings specific for Read the Docs. These settings are for third-party applications, so I'd use a different prefix here. I suggest DJANGO_ for all these settings.

[stsewd]

All environment variables that are passed to our application are prefixed with RTD, settings related to the platform are prefixed with RTD (settings.py).

[humitos]

Is this TODO still valid? It seems it's its own setting already.

[stsewd]

This setting is linked to storage, not the region name of the account used to interact with STS, not sure if it matters anyway...

[stsewd]

I could try testing without region and see 🤷‍♂️

[stsewd]

Looks like it's recommend to explicitly set a region, in our case all the resources are from the same account that is in the same region, so it's valid to share the same region with the S3 setting

[stsewd]

We could probably have a AWS_REGION_NAME setting, and AWS_S3_REGION_NAME defaults to that one, but could also just use AWS_S3_REGION_NAME everywhere.