1515100 - Display warning about not leaking information in Lando for confidential revisions

Status: RESOLVED FIXED

(Conduit :: Lando, enhancement)

Description

[Mark Côté [:mcote]]

When someone is about to land a confidential revision, we should warn them about not including revealing information in the commit message.  There are some guidelines at https://wiki.mozilla.org/Security/Bug_Approval_Process.  Although we don't follow all of them exactly, there is some good information there about how to be careful with security patches.

There is more we can and should do to automatically enforce and/or help with some of these policies, but we should start with a simple warning.

Comment 1

[Israel Madueme [:imadueme]]

Related: https://docs.google.com/document/d/1B5OdmfVRj8kMHktcOkExEIg4eCGjXwIIehbvrtZbPbc

Comment 2

[Israel Madueme [:imadueme]]

So I've verified that BMO already tags a revision with the proper security group project. These projects are accessible via the 'projects' attachment in differential.revision.search.

One problem: I've run into a situation where it could be that a reviewer or subscriber tasked with landing the revision isn't part of the security group connected to that project tag. Phabricator doesn't let you query project information if you aren't a part of that group. Because lando-api uses the user's api key when landing things, we're in the situation where the user can see the revision (since they are a reviewer) but can't see the project.

We'd either need to have lando-api have an all seeing api key, or use the fact that the user can query the revision but not the project to assume it is a confidential revision.

Let me know how you'd like to proceed with this or if I'm misunderstanding something in the doc. Thanks!

Comment 3

[Steven MacLeod [:smacleod]]

(In reply to Israel Madueme [:imadueme] from comment #2)

Phabricator doesn't let you query project information if you aren't a part of that group.

Am I correct that you mean project.search will not allow you to query the project, but differential.revision.search will still return the PHID for the project in the projects attachment? In other words, we can find the PHIDs of the projects, but can't query those PHIDs to find more project information?

If I'm correct:

We'd either need to have lando-api have an all seeing api key, or use the fact that the user can query the revision but not the project to assume it is a confidential revision.

I think a good solution here would be to provide the #secure-revision project's PHID as configuration to Lando. We don't really care about differentiating between different types of security groups, just whether something is confidential or not (at least for now). We could then not bother querying the projects at all, just checking the projects attachment for the PHID we care about.

From your investigation so far do you believe that would work?

Comment 4

[Israel Madueme [:imadueme]]

(In reply to Steven MacLeod [:smacleod] from comment #3)

Am I correct that you mean project.search will not allow you to query the project, but differential.revision.search will still return the PHID for the project in the projects attachment? In other words, we can find the PHIDs of the projects, but can't query those PHIDs to find more project information?

Yup, exactly.

We'd either need to have lando-api have an all seeing api key, or use the fact that the user can query the revision but not the project to assume it is a confidential revision.

I think a good solution here would be to provide the #secure-revision project's PHID as configuration to Lando. We don't really care about differentiating between different types of security groups, just whether something is confidential or not (at least for now). We could then not bother querying the projects at all, just checking the projects attachment for the PHID we care about.

This will work, but, do we care about only showing these messages for specific repos? E.g. the same message shows up for mozilla-central and vct? I guess this is at least a good start.

Comment 5

[Steven MacLeod [:smacleod]]

(In reply to Israel Madueme [:imadueme] from comment #4)

This will work, but, do we care about only showing these messages for specific repos? E.g. the same message shows up for mozilla-central and vct? I guess this is at least a good start.

Ya, this can be all repositories. Our focus is Firefox but it's not a big deal to warn other people when landing sensitive changes.

Comment 6

[Israel Madueme [:imadueme]]

Attachment: transplants: Add security bug approval process warning (bug 1515100)

Prior to this commit lando-api could not differentiate between a secure
revision and a public revision; it could only see or not see a revision.
This change allows the api to inform clients if a revision that they can
see is a security revision.

Comment 7

[Israel Madueme [:imadueme]]

Attachment: template_helpers: autolink to Security Bug Approval Process wiki (bug 1515100)

Linkify 'Security Bug Approval Process' to the appropriate wiki in landing warnings.