project_distribution, project_module, project_theme, project_theme_engine should get a new field for security team coverage with values “Covered”, “Not covered”, “Unsupported due to security issue”

  • All current projects with at least one stable D7+ release will be moved to “Covered”
    • If there is not a stable release, we will email the maintainer when this is rolled out.
  • New projects will default to “Not covered”.
    • Only “Git vetted” users can mark their own project, not co-maintainers, “Covered”.
    • Once a project is marked “Covered”, it can only be changed by Security team members.
    • The field can be changed 10 days after the project has been created.
  • A message explaining this on the project page, along with an “not shielded” icon. See #2035277: Draft text for security advisory coverage messages.

    This project is not covered by security advisory policy.
    It may have publicly disclosed vulnerabilities. Use at your own risk!

  • Update status XML feeds need to include the new status (as a new element so they are backward compatible). #2766491: Update status should indicate security coverage
  • Update documentation at https://www.drupal.org/security-advisory-policy

Followups

#2825906: Encourage security coverage by emailing maintainers
#2825912: Rename “Git vetted user” role to “Opt into security advisory coverage”
#2825908: Move security team coverage from per-project to per-branch

Files: 
CommentFileSizeAuthor
#29 Screen Shot 2017-02-16 at 2.44.48 PM.png29.4 KBdrumm

Comments

drumm created an issue. See original summary.

drumm’s picture

drumm’s picture

Issue summary: View changes
drumm’s picture

Issue summary: View changes
mlhess’s picture

Issue tags: -project application revamp +project application revamp MWDS2016
drumm’s picture

Status: Active » Postponed (maintainer needs more info)
mlhess’s picture

Issue tags: -project application revamp MWDS2016 +project application revamp, +MWDS2016
drumm’s picture

Issue summary: View changes

Adding draft from #2035277: Draft text for security advisory coverage messages to the issue summary.

drumm’s picture

Issue summary: View changes
drumm’s picture

Issue summary: View changes
drumm’s picture

Issue summary: View changes

We agreed to a shorter new project waiting period at DrupalCon Dublin.

drumm’s picture

Issue summary: View changes
drumm’s picture

Status: Postponed (maintainer needs more info) » Active

I think this is enough information to start implementing this.

drumm’s picture

Issue summary: View changes

One more followup, #2825912: Rename “Git vetted user” role to “Opt into security advisory coverage”. Since that involves updating quite a bit of documentation, and is a place to signal changes to the project application process, I’d like to see it taken on with that project.

drumm’s picture

Assigned: drumm » Unassigned

I’m not starting the implementation here soon, just triaging issues.

drumm’s picture

Status: Active » Postponed
David_Rothstein’s picture

This project has not opted into coverage from the Security Team.
It may have publicly disclosed vulnerabilities. Use at your own risk!

Based on discussion in #2035277: Draft text for security advisory coverage messages the text for the first sentence should actually be more like "This project is not covered by the security advisory policy."

hestenet’s picture

Assigned: Unassigned » drumm
Status: Postponed » Active
Parent issue: » #2666584: [Community Initiative Proposal] Project Applications Process Revamp
drumm’s picture

Title: Add security team coverage field to projects » Add security advisory coverage field to projects
Issue summary: View changes

Updating to replace team with advisory. Thanks for the reminder David_Rothstein.

I plan to do an initial deployment later this week, just showing the field on project edit.

For the project page, we might want an alternative bit of copy for the "Unsupported due to security issue" state.

  • drumm committed c0d1b5c on 2787165-security-coverage
    Issue #2787165: Add security advisory coverage field to projects
    
drumm’s picture

Status: Active » Needs review

The commit above adds the field and logic around the editing form. It contains a few bits of copyediting seen by project maintainers and Security Team members that could use proofreading.

  • drumm committed 28822ed on 2787165-security-coverage
    Issue #2787165: Populate field_security_advisory_coverage
    
hestenet’s picture

