Security/Reviews/AutomatedLanding
Jump to navigation
Jump to search
Full Query
Full Query
Please use "Edit with form" above to edit this page.
Item Reviewed
| Automated/Assisted landing from Bugzilla to tip of $branch | |||||||||
| Target |
1 Total; 0 Open (0%); 1 Resolved (100%); 0 Verified (0%); |
||||||||
{{#set:SecReview name=Automated/Assisted landing from Bugzilla to tip of $branch
|SecReview target=
| ID | Summary | Priority | Status |
|---|---|---|---|
| 721923 | Security Review request for Automated/Assisted landing from Bugzilla to tip of $branch | P4 | RESOLVED |
1 Total; 0 Open (0%); 1 Resolved (100%); 0 Verified (0%);
}}
Introduce the Feature
Goal of Feature, what is trying to be achieved (problem solved, use cases, etc)
- from bugzilla a flag can be set to land bugs on try or branch
- been done with whiteboard flags and bugzilla api so far
- uses an autoland queue and an HG push to manage the jobs
- user requesting has to have proper permissions to do this, checked against LDAP
(from team)
- Show staging bug with Autoland cycle completed (real world bug, current usage)
- Demonstrate the BMO extension for input (on staging, no API available, so SSL for testing)
- Explain the logic that is happening over on autoland-master (three main modules: autoland_queue, schedulerdbpoller, hg_pusher)
- Allowing developers to save time applying/landing patches for contributors (or themselves)
- Allowing release drivers to backport patches and save dev time (they continue on with current work unless there are issues backporting)
- Better tracking in the bug of what has landed on what branches (bug comments == permanent record)
What solutions/approaches were considered other than the proposed solution?
- current solution is using whiteboard tags
- problem is that checking branch permissions, as we don't know who flagged the job
- use bugzilla history to see who added the whiteboard tag? :/
- problem is that checking branch permissions, as we don't know who flagged the job
- extension allows for this permission checking
- on bz side there is a group that restricts the ability to see the option
Why was this solution chosen?
- Security checking done on Bugzilla side (using hg group)
- Easy to check pushing permissions, since we are (easily) given the pushing users information
Any security threats already considered in the design and why?
- Ensuring a valid SSL connection with the BMO extension's Web Service
- Ensuring that the ldap tool is up to date and that people have correct repo permissions (Autoland user has high level of permissions so can push anywhere)
Threat Brainstorming
- LDAP checking
- does this mean the bugzilla username has to match the hg username?
- not necessarily, we can check for employees against a bugzillaMail field in ldap
- for mozilla.net (non employee) there is no current access if they use a non-ldap email address but that can be added in the future
- does this mean the bugzilla username has to match the hg username?
- Access is separate(?) from L1/L2/L3 access for hg
- We're aiming for LDAP support first, we'll expand to other contributors later.
- having release drivers suddenly fully responsible for branch merges scares me. developers know about conflicts that hg can't detect. and sometimes they know about regressions or regression risk.
tools/scripts/autoland/config.ini password storage
- identify policies for password storage at a minimum, they should include:
- restrictive file permissions - the accounts used by these services should have the minimum set of permissions possible - more than average detail of logging of usage of these accounts - accounts should be unique to this application, not some kind of generic service account
- push comes from autoland user, commit comes from patch author (as listed in the patch headers)
- yes, from patch headers (or autoland extension setter for try)
- what if patch headers are missing?
- depends on branch (per-branch policies, eg: try doesn't require patch headers)
- what if patch headers are a lie?
- it's the responsibility of the person adding the autoland whiteboard tag to check this
- :/ i'm not sure all the patch-viewing tools in bugzilla even show this metadata
- i'd prefer if bugzilla flagged patches where the attacher and commit-author are different people
- :/ i'm not sure all the patch-viewing tools in bugzilla even show this metadata
- it's the responsibility of the person adding the autoland whiteboard tag to check this
- [dveditz] previously, commits were protected by SSH keys. now, they are protected by bugzilla passwords. bugzilla users (who are not in the security group) are not going to be as careful with their bugzilla passwords (or email accounts).
- require a more secure password-reset procedure for users who have autoland enabled?
- audit trail?
- log (includes some Try-spam stuff)
- search pushlog by user (for each repo you care about)
- reviewers are checked. does this allow self-review / carryover reviews?
- allowing it means you only have to hack one bugzilla account (as long as the account is allowed to review patches)
- disallowing it makes the feature less convenient
{{#set: SecReview feature goal=* from bugzilla a flag can be set to land bugs on try or branch
- been done with whiteboard flags and bugzilla api so far
- uses an autoland queue and an HG push to manage the jobs
- user requesting has to have proper permissions to do this, checked against LDAP
(from team)
- Show staging bug with Autoland cycle completed (real world bug, current usage)
- Demonstrate the BMO extension for input (on staging, no API available, so SSL for testing)
- Explain the logic that is happening over on autoland-master (three main modules: autoland_queue, schedulerdbpoller, hg_pusher)
- Allowing developers to save time applying/landing patches for contributors (or themselves)
- Allowing release drivers to backport patches and save dev time (they continue on with current work unless there are issues backporting)
- Better tracking in the bug of what has landed on what branches (bug comments == permanent record)
|SecReview alt solutions=* current solution is using whiteboard tags
- problem is that checking branch permissions, as we don't know who flagged the job
- use bugzilla history to see who added the whiteboard tag? :/
- problem is that checking branch permissions, as we don't know who flagged the job
- extension allows for this permission checking
- on bz side there is a group that restricts the ability to see the option
|SecReview solution chosen=* Security checking done on Bugzilla side (using hg group)
- Easy to check pushing permissions, since we are (easily) given the pushing users information
|SecReview threats considered=* Ensuring a valid SSL connection with the BMO extension's Web Service
- Ensuring that the ldap tool is up to date and that people have correct repo permissions (Autoland user has high level of permissions so can push anywhere)
|SecReview threat brainstorming=* LDAP checking
- does this mean the bugzilla username has to match the hg username?
- not necessarily, we can check for employees against a bugzillaMail field in ldap
- for mozilla.net (non employee) there is no current access if they use a non-ldap email address but that can be added in the future
- does this mean the bugzilla username has to match the hg username?
- Access is separate(?) from L1/L2/L3 access for hg
- We're aiming for LDAP support first, we'll expand to other contributors later.
- having release drivers suddenly fully responsible for branch merges scares me. developers know about conflicts that hg can't detect. and sometimes they know about regressions or regression risk.
tools/scripts/autoland/config.ini password storage
- identify policies for password storage at a minimum, they should include:
- restrictive file permissions - the accounts used by these services should have the minimum set of permissions possible - more than average detail of logging of usage of these accounts - accounts should be unique to this application, not some kind of generic service account
- push comes from autoland user, commit comes from patch author (as listed in the patch headers)
- yes, from patch headers (or autoland extension setter for try)
- what if patch headers are missing?
- depends on branch (per-branch policies, eg: try doesn't require patch headers)
- what if patch headers are a lie?
- it's the responsibility of the person adding the autoland whiteboard tag to check this
- :/ i'm not sure all the patch-viewing tools in bugzilla even show this metadata
- i'd prefer if bugzilla flagged patches where the attacher and commit-author are different people
- :/ i'm not sure all the patch-viewing tools in bugzilla even show this metadata
- it's the responsibility of the person adding the autoland whiteboard tag to check this
- [dveditz] previously, commits were protected by SSH keys. now, they are protected by bugzilla passwords. bugzilla users (who are not in the security group) are not going to be as careful with their bugzilla passwords (or email accounts).
- require a more secure password-reset procedure for users who have autoland enabled?
- audit trail?
- log (includes some Try-spam stuff)
- search pushlog by user (for each repo you care about)
- reviewers are checked. does this allow self-review / carryover reviews?
- allowing it means you only have to hack one bugzilla account (as long as the account is allowed to review patches)
- disallowing it makes the feature less convenient
}}
Action Items
| Action Item Status | In Progress | ||||||||||||||||||||||||||||||||||||||||
| Release Target | ` | ||||||||||||||||||||||||||||||||||||||||
| Action Items | |||||||||||||||||||||||||||||||||||||||||
4 Total; 0 Open (0%); 4 Resolved (100%); 0 Verified (0%); |
|||||||||||||||||||||||||||||||||||||||||
{{#set:|SecReview action item status=In Progress
|Feature version=`
|SecReview action items=
| Who | Action | By When | Completed date
[NEW] new [DONE] Done [MISSED] Miss |
| Apsec-Yvan | identify policies for password storage bug 740419 | Policy:https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines#Password_Storage (blocker for expanding beyond current user group) | [NEW] new |
| security assurance - Yvan to followup | secure password reset required for accounts checking into (non-Try) repos bug 740421 | Blocker (only for expanding beyond current user group) | [NEW] new |
| releng | audit trail - make sure we have persistent data of what autoland bot has done over time bug 740425 | [NEW] new | |
| security assurance - Yvan | code review Blocker! bug 740418 | EOM March 2012 | [NEW] new |
| ID | Summary | Priority | Status |
|---|---|---|---|
| 740418 | [[Security Review][Action Item]Automated Assisted Landing Code Review | -- | RESOLVED |
| 740419 | [[Security Review][Action Item]Automated Assisted Landing - Password Policies | -- | RESOLVED |
| 740421 | [[Security Review][Action Item]Automated Assisted Landing - Password Reset | -- | RESOLVED |
| 740425 | [Security Review][Action Item]Automated Assisted Landing - Audit Trail | -- | RESOLVED |
4 Total; 0 Open (0%); 4 Resolved (100%); 0 Verified (0%);
}} Yvan & Curtis met with Lukkas and John and came to an agreement where current release managers can use the feature only. To expand beyond this group we need to resolve the issues in the action items section.