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.

Sign up for GitHub

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

Jump to bottom

RFC072 Artifact Yanking: disallow cookbook removal by owner #1789

Merged
merged 1 commit into from
Dec 12, 2018
Merged

RFC072 Artifact Yanking: disallow cookbook removal by owner #1789

merged 1 commit into from
Dec 12, 2018

Conversation

robbkidd
Copy link
Contributor

@robbkidd robbkidd commented Dec 10, 2018 •
edited
Loading

RFC072 Artifact Yanking

Pertinent thread recording thoughts about unsharing, permanent removal, and the disallowing of such.

TODO for this PR:

  • smoosh the commits with güd words about the whys and wherefores
  • test the omnibus environment variable getting set

@robbkidd robbkidd requested a review from a team December 10, 2018 22:19
@robbkidd
Copy link
Contributor Author

I expect some failures from spec/api/cookbook_*. Seems like there are about three different places the permissions to do things to cookbooks are tested. Trying to figure out if any can be removed because the other one or two cover the cases.

@tyler-ball
Copy link
Contributor

So far, the logic you added LGTM and covers what we talked about. The new test coverage looks good too.

@robbkidd
Copy link
Contributor Author

I'm pondering adding a useful message to the unauthorized response—akin to the message returned for unauthorized attempts to create—to communicate more clearly why owners may no longer be able to destroy cookbooks or cookbook versions.

@jonsmorrow
Copy link

Overall I think this looks good. I'd be fine with improving the unauth response if you want, but I think this is gtg.

marcparadise
marcparadise approved these changes Dec 11, 2018
View reviewed changes
Copy link
Member

@marcparadise marcparadise left a comment

Choose a reason for hiding this comment

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

One testing phrasing nit in-line, otherwise this looks good. As we discussed we'll want to be really clear in the release notes about discussing the security impacts, and the more subtle change of now allowing admins to yank cookbooks.

src/supermarket/spec/authorizers/cookbook_authorizer_spec.rb Outdated
@@ -8,8 +8,13 @@

subject { described_class.new(user, record) }

context 'and owners can remove cookbooks' do
Copy link
Member

Choose a reason for hiding this comment

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

"can remove cookbooks when the server is configured to allow it"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, that original phrasing is no bueno. 🤔

@marcparadise
Copy link
Member

marcparadise commented Dec 11, 2018 •
edited
Loading

I'm pondering adding a useful message to the unauthorized response—akin to the message returned for unauthorized attempts to create—to communicate more clearly why owners may no longer be able to destroy cookbooks or cookbook versions.

Does the UI show the messages? I recall running into issues with chef server where we added nice detail to error messages - only to have it swallowed by knife and manage ui.

@robbkidd
Copy link
Contributor Author

Does the UI show the messages?

Maybe.

There is very intentionally no mechanism in the web UI to remove cookbooks or cookbook versions.

The UI someone will experience for unsharing is the knife supermarket client (or for the masochists, cobbling together an HTTP DELETE request in something else). knife supermarket helpfully shows the message from the server's response when sharing a cookbook, but not necessarily when unsharing.

I think it is still worth having the API response include a message with the unauthorized response providing details.

@robbkidd
Copy link
Contributor Author

Looking at the knife supermarket unshare error handling, we'll need to update that to display the error coming back in the response instead of the client interpreting all 403:Forbidden errors as "you gotta be a maintainer."

@jonsmorrow
Copy link

Good catch on knife. We can do that as a separate card.

@robbkidd @marcparadise @tyler-ball
implement RFC072: only allow admins to remove cookbooks and cookbook …
c10fe49 
…versions

Chef RFC072[1] was accepted as a plan for Supermarket to prevent cookbook
artifacts from disappearing. These changes are a beginning: prevent a
cookbook owner from removing any version of a cookbook or the entire
cookbook itself. This change does not address the proposed hiding
behavior in the RFC.

For backward compatibility, a new configuration option has been added to
enable the previous behavior to allow artifact removal by owners. The
option is set via an environment variable managed by the omnibus
install. The current default is to enable it with the intention to
change the default to disabled in a future major release.

[1] https://github.com/chef/chef-rfc/blob/f8250a4746d2df530b605ecfaa2dc5ae9b7dc7ff/rfc072-artifact-yanking.md

Co-authored-by: Jon Morrow <[email protected]>
Co-authored-by: Marc A. Paradise <[email protected]>
Co-authored-by: tyler-ball <[email protected]>
Signed-off-by: Robb Kidd <[email protected]>
@robbkidd robbkidd force-pushed the robb/disallow-unsharing branch from 7c8614b to c10fe49 Compare December 11, 2018 20:23
@robbkidd
Copy link
Contributor Author

With an omnibus build of this branch:

$ sudo grep OWNERS /opt/supermarket/embedded/service/supermarket/.env.production
export OWNERS_CAN_REMOVE_ARTIFACTS="true"

$ sudo -u supermarket supermarket-ctl console
Loading production environment (Rails 5.0.7.1)
irb(main):001:0> ENV['OWNERS_CAN_REMOVE_ARTIFACTS'] == 'true'
=> true

👍

@robbkidd
Copy link
Contributor Author

Another test with the actual knife supermarket client.

With the current default backward compatible configuration (owners_can_remove_artifacts: true):

> knife supermarket share blargle -m https://superkitchen -o .
Generating metadata for blargle from /var/folders/ck/8bzr4f5j3bnd_prrbh6kyblh0000gn/T/chef-blargle-build20181212-39905-s8n9c3/blargle/metadata.rb
Making tarball blargle.tgz
Upload complete

> knife supermarket unshare blargle -m https://superkitchen
Do you really want to unshare all versions of the cookbook blargle? (Y/N) y
Unshared all versions of the cookbook blargle

With owner artifact removal disabled (owners_can_remove_artifacts: false):

> knife supermarket share blargle -m https://superkitchen -o .
Generating metadata for blargle from /var/folders/ck/8bzr4f5j3bnd_prrbh6kyblh0000gn/T/chef-blargle-build20181212-40659-120izhr/blargle/metadata.rb
Making tarball blargle.tgz
Upload complete

> knife supermarket unshare blargle -m https://superkitchen
Do you really want to unshare all versions of the cookbook blargle? (Y/N) y
ERROR: Forbidden: You must be the maintainer of blargle to unshare it.

☝️ Demonstrating the now-misleading message from knife supermarket.

@robbkidd robbkidd changed the title step towards RFC072 Artifact Yanking: disallow cookbook removal by owner RFC072 Artifact Yanking: disallow cookbook removal by owner Dec 12, 2018
@robbkidd
Copy link
Contributor Author

:shipit: 🚀 🍰

@robbkidd robbkidd merged commit 7660362 into master Dec 12, 2018
@chef-ci chef-ci deleted the robb/disallow-unsharing branch December 12, 2018 16:21
@robbkidd robbkidd mentioned this pull request Dec 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyler-ball tyler-ball tyler-ball approved these changes

@jonsmorrow jonsmorrow jonsmorrow approved these changes

@marcparadise marcparadise marcparadise approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@robbkidd @tyler-ball @jonsmorrow @marcparadise