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

Document security reviewer conflict of interest #247

Merged
merged 5 commits into from Aug 19, 2019

Conversation

lumjjb
Copy link
Collaborator

@lumjjb lumjjb commented Aug 2, 2019

To address #156

Signed-off-by: Brandon Lum lumjjb@gmail.com

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@lumjjb lumjjb requested a review from ultrasaurus August 2, 2019 19:11
assessments/guide/security-reviewer.md Outdated Show resolved Hide resolved
assessments/guide/security-reviewer.md Show resolved Hide resolved
assessments/guide/security-reviewer.md Show resolved Hide resolved
assessments/guide/security-reviewer.md Outdated Show resolved Hide resolved
@lumjjb
Copy link
Collaborator Author

lumjjb commented Aug 7, 2019

From meeting 08/07: Create a process to evaluate conflicts by chairs

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
ultrasaurus
ultrasaurus previously approved these changes Aug 13, 2019
Copy link
Member

@ultrasaurus ultrasaurus left a comment

Choose a reason for hiding this comment

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

Previous comments have been addressed.
Adding @pragashj and @dshaw to review -- ideally we have +1 from all chairs,
but majority is sufficient

@ultrasaurus ultrasaurus requested a review from dshaw August 13, 2019 15:29
pragashj
pragashj previously approved these changes Aug 19, 2019
Copy link
Collaborator

@pragashj pragashj left a comment

Choose a reason for hiding this comment

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

Few minot nits, but otherwise LGTM. Thanks Sarah, for putting this together!


- Individuals from intentionally or unintentionally promote their own company's project
- SIG-Security chairs and assessment leads could intentionally or unintentionally limit the participation of an individual unfairly by asserting conflict of interest
- Security reviews being stalled while groups discuss whether this person or that person should be allowed to participate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a nit
Instead of discuss whether this person or that person how about "groups belabor on who"

security assessment documentation. It is the responsibility of the security reviewer
to make known his/her own conflict of interests.

For each project, 2 SIG-Security chairs must sign off on the conflicts presented to them that the assessment lead has no conflicts, and reviewers have no hard conflicts.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As much as possible we try to avoid conflict, but in cases where we need to go with a conflict, we should document the justification for allowing the conflict. This could be as specific as "This reviewer is the expert in this field and therefore we require their input" or as conventional as "No one else has the bandwidth to take this on at this time so we are letting an exception" -- over time, if we are documenting and reviewing the reason for exception it will allow us to keep the process honest.

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
@lumjjb lumjjb dismissed stale reviews from pragashj and ultrasaurus via cb086ba August 19, 2019 13:31
@lumjjb
Copy link
Collaborator Author

lumjjb commented Aug 19, 2019

@pragashj addressed comments!

@lumjjb lumjjb dismissed stale reviews from pragashj and ultrasaurus via cb086ba 9 minutes ago

It seems like a setting is enabled to dismiss LGTMs on code pushes? @ultrasaurus @pragashj could you review it again?

Do we want to keep this setting? Seems like it's not critical for us to ensure that reviews need to be closely up to date.

@ultrasaurus
Copy link
Member

@lumjjb agreed on the setting... changed to not required. Will take a look at the changes, since it's not retractive!

Copy link
Member

@ultrasaurus ultrasaurus left a comment

Choose a reason for hiding this comment

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

nice edits from @lumjjb based on suggestions from @pragashj

@ultrasaurus ultrasaurus merged commit 5508775 into master Aug 19, 2019
@ultrasaurus ultrasaurus deleted the conflict-of-interest branch August 19, 2019 16:43
Michael-Susu12138 pushed a commit to Michael-Susu12138/tag-security that referenced this pull request Dec 12, 2023
* Document security reviewer conflict of interest
* included feedback from SIG Security meeting and PR feedback
* Approved by two chairs @pragashj and @ultrasaurus 

Signed-off-by: Brandon Lum <lumjjb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants