Security/Reviews/AutomatedLanding
From MozillaWiki
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%); |
||||||||
The given value "
| 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%);
" contains strip markers and therefore it cannot be parsed sufficiently.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
- Property "SecReview feature goal" (as page type) with input value "* 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)" contains invalid characters or is incomplete and therefore can cause unexpected results during a query or annotation process.
- Property "SecReview alt solutions" (as page type) with input value "* 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" contains invalid characters or is incomplete and therefore can cause unexpected results during a query or annotation process.
- Property "SecReview solution chosen" (as page type) with input value "* Security checking done on Bugzilla side (using hg group)
- Easy to check pushing permissions, since we are (easily) given the pushing users information" contains invalid characters or is incomplete and therefore can cause unexpected results during a query or annotation process.
- Property "SecReview threats considered" (as page type) with input value "* 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)" contains invalid characters or is incomplete and therefore can cause unexpected results during a query or annotation process.
- Property "SecReview threat brainstorming" (as page type) with input value "* 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" contains invalid characters or is incomplete and therefore can cause unexpected results during a query or annotation process.
Action Items
| Action Item Status | In Progress | ||||||||||||||||||||||||||||||||||||||||
| Release Target | ` | ||||||||||||||||||||||||||||||||||||||||
| Action Items | |||||||||||||||||||||||||||||||||||||||||
4 Total; 0 Open (0%); 4 Resolved (100%); 0 Verified (0%); |
|||||||||||||||||||||||||||||||||||||||||
The given value "
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%);
" contains strip markers and therefore it cannot be parsed sufficiently.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.
