Proposal: trust model around custom plugin indexes

[ahmetb]

Related: #483

Currently every plugin installed/upgraded from krew-index comes with a security warning:

WARNING: You installed plugin "ns" from the krew-index plugin repository.
   These plugins are not audited for security by the Krew maintainers.
   Run them at your own risk.

The reason we added this is because users must understand that they are downloading potentially unvetted binaries.

With the fact that multiple indexes are now happening, this opens several possibilities to rethink this experience, given:

• adding a plugin index is an explicit operation for the user (or IT admin)
• default index (krew-index) becomes just another index that user (or IT admin) can remove

Proposal 1

• Trust all "custom" indexes implicitly.

  • do not print security warning during install
  • (maybe) add a warning while the index is added via "krew index add" command

• default index (i.e. krew-index) would still print installation warnings

  1. krew-index is still what 90% of our users will be using
  2. krew-index is added automatically (though, we haven't discussed how that looks like in a multi-index mode, but it's likely we don't want explicit user interaction as that'd break many user journeys)

Notes:

• does a custom treatment of "default" index
• what if "default" isn't actually pointing to krew-index repository?

Proposal 2

Trust "all" indexes explicitly on the first use.

• First time someone tries to install a plugin through "krew install", we present an interactive (Y/n?) consent with the security message.
• The "trust" action is stored on disk (likely through a file indicator)
• Successive "krew install" commands using that index don't cause security messages.

Notes:

• Most first-time users of krew will see an interactive warning, once (as they're installing via krew-index)
  • Does an "interactive prompt" actually achieve something –or are we just covering our back with this warning? If the user meant to run a command, they are very likely to say "no" –but they should be aware of the risks.
• Is trusting an index once a good model? e.g. users might forget the risks several months after acknowledging the centralized krew-index
• How does it work non-interactively (e.g. krew install < myplugins.txt)? (Likely we'd need to fail and add a --trust/--quiet flag to install which I'm not super eager about.)
• Introduces new files and logic to store this "trust".
  • The logic must be able to handle when index is removed and re-added (potentially with a new git remote)
• How does the user revoke "trust" action? (I don't want to add cmds or flags about trust.)
• No special treatment of krew-index (existing users also go through this)

/kind proposal
/area multi-index
/priority important-soon
/cc @corneliusweig
/cc @chriskim06

[corneliusweig]

A major consideration for me is that many users only run krew very occasionally. Those users will not benefit from a one-time warning, because it is easily forgotten. Thus, we'd just cover our back, as you said.

Since this is a security issue, I'd rather print that warning too often than too seldom. And our approach to show it after every install fulfils its purpose well. Frequent users know the warning anyway and won't even look at it.

Consequently, I would not automatically add trust to custom indexes.

---

On the other hand, for corporate setups where admins want to distribute plugins from a vetted registry should somehow have the capability to add that trust. I think a good way is to add two more index commands:

Option 3

krew index trust my-index
krew index untrust my-index

These commands will also give us the opportunity to print an extra-nasty warning that trusting an untrustworthy source can have bad consequences.

This does increase the API surface of the cli, but the complexity is low. In contrast, interactive commands+workarounds sound much more complex.

[ahmetb]

IMO adding commands (e.g. krew index trust) that we can't remove later isn't that great either (I'm -1 on that for now). By adding commands for it, we're making it a concept users will find out and try to understand how to use.

We currently don't know how much the users/IT admins care about this. I'll try to go to #helm chat on Slack and ask around how/if security warnings are handled.

[corneliusweig]

Well another idea is to make this fully manual like echo my-index >> $HOME/.krew/trust. This should be easy enough to do for sysadmins and we don't make an API contract. This way of adding trust is nothing we can communicate to normal users, for example as part of the security warning.
I don't share your concern about adding that command though. It's straightforward and we can even hide it via cobra. An interactive dialog worries me much more.

Anyway, finding out the demand for this feature is the best next step.

[ahmetb]

I think I'm good with an implicit method (a file with trusted index list –OR– per-index sentinel file indicating trust –OR– an env var like KREW_TRUST_INDEXES)

OK, so it seems like we're leaning towards:

• show security warning for all indexes (even for user/admin-configured ones)
• provide an intentionally "less documented" escape hatch (tentatively)
• wait for someone to complain and then pick a direction from there

Correct?

[corneliusweig]

wait for someone to complain and then pick a direction from there
😆

Yes, I fully agree!

@chriskim06 what's your take on this?

[chriskim06]

show security warning for all indexes (even for user/admin-configured ones)

this sounds good to me. It also seems like the simplest path going forward.

wait for someone to complain and then pick a direction from there

I also agree with this. I feel like providing a way to "trust" plugins/indexes could wait until people complain about excessive warnings. But I also have less experience with this than you two so would defer to your judgment on it.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[corneliusweig]

/lifecycle frozen
Let's come back to this when multi-index work is done.

[dirtycajunrice]

This hasnt moved in a while so i'd like to chime in and request initially just the ability to configure an environment variable to silence the warning. KREW_UNTRUSTED_WARNING=false. I understand it may be few and far between when a new plugin is installed... but this also shows on updates.

[ahmetb]

The problem with those knobs is that it's yet another thing to know about for the user.
@corneliusweig but I believe showing the warning on version upgrades sounds like a bug. I don't think we had that at the beginning. it might've creeped in.

[dirtycajunrice]

The functionality has always been to fill your terminal with a massive wall of text most of us are acutely aware of already. Adding a user defined override that has to be explicitly looked for in the docs and intentionally set to be enabled is, in my opinion, the opposite of something the user has to be aware of. I would safely assume the significant majority would never even know a toggle option became available and the functionality stays status quo

[corneliusweig]

IIRC, we decided to show the warning on updates, because plugin updates do not get vetted at all and could ship entirely different software.
How about showing the warning just a single time instead for every updated plugin? Would that be good enough?

[dirtycajunrice]

IIRC, we decided to show the warning on updates, because plugin updates do not get vetted at all and could ship entirely different software.

How about showing the warning just a single time instead for every updated plugin? Would that be good enough?

The output verbosity and its warning is a statement that currently stands for every single plugin. Showing it sometimes is the same as showing it all of the time.

When krew has an official vetted repo option, the warnings would fit the purpose. When it is simply the application as a whole, it is obtrusive and repetitive. You have the option to "trust" a self signed certificate, which squelches the warning for that cert until it expires. This, essentially, is the same flow.

[corneliusweig]

Are you saying that this is not good enough?

How about showing the warning just a single time instead for every updated plugin? Would that be good enough?

[dirtycajunrice]

Correct. Was trying to give a why and not just a "no".

[subhamkrai]

@ahmetb @chriskim06 @corneliusweig any further movement on this discussion? The log message is of little concern for users if they see it every time.

Ideas:

1. I liked the idea of giving users interactive y/n for the first time installation or during the last upgrade for the users who already have the plugin.
2. Also, softening the warning message a little bit until we have the proper solution to the discussion will help us a lot. Thanks

[ahmetb]

I'm not a big fan of introducing prompts, but open to disabling the message on upgrades (as discussed above, it wasn't intentional to begin with), as well as choosing a softer message that would panic the users. Open to alternative message proposals.

[travisn]

What about softening the message by simply removing this sentence? Run them at your own risk.
The point about the plugins not being audited for security is still clear. IMO it's just that last sentence that makes it more alarming than it needs to be.

[ahmetb]

SGTM.

[yermulnik]

Is there still no option to suppress that warning message?