I think I've pulled out all the relevant bits of language from those commits:

      'allowed_values' => [
        'not-covered' => 'Not covered',
        'covered' => 'Opt into <a href="/security-advisory-policy">security advisory coverage</a>',
        'revoked' => 'Unsupported due to security issue',
      ],

Suggestion: 'Unsupported due to unresolved security issue. What does this mean?' - linking to a page that explains that security coverage is revoked if maintainers fail to cooperate with the security team to resolve security issues in a timely fashion.

      'description' => '<em>Read & understand <a href="/security-advisory-policy">the policy</a>!</em> Once you opt-in your project, only the Security Team can change this.',

Suggestion: 'Read & understand the policy! Once you opt-in your project, you may not opt-out. Only the Security Team will be able to change this.'

function drupalorg_permission() {
  return [
    'opt into security advisory coverage' => [
      'title' => t('Opt into security advisory coverage'),
      'description' => t('Agree to the policy for projects.'),
    ],
  ];
}

Suggestion: Would it be more clear to say 'May opt into security advisory coverage for projects'? many people will have this role, but it almost implies that if you have the role your projects will all be opted in. (especially since that's kinda sorta how the old process worked).

    elseif ($wrapper->field_security_advisory_coverage->value() !== 'not-covered') {
      $form['field_security_advisory_coverage'][LANGUAGE_NONE]['#description'] = t('Only Security Team members may change coverage. Notice of discontinuing security advisory coverage should be given to users.');

Question: what do we mean by 'notice of discontinuing coverage should be given to users' - do we mean 'When changing this, the security team will require you as a maintainer to notify your users" ? Do we mean that a message will be posted to the update status? I think we should be more clear who's responsible for the communication, whether it is automated or manual, and where it will be made.

      if ($wrapper->field_project_type->value() !== 'full') {
        $form['field_security_advisory_coverage'][LANGUAGE_NONE]['#description'] = t('Only promoted full projects may opt into security advisory coverage.');

Suggestion: none - this looks good.

      elseif ($form_state['node']->uid !== $GLOBALS['user']->uid && !user_access('post to newsletter')) {
        $form['field_security_advisory_coverage'][LANGUAGE_NONE]['#description'] = t('Only the project owner may opt into security advisory coverage. <a href="!url">Learn about transferring ownership</a>', ['!url' => url('project/projectownership')]);

Suggestion: none - this looks good.

      elseif (!user_access('opt into security advisory coverage')) {
        $form['field_security_advisory_coverage'][LANGUAGE_NONE]['#description'] = t('You must be given access to opt into security advisory coverage. <a href="!url">Learn how to apply</a>', ['!url' => url('project/projectapplications')]);

Suggestion: Maaaybe: t('You must be given access to opt into security advisory coverage for your projects. Learn how to apply', ['!url' => url('project/projectapplications')]); -- but it may be fine as is.

      elseif ($form_state['node']->created > REQUEST_TIME - 10 * 24 * 60 * 60 && !user_access('post to newsletter')) {
        $form['field_security_advisory_coverage'][LANGUAGE_NONE]['#description'] = t('New projects must wait 10 days. Please take this time to review <a href="!url">writing secure code</a>.', ['!url' => url('writing-secure-code')]);

Suggestion: Might be nice to give a why? 'New projects must wait 10 days before opting into security advisory coverage, as this is often a period of flux for a new project's codebase. Please take this time to review writing secure code.', ['!url' => url('writing-secure-code')]);

Question: sounds like we mean literally new - but if they're newly promoted they can opt in on day 1?

Leaving on needs review because some security team input would be good.

drumm’s picture

Question: what do we mean by 'notice of discontinuing coverage should be given to users' - do we mean 'When changing this, the security team will require you as a maintainer to notify your users" ? Do we mean that a message will be posted to the update status? I think we should be more clear who's responsible for the communication, whether it is automated or manual, and where it will be made.

We haven’t done this process yet, but the general idea is that when a previously-covered project becomes unsupported, there will probably be a public security issue in the long run, if there isn’t one already. This isn’t quite an SA, but the Security Team does have a responsibility to inform users, potentially using all the communication channels like update status and/or an SA or PSA email.

Question: sounds like we mean literally new - but if they're newly promoted they can opt in on day 1?

Yes, that’s my understanding from the various meetings, and easy to implement. (We don’t have an easily-accessible project promotion time, other than scrubbing through the project’s revision history.)

The general reason why is that slowing people down hopefully gives them time to improve their project and a chance to consider more things they might not have spotted yet. We might want to link to https://www.drupal.org/node/7765 too.

hestenet’s picture

We haven’t done this process yet, but the general idea is that when a previously-covered project becomes unsupported, there will probably be a public security issue in the long run, if there isn’t one already. This isn’t quite an SA, but the Security Team does have a responsibility to inform users, potentially using all the communication channels like update status and/or an SA or PSA email.

Ah - that makes sense - in that case I'd suggest the language be more specific if we can be.

Suggestion: "Only Security Team members may change coverage. A security advisory should be posted to notify users that the project is no longer receiving security advisory coverage."

Re: 10 day wait:

Yes, that’s my understanding from the various meetings, and easy to implement. (We don’t have an easily-accessible project promotion time, other than scrubbing through the project’s revision history.)

Cool - that sounds good to me then - just wanted to clarify

  • drumm committed ffc18d7 on 2787165-security-coverage
    Issue #2787165 by hestenet: Copyediting
    
drumm’s picture

Committed most of hestenet’s suggestions. I did err on the side of vagueness, like “A security advisory should may be posted” since the policy isn’t set at https://www.drupal.org/security-advisory-policy yet.

drumm’s picture

  • drumm committed 28822ed on 7.x-3.x, dev
    Issue #2787165: Populate field_security_advisory_coverage
    
  • drumm committed c0d1b5c on 7.x-3.x, dev
    Issue #2787165: Add security advisory coverage field to projects
    
  • drumm committed ffc18d7 on 7.x-3.x, dev
    Issue #2787165 by hestenet: Copyediting
    

  • drumm committed 5716701 on 7.x-3.x
    Issue #2787165: Export the new permission
    
  • drumm committed fc9f5d5 on 7.x-3.x
    Issue #2787165: Separate class for graying an icon
    
drumm’s picture

The first phase has been deployed, the field on editing project. The display on public project pages will come next.

Now is a good time to update the value for special projects like https://www.drupal.org/project/bad_judgement and friends. And update the policy at https://www.drupal.org/security-advisory-policy.

  • drumm committed 1048aa8 on 2787165-security-coverage
    Issue #2787165: Public project page changes
    
  • drumm committed 5716701 on 2787165-security-coverage
    Issue #2787165: Export the new permission
    
  • drumm committed fc9f5d5 on 2787165-security-coverage
    Issue #2787165: Separate class for graying an icon
    

  • drumm committed 1048aa8 on 7.x-3.x, dev
    Issue #2787165: Public project page changes
    
drumm’s picture

This has been deployed: https://www.drupal.org/project/bad_judgement

There is one last part - putting something in update status XML

drumm’s picture

Status: Needs review » Fixed

The XML is not a small followup, so it gets a fresh issue: #2853696: Add security advisory coverage to update status XML

  • drumm committed 5df1bc8 on 7.x-3.x
    Issue #2787165: Export security advisory coverage field
    
shrop’s picture

drumm mentioned updating docs at https://www.drupal.org/security-advisory-policy. I think part of the update should include some info and/or link to a page discussing the opt-in process. A project maintainer may not realize what the message really means and what options are available. Since the link from the new feature goes to https://www.drupal.org/security-advisory-policy, that could be the place to mention it.

Maybe the info about the opt-in process is out there already and we just need to add a link to https://www.drupal.org/security-advisory-policy?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.