Skip to content

RFC 310: Add config encryption to ExternalServiceStore

Warren Gifford requested to merge encrypt-extsvc into main

Created by: arussellsaw

closes: #18059 (closed) #18058 (closed)

This PR updates the database.ExternalServiceStore to encrypt & decrypt the config field in and out of the database. This is a compromise as i think we'd rather only encrypt/decrypt last minute, but our config architecture makes this hard to do. eventually we'd like to move to a new format where we can encrypt individual fields, and decrypt them on access. whilst this PR doesn't achieve all of our goals, it does incrementally improve the security of our external service storage.

In defense of a package level variable:

Whilst i agree that package level state is bad, the keyring.Default doesn't necessarily contain any state, but is a set of functions defined by config. if our architecture were at the point where there was a centralised point to inject dependencies, i would definitely opt to inject this into the store, but considering these stores are created ad-hoc all over the place, the scope of this change would be huge, and require threading these dependencies through a huge number of unrelated packages. I think we should aim to migrate to a better injection pattern in the long term, but be pragmatic about how we write code at the moment. I think in this case using dependency injection for this keyring would only stand to introduce more package coupling, and make things worse rather than better.

Merge request reports

Loading