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

DOC: Add security policy document #7199

Merged
merged 4 commits into from Sep 11, 2023
Merged

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Aug 29, 2023

This pull request follows up #7197 and is expected to address the Security Policy warning1 reported on our scorecard (as of 9ceb5d2).

Language was adapted from https://github.com/microsoft/vscode/blob/main/SECURITY.md

Warning

Reporting Procedure

In this pull request, it assumes we create a private discourse category called Security associated with the email slicer+security@discoursemail.com

Alternatively, as illustrated below, we could also enable the built-in private vulnerability reporting2 feature available in GitHub.

image image

References

Footnotes

  1. https://github.com/ossf/scorecard/blob/v4.12.0/docs/checks.md#security-policy

  2. https://docs.github.com/en/code-security/security-advisories/guidance-on-reporting-and-writing/privately-reporting-a-security-vulnerability

@jcfr jcfr added the Type: Documentation Issues regarding documentation label Aug 29, 2023
@jcfr jcfr added this to the Slicer 5.5 milestone Aug 29, 2023
Copy link
Contributor

@jamesobutler jamesobutler left a comment

Choose a reason for hiding this comment

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

This adapted language makes sense. I was viewing what GitHub included in their template for a SECURITY.md at https://github.com/Slicer/Slicer/security/policy and it included a section that states what versions get security updates. This would probably be good to mention about the Slicer 5.2.x series no longer getting security updates now that Slicer 5.4.x is out.

Supported Versions

Use this section to tell people about which versions of your project are
currently being supported with security updates.

Version Supported
5.1.x
5.0.x
4.0.x
< 4.0

SECURITY.md Show resolved Hide resolved
Co-authored-by: James Butler <james.butler@revvity.com>
Co-authored-by: Steve Pieper <pieper@isomics.com>
@jcfr
Copy link
Member Author

jcfr commented Aug 29, 2023

And using the latest support for admonition1 allows to nicely highlight the note.

Before After
image image

Footnotes

  1. https://github.com/orgs/community/discussions/16925

@jcfr
Copy link
Member Author

jcfr commented Aug 29, 2023

@pieper @jamesobutler @sjh26 @pieper Could you review and approve ?

@jcfr jcfr requested a review from jamesobutler August 29, 2023 15:04
pieper
pieper previously approved these changes Aug 29, 2023
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jamesobutler any other suggestions?

@jamesobutler
Copy link
Contributor

jamesobutler commented Aug 29, 2023

Is there any documentation any where that mentions that latest Slicer stable (Slicer 5.4.0) means that no more security releases will be created for the Slicer 5.2.x? For example if a security patch was added and a Slicer 5.4.1 was created, would a Slicer 5.2.3 also be created?

The table mentions which versions get security fixes, but makes no mention about which versions do not get security fixes.

@pieper
Copy link
Member

pieper commented Aug 29, 2023

For example if a security patch was added and a Slicer 5.4.1 was created, would a Slicer 5.2.3 also be created?

No, I wouldn't expect that to happen.

I'll add some text to clarify.

@jcfr
Copy link
Member Author

jcfr commented Aug 29, 2023

I'll add some text to clarify.

🙏

Reporting Security Issue

In the meantime, I also created the "Security" category:

along with a private group called security (aka the response team)

Before integrating, I will also circle back with Kitware security team.

@@ -0,0 +1,31 @@
# Security

If you believe you have found a security vulnerability in Slicer, please report it to us as described below.
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we indicate the scope as well ?

Suggested change
If you believe you have found a security vulnerability in Slicer, please report it to us as described below.
If you believe you have found a security vulnerability in the Slicer application or websites, please report it to us as described below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a good idea. Are security vulnerabilities in Slicer extensions included in the scope? Or should users report security issues of extensions through different means?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I think the correct answer is that we have limited influence on the extension developers and claim no responsibility for their actions. We might, as a best effort, pass on any reports but we can't claim to be doing anything rigorous (in fact, it might be that we don't do anything, e.g. if nobody notices the report or someone thinks someone else will handle it).

Copy link
Contributor

Choose a reason for hiding this comment

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

