PSS: Add interactive confirmation of state storage provider trust when initialising a state store for the first time#38395
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
2988e59 to
2a398fd
Compare
cb46802 to
aa50304
Compare
e65292f to
7945eb3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c47c251 to
03f4de7
Compare
| var authentication string | ||
| if authResult != nil && authResult.KeyID != "" { | ||
| authentication = fmt.Sprintf("%s, key ID %s", authResult.String(), authResult.KeyID) | ||
| } else { | ||
| authentication = authResult.String() | ||
| } |
There was a problem hiding this comment.
As the (AI?) comment also noted, this is an area that I'd like to get some feedback on in the review process. I think that the auth result is only populated when I download a provider from the Registry, which makes it hard to test.
Given that the prompt is only shown when a provider is downloaded via HTTP I think we always expect there to be an auth result when a prompt is shown. My test simulating download via HTTP is highly artificial, and it's unclear to me right now what the gap is there versus downloading a provider from a Registry or a network mirror.
There was a problem hiding this comment.
Researching this some more today...
There was a problem hiding this comment.
Ok, so here's my understanding:
The process of installing providers gets meta data for a given provider here. An installer can include multiple sources of providers, and the presence of Authentication metadata (that this thread focuses on) depends on the source that's in play.
Irrelevant sources, which just wrap other sources: MemoizeSource, MultiSource
Sources used by end-users:
- FilesystemMirrorSource
- HTTPMirrorSource
- RegistrySource (uses registryClient internally)
Sources used in our tests:
The Authentication metadata returned from these sources vary:
FilesystemMirrorSourcenever has Authentication metadata:- Obtains provider metadata via SearchLocalDirectory, which never sets Authentication metadata (also, note the FIXMEs -lol)
HTTPMirrorSourceonly has Authentication metadata if the network mirror's response data includes hashes - not guaranteed.- Sets meta data directly in the PackageMeta method. Here, Authentication is conditionally set depending on whether the network mirror sends information about expected hashes. See example network mirror response data that does contain hashes.
RegistrySourcealways has Authentication metadata- Gets meta data via
(registryClient) PackageMeta
- Gets meta data via
MockSourcereturns Authentication metadata like this.
The features in this PR are only active when a provider is downloaded via HTTP. So, only HTTPMirrorSource and RegistrySource are relevant. The only time when we'd have a lack of Authentication data is when using a network mirror and that network mirror doesn't include any hash data for the state store provider.
There was a problem hiding this comment.
I could look at enabling tests to use an HTTPMirrorSource in combination with a test server - I'll look into that next week.
d67c9ab to
c6c30a6
Compare
…ing the new newMockProviderSourceUsingTestHttpServer method
…ow finer control of callbacks when implementing security related features
…use, and only if downloaded via HTTP.
… method I originally added this to have more control over what we render in the prompt, but I'm walking this back before seeking feedback in review. Could (*PackageAuthenticationResult) .String() be sufficient?
…k mirror doesn't include hashes
…ining reused callbacks Co-authored-by: Copilot <copilot@github.com>
972ac1f to
95bd13f
Compare
SarahFrench
left a comment
There was a problem hiding this comment.
Some quick comments for reviewers
| return "Prepare your working directory for other commands" | ||
| } | ||
|
|
||
| // Returns a reused callback function for the ProviderAlreadyInstalled event in a providercache.InstallerEvents struct. |
There was a problem hiding this comment.
This section of many, many callbacks was essentially a copy-paste job from getProvidersFromConfig and then working out what variables were previously caught by closures and now needed to be explicit arguments for creating the callback.
| // 2. Call the shared callback for FetchPackageSuccess | ||
| cb := fetchPackageSuccessCallback(view) | ||
| cb(provider, version, localDir, authResult) |
There was a problem hiding this comment.
Whereas PendingProviders and QueryPackagesBegin have their own logic above due to needing specific message content to send to the view, the FetchPackageSuccess callback is handling distinct tasks and I think using a callback like this helps highlight what's special logic for this calling code .
mildwonkey
left a comment
There was a problem hiding this comment.
love the changes to events, thank you!
Closes https://hashicorp.atlassian.net/browse/TF-33680
This is a reworking of discovery in #38205. Following design changes, we no longer expect a new flag to be used in this feature.
The security features added in this PR will be active during
initonly. They'll only be active when the state store provider is being installed for the first time, via HTTP. We believe using provider from a local cache or filesystem mirror shows that trust has already been established for the provider so the prompt is not shown then. See the ticket for more information.Given that all changes to a state store are going to be implemented in a new
state migratecommand (see #38388), the work in this PR will need to be replicated in the newstate migratecommand.Reviewer 👋🏻
It's worth going commit by commit in this PR, given that I start with this change 7f9c48c because that obscures how I later change the callbacks slightly ingetProvidersFromConfigHang in there! 😉
Target Release
1.16.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry