Security/Reviews/AutomatedLanding

From MozillaWiki
Jump to: navigation, search
Please use "Edit with form" above to edit this page.

Item Reviewed

Automated/Assisted landing from Bugzilla to tip of $branch
Target
   
     Full Query    
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%);

The given value "
   
     Full Query    
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? :/
  • 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
  • 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
  • [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? :/
    • 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
    • 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
    • [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
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
Full Query
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%);

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


Full Query
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.