We could only attempt to make commitments for selected extensions - that we develop ourselves, or we know the developers of, or at least the developers are known to be responsive. For this, we would need to do what we have been planning for years: specify "support level" or "stability" for each extension (at least 2 but maybe 3 levels).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be making any "commitments" in a sense that people would interpret as legally binding, but we can say we intend to make an effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

A more direct example would be Slicer extensions under the "Slicer" GitHub organization.

These 2 Slicer extensions are under the "Slicer" GitHub organization and are built-in to Slicer currently. Security reports related to this code falls under the scope of this SECURITY.md document where reports should go to the discourse email?
https://github.com/Slicer/LandmarkRegistration
https://github.com/Slicer/SlicerSurfaceToolbox

These are additional Slicer extensions under the "Slicer" GitHub organization, but are not built-in to Slicer and are installed as extensions through the extensions manger. Do these also fall under the scope of this SECURITY.md?
https://github.com/Slicer/SlicerNeuro
https://github.com/Slicer/SlicerJupyter

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not expected SECURITY.md document in Slicer/Slicer would propagate to Slicer/other repositories, but it does not hurt to clarify this. Later we can add SECURITY.md documents to other repositories.

Copy link
Contributor

Choose a reason for hiding this comment

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

You wouldn't expect SECURITY.md to apply to https://github.com/Slicer/LandmarkRegistration and https://github.com/Slicer/SlicerSurfaceToolbox even though it is included in the binaries at https://download.slicer.org/?

Copy link
Member Author

@jcfr jcfr Aug 29, 2023

Choose a reason for hiding this comment

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

I would not expected SECURITY.md document in Slicer/Slicer would propagate to Slicer/other repositories

This could be addressed by creating a repository called .github where we would include the file SECURITY.md

See https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/creating-a-default-community-health-file

Since the Slicer project is our main code base, the default SECURITY.md could simply include a link to the one of Slicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, this could easily be done in follow-up step.

@pieper
Copy link
Member

pieper commented Aug 29, 2023

Yes, I would expect the policy to apply to those. I don't think we need to restrict what can be reported under this policy. Anything people reasonably think might be related to Slicer's security can be reported and if it's feasible to address we'll try. Again, we don't want to make any implication that we are taking responsibility for anything.

@jcfr
Copy link
Member Author

jcfr commented Aug 30, 2023

By adding a "Scope" section, I believe I addressed all concern.

Could you have another look and approve ?

@jcfr jcfr requested a review from pieper August 30, 2023 00:14
Co-authored-by: Andras Lasso <lasso@queensu.ca>
Co-authored-by: James Butler <james.butler@revvity.com>
Co-authored-by: Steve Pieper <pieper@isomics.com>
@jamesobutler
Copy link
Contributor

Additionally here is a link to OSSF’s document about setting up a security reporting process that was linked in the remediation area of https://github.com/Slicer/Slicer/security/code-scanning/12.

https://github.com/ossf/oss-vulnerability-guide/blob/main/maintainer-guide.md

@jamesobutler
Copy link
Contributor

Before integrating, I will also circle back with Kitware security team.

@jcfr does this previous statement still apply or is this ready to integrate?

@jcfr
Copy link
Member Author

jcfr commented Sep 5, 2023

We have been discussing this and just sent a reminder so that we conclude.

Out traveling this week, I will be follow up when back next week.

@jcfr jcfr merged commit 0e332ec into Slicer:main Sep 11, 2023
5 checks passed
@jcfr jcfr deleted the add-security-policy branch September 11, 2023 13:11
jcfr referenced this pull request Sep 27, 2023
Qt lupdate threw warnings about unconsumed metadata for lines that had translator's comments (that is exported to the language translation file to provide additonal context for translators) in the same line as the translatable text. For example:

    this->SupportedReadFileTypes->InsertNextValue(vtkMRMLTr("vtkMRMLColorTableStorageNode", "MRML Color Table") + " (.ctbl)");  //: file format name

The issue with this is that Qt expects translator comment to be in the previous line like this:

    //: File format name
    this->SupportedReadFileTypes->InsertNextValue(vtkMRMLTr("vtkMRMLColorTableStorageNode", "MRML Color Table") + " (.ctbl)");
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Issues regarding documentation
Development

Successfully merging this pull request may close these issues.

None yet

4 participants