Skip to content

Modify language for reporting signing state#24932

Merged
alisdair merged 1 commit intomasterfrom
signing-language
May 28, 2020
Merged

Modify language for reporting signing state#24932
alisdair merged 1 commit intomasterfrom
signing-language

Conversation

@paultyng
Copy link
Copy Markdown
Contributor

Be more explicit about the signing status of fetched plugins and provide documentation about the different signing options.

@paultyng paultyng force-pushed the signing-language branch from 4f0fb1f to 5217fea Compare May 12, 2020 18:05
programatically. To use partner providers in your Terraform configuration, you need to specify the
provider source, typically this is the namespace and name to download from the registry.
* **Self-signed** - are built, signed, and supported by a third party. HashiCorp does not provide a verification or chain of trust for the signing. You will want to obtain and validate fingerprints manually if you want to ensure you are using a binary you can trust.
* **Unsigned** - Terraform does support fetching and using unsigned binaries, but you should take extreme care when doing so as no programatic authentication is performed on the downloaded binary.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is true, just wanted to cover all the states I saw in code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might, but not from the Registry, which this doc currently lives in the nav hierarchy. Maybe we could clarify that or group them somehow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved the doc, this is top level now under plugins.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't support fetching unsigned providers. The only non-local sources follow the Registry protocol, and signatures are mandatory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I modified it to say this, let me know your thoughts. I wanted to keep it in there, as we will run unsigned, if not fetch them.

@paultyng paultyng requested a review from justincampbell May 12, 2020 18:12
Copy link
Copy Markdown
Contributor

@pkolyvas pkolyvas left a comment

Choose a reason for hiding this comment

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

I've left a comment on the warning.

@paultyng paultyng removed the request for review from justincampbell May 12, 2020 18:26
programatically. To use partner providers in your Terraform configuration, you need to specify the
provider source, typically this is the namespace and name to download from the registry.
* **Self-signed** - are built, signed, and supported by a third party. HashiCorp does not provide a verification or chain of trust for the signing. You will want to obtain and validate fingerprints manually if you want to ensure you are using a binary you can trust.
* **Unsigned** - Terraform does support fetching and using unsigned binaries, but you should take extreme care when doing so as no programatic authentication is performed on the downloaded binary.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might, but not from the Registry, which this doc currently lives in the nav hierarchy. Maybe we could clarify that or group them somehow.

Copy link
Copy Markdown
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

The structure of the code makes sense to me. I left some comments inline, mostly about the documentation.

Terraform plugin signing trust levels
---

# Plugin Signing
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This document describes plugins in general, but Terraform's tiered signature verification only applies to providers specifically.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in this case i was hedging a little on the future, as this is a URL hard coded in to a shipped binary, was making it a bit more future proof. I can add some language though that currently only provider plugins are authenticated.

verified the ownership of the private key and we provide a chain of trust to the CLI to verify this
programatically. To use partner providers in your Terraform configuration, you need to specify the
provider source, typically this is the namespace and name to download from the registry.
* **Self-signed** - are built, signed, and supported by a third party. HashiCorp does not provide a verification or chain of trust for the signing. You will want to obtain and validate fingerprints manually if you want to ensure you are using a binary you can trust.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You will want to obtain and validate fingerprints manually if you want to ensure you are using a binary you can trust.

It's not clear to me how a user could take action on this recommendation, unless we give some more detail about how to obtain the key fingerprint, and how to usefully validate it. As I understand it that is the subject of some unspecified future work. Until we have something more concrete to offer here, I think it's better to omit this sentence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Until we offer an ability to authenticate, perhaps we should print a fingerprint for this self-signed category to the output for manual verification?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@paultyng we need to limit scope creep, but we will definitely consider that suggestion for a future enhancement. We don't have concrete plans yet, but a number of ideas to extend the community provider trust process in later releases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added in this PR to output KeyID when its third party signed, maybe that will be sufficient?

@paultyng paultyng force-pushed the signing-language branch 2 times, most recently from df28d4c to a141ee7 Compare May 13, 2020 17:36
@codecov
Copy link
Copy Markdown

codecov bot commented May 13, 2020

Codecov Report

Merging #24932 into master will decrease coverage by 0.01%.
The diff coverage is 52.38%.

Impacted Files Coverage Δ
internal/providercache/installer_events.go 42.85% <ø> (ø)
command/init.go 67.07% <31.25%> (-1.20%) ⬇️
internal/providercache/installer.go 20.57% <40.00%> (+0.08%) ⬆️
internal/getproviders/package_authentication.go 88.33% <71.42%> (-4.46%) ⬇️
dag/walk.go 92.54% <0.00%> (-0.63%) ⬇️

Copy link
Copy Markdown
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

👍 from me once the remaining minor issues are addressed. Thanks!

@danieldreier danieldreier added the waiting-response An issue/pull request is waiting for a response from the community label May 21, 2020
@paultyng paultyng force-pushed the signing-language branch 2 times, most recently from cf3fc68 to 8130fae Compare May 26, 2020 17:12
Be more explicit about the signing status of fetched plugins and provide documentation about the different signing options.
@paultyng paultyng force-pushed the signing-language branch from 8130fae to 22ef5cc Compare May 26, 2020 17:14
@paultyng paultyng marked this pull request as ready for review May 26, 2020 17:15
@paultyng paultyng removed the waiting-response An issue/pull request is waiting for a response from the community label May 26, 2020
@paultyng
Copy link
Copy Markdown
Contributor Author

The warning was removed after discussions with @alisdair as it was not longer used.

Copy link
Copy Markdown
Contributor

@justincampbell justincampbell left a comment

Choose a reason for hiding this comment

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

Language and docs look good to me!

Copy link
Copy Markdown
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Thank you!

@alisdair alisdair merged commit ef28671 into master May 28, 2020
@alisdair alisdair deleted the signing-language branch May 28, 2020 13:09
@ghost
Copy link
Copy Markdown

ghost commented Jun 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants