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

Mandate minimum TLS version of 1.2 for all storage accounts #2133

Merged
merged 4 commits into from
Aug 16, 2024

Conversation

craddm
Copy link
Contributor

@craddm craddm commented Aug 15, 2024

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

⤴️ Summary

As per pentest report, storage accounts in SREs currently allow a minimum TLS version of 1.0, which is deprecated.

Note that this also applies to the SHM storage account, and this PR also modifies the minimum version there.

🌂 Related issues

🔬 Tests

Deployed SRE using these changes.

  • mounting the accounts inside the SRE
  • reading/writing from inside the SRE
  • accessing them from Azure Store Explorer

Sorry, something went wrong.

@craddm craddm changed the base branch from develop to release-v5.0.0 August 15, 2024 13:21
@jemrobinson
Copy link
Member

Is this a from-scratch SRE? I'd like to know whether this causes any problems for any of:

  • mounting the accounts inside the SRE
  • reading/writing (as appropriate) from inside the SRE
  • accessing them from Azure Storage Explorer

Could you give details on this issue when you have them?

@craddm
Copy link
Contributor Author

craddm commented Aug 15, 2024

Is this a from-scratch SRE? I'd like to know whether this causes any problems for any of:

  • mounting the accounts inside the SRE
  • reading/writing (as appropriate) from inside the SRE
  • accessing them from Azure Storage Explorer

Could you give details on this issue when you have them?

It isn't a from scratch SRE - I deleted and redeployed the storage accounts. However, currently having issues restarting the gitea container group. Going to try a fresh deployment instead.

Copy link

github-actions bot commented Aug 15, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

craddm added 2 commits August 15, 2024 14:04

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@craddm
Copy link
Contributor Author

craddm commented Aug 15, 2024

Is this a from-scratch SRE? I'd like to know whether this causes any problems for any of:

  • mounting the accounts inside the SRE
  • reading/writing (as appropriate) from inside the SRE
  • accessing them from Azure Storage Explorer

Could you give details on this issue when you have them?

All worked fine as far as I can tell. Successfully uploaded and downloaded from ingress and egress via storage explore. Successfully copied from input and to output from the SRE desktop

@craddm craddm marked this pull request as ready for review August 15, 2024 16:25
@craddm craddm requested a review from a team as a code owner August 15, 2024 16:25
@jemrobinson
Copy link
Member

Cool - that's /ingress and /egress. Can you check /home (presumably working if you logged in) and /shared as well?

@JimMadge JimMadge changed the title [WIP] Mandate minimum TLS version of 1.2 for all storage accounts Mandate minimum TLS version of 1.2 for all storage accounts Aug 16, 2024
Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

LGTM. We should be careful to test this thoroughly though as we are merging to the release branch.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jemrobinson
Copy link
Member

@JimMadge : I'm comfortable that Matt has tested /data and /output (also /home and maybe /shared). Do you think we need/want an independent test of these?

@craddm
Copy link
Contributor Author

craddm commented Aug 16, 2024

@JimMadge : I'm comfortable that Matt has tested /data and /output (also /home and maybe /shared). Do you think we need/want an independent test of these?

Yes, home and shared working fine too

Just now deployed an SHM with the updated TLS too

@jemrobinson
Copy link
Member

Happy to merge this @JimMadge ?

@JimMadge JimMadge merged commit 583bd77 into alan-turing-institute:release-v5.0.0 Aug 16, 2024
11 checks passed
@craddm craddm deleted the storage-tls branch August 29, 2024 11:31
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

3 participants