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.

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

Show gem owners that are not using MFA when logged in as owner #2129

Merged
merged 9 commits into from
Nov 18, 2019

Conversation

lmansur
Copy link
Contributor

@lmansur lmansur commented Sep 17, 2019

As a response to recent gem exploits, we are seeing an increased desire to ensure all gem owners are using MFA.

This PR adds a way for gem owners to check if their fellow owners are using MFA. Regular users can't see this.

The following gif shows the behavior when logged in as an owner and MFA is not enabled for everyone:

rubygem_owner_view

The following screenshot shows the behavior when logged in as an owner and MFA is enabled for everyone:

image

Copy link
Contributor

@eliotsykes eliotsykes left a comment

Choose a reason for hiding this comment

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

Good enhancement 👍

app/assets/javascripts/pages.js Outdated Show resolved Hide resolved
test/unit/helpers/rubygems_helper_test.rb Outdated Show resolved Hide resolved
app/views/rubygems/_gem_members.html.erb Show resolved Hide resolved
@lmansur
Copy link
Contributor Author

lmansur commented Sep 18, 2019

Thank you for your review, @eliotsykes .

Let me know what you think of the changes.

@geemus
Copy link
Contributor

geemus commented Sep 18, 2019

Looks good to me, thanks for taking this on!

@eliotsykes
Copy link
Contributor

Thanks for building this @lmansur. The changes look good.


IIRC, currently any gem owner can silently and without confirmation add another user as an owner.

If this is still true, what risks are there, if any, of the feature in this PR being abused?

One risk might be a malicious owner could add another user as an owner to determine their MFA settings. Is this a problem?

@lmansur
Copy link
Contributor Author

lmansur commented Sep 19, 2019

I think it can be exploited.

However, I also think the benefit of having a way to ensure all owners are using MFA outweighs this risk.

I would love some more input into this matter.

@geemus
Copy link
Contributor

geemus commented Sep 20, 2019

It may be that being able to add people without confirmation is more a bug/coincidence than a feature, but that also seems way out of scope for this particular change.

Is there additional danger associated with knowing that someone doesn't have MFA? You would still need their password, and if you already had their password you would already be able to find out if they had MFA by attempting to use it. Given that, I'm not sure we are any worse off, but I know my security-mindedness is not the strongest. Please let me know if there is a case I'm not thinking about.

@lmansur
Copy link
Contributor Author

lmansur commented Sep 20, 2019

I think it's more a case of specifically targeting owners that don't have MFA and not wasting time with those that do.

It's not ideal, but I wouldn't call that a blocker.

@lmansur
Copy link
Contributor Author

lmansur commented Oct 16, 2019

Anything we can do to move this forward?

test/integration/yank_test.rb Show resolved Hide resolved
config/locales/es.yml Outdated Show resolved Hide resolved
config/locales/fr.yml Show resolved Hide resolved
@bronzdoc
Copy link
Member

@lmansur Thanks for the PR!

This seems like a nice feature It will help create awareness around not using MFA.

@lmansur
Copy link
Contributor Author

lmansur commented Nov 13, 2019

@bronzdoc I appreciate you taking the time to review this!

@mensfeld
Copy link
Member

@geemus I believe it should also include the code for the data dumps export. This info is super valuabvle for probabilistic exploit analysis (cc @colby-swandale @dwradcliffe)

@indirect indirect merged commit a619262 into rubygems:master Nov 18, 2019
@indirect
Copy link
Member

Thanks, this seems like a solid improvement!

@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging November 18, 2019 13:26 Inactive
@geemus
Copy link
Contributor

geemus commented Nov 18, 2019

Thanks!

@lmansur lmansur deleted the add-mfa-check-for-owners branch November 18, 2019 14:28
@lmansur
Copy link
Contributor Author

lmansur commented Nov 18, 2019

Thanks, everyone! 🎉

@sonalkr132 sonalkr132 temporarily deployed to production November 18, 2019 16:20 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants