-
Notifications
You must be signed in to change notification settings - Fork 885
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] Modify SECURITY.md nested dependency fix and add backport process #3497
[Doc] Modify SECURITY.md nested dependency fix and add backport process #3497
Conversation
d38232b
to
b1e979b
Compare
If you discover a potential security issue in this project we ask that you notify AWS/Amazon Security via our [vulnerability reporting page](http://aws.amazon.com/security/vulnerability-reporting/) or directly via email to aws-security@amazon.com. Please do **not** create a public GitHub issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you discover a potential security issue in this project we ask that you notify AWS/Amazon Security via our [vulnerability reporting page](http://aws.amazon.com/security/vulnerability-reporting/) or directly via email to aws-security@amazon.com. Please do **not** create a public GitHub issue. | |
If you discover a potential security issue in this project, we ask that you notify AWS/Amazon Security via the [vulnerability reporting page](http://aws.amazon.com/security/vulnerability-reporting/) or directly, via email, to aws-security@amazon.com. Please do **not** create a public GitHub issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this paragraph is using a standard template for all repos. For example, here is the SECURITY.md from OpenSearch: https://github.com/opensearch-project/OpenSearch/blob/main/SECURITY.md. Let's just use it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure where they got that but the grammar errors are bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the missing comma after the first subordinate clause, the other suggestions are less black and white... and I wouldn't classify any of them as "bad grammar", but I'm not a native speaker so what do I know :)
@AMoo-Miki: If you feel strongly about the way the paragraph is stated, I'd recommend opening up a PR against the one @ananzh links above, so that we can all benefit from this feedback.
SECURITY.md
Outdated
Please be aware of that fixing nested dependency can be tricky. Sometimes, bump the dependent package will auto bump all the nested dependencies which will solve multiple security vulnerabilities and provide a more maintainable code base. | ||
|
||
- To backport the CVEs fix to previous versions, add backport label (ex: `backport 1.x`) to trigger auto backport. If auto backport fail, pls use the following steps to manually backport: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- To backport the CVEs fix to previous versions, add backport label (ex: `backport 1.x`) to trigger auto backport. If auto backport fail, pls use the following steps to manually backport: | |
- To backport a pull request for a CVE fix to previous versions of OpenSearch Dashboards, add the desired backport labels (ex: `backport 1.x`) to the PR. When the PR is merged, a workflow will attempt to backport the PR. If this backporting attempt fail, the PR will be updated with comments about the failure; follow the instructions in those comments to manually backport your changes which you can then use in a new PR. |
SECURITY.md
Outdated
``` | ||
|
||
It's worth noting that backporting a pull request can be a complex process, and there may require additional steps depending on the changes involved and target branch. It's important to carefully review and test the changes to ensure they are applied correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that backporting a pull request can be a complex process, and there may require additional steps depending on the changes involved and target branch. It's important to carefully review and test the changes to ensure they are applied correctly. | |
It's worth noting that backporting a pull request can be a complex process and depending on the changes involved, additional steps might be required to resolve conflicts. It's important to carefully review and test the changes to ensure they are compatible with the version of OpenSearch Dashboards in the target branch and that the changes are applied correctly. |
Do we need to get this reviewed as an org perspective? I know other SECURITY.md is identical and was provided as a template. |
I am curious when we initially modify this file here ... did we get this reviewed as an org perspective? Seems in the previous comments there is no reviewed? |
@kavilla I have discussed this with team and the point brought by @AMoo-Miki is that this is not related to repo security standard, this doc is more like a reference for any contributors who want to contribute. So I think the question is now that is this the right place to add these instruction references? I know that we started to modify the file in this PR #2674. But is SECURITY.md the correct place to add these steps? |
@kavilla I think we could move on to approve the file. I already add @davidlago in the reviewer. Since we own the repo, we should be able to modify this file. If there is any organization concerns, we could restore the standard file later. |
b1e979b
to
e2912a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ananzh, this is a definite improvement. I have a few minor suggestions about how we could make it better, but most of them could be added in a follow-up PR. The one change I would like to see before approving is a running yarn osd bootstrap
as part of the process of doing a manual dependency backport.
SECURITY.md
Outdated
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will remove
If you discover a potential security issue in this project we ask that you notify AWS/Amazon Security via our [vulnerability reporting page](http://aws.amazon.com/security/vulnerability-reporting/) or directly via email to aws-security@amazon.com. Please do **not** create a public GitHub issue. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a header to separate Reporting from Fixing
## Fixing a Vulnerability | |
SECURITY.md
Outdated
|
||
|
||
- For direct dependencies - After identifying a version of the package that is both compatible with OpenSearch Dashboards and includes a fix for the vulnerability, update the dependency in `package.json` and run `yarn osd bootstrap` to build the project and update the `yarn.lock` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can clarify what counts as a direct dependency:
- For direct dependencies - After identifying a version of the package that is both compatible with OpenSearch Dashboards and includes a fix for the vulnerability, update the dependency in `package.json` and run `yarn osd bootstrap` to build the project and update the `yarn.lock` file. | |
- For direct dependencies (listed explicitly in `package.json`) - After identifying a version of the package that is both compatible with OpenSearch Dashboards and includes a fix for the vulnerability, update the dependency in `package.json` and run `yarn osd bootstrap` to build the project and update the `yarn.lock` file. |
that is both compatible with OpenSearch Dashboards
Do we want to clarify more guidance on how to pick a version? For changes to main
, major version upgrades are welcome, but may require migrations of deprecated methods and APIs. Otherwise, you can look for the latest version that works out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me think about it. Maybe not in this PR. Because it could be complex, major version upgrades could also be not in main.
- For nested dependencies (sub-dependencies) | ||
|
||
- If the package range is suitable but `yarn.lock` does not include the desired version, edit the `yarn.lock` file and remove the lines corresponding to the package. Then, run `yarn osd bootstrap` to have the latest version of the package, that satisfies the range from `package.json`, added to `yarn.lock`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the package range is suitable
Add instructions on how to determine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add instructions.
SECURITY.md
Outdated
Please be aware of that fixing nested dependencies can be tricky. Sometimes, bumping a parent package of the nested dependency can upgrade several of the nested dependencies at once to solve multiple security vulnerabilities and provide a more maintainable code base. | ||
|
||
- To backport a pull request for a CVE fix to previous versions of OpenSearch Dashboards, add the desired backport labels (ex: `backport 1.x`) to the PR. When the PR is merged, a workflow will attempt to backport the PR. If this backporting attempt fail, the PR will be updated with comments about the failure; follow the instructions in those comments to manually backport your changes which you can then use in a new PR. Here are some steps for reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- To backport a pull request for a CVE fix to previous versions of OpenSearch Dashboards, add the desired backport labels (ex: `backport 1.x`) to the PR. When the PR is merged, a workflow will attempt to backport the PR. If this backporting attempt fail, the PR will be updated with comments about the failure; follow the instructions in those comments to manually backport your changes which you can then use in a new PR. Here are some steps for reference. | |
- To backport a pull request for a CVE fix to previous versions of OpenSearch Dashboards, add the desired backport labels (ex: `backport 1.x`) to the PR. When the PR is merged, a workflow will attempt to backport the PR. If this backporting attempt fails, the PR will be updated with a comment about the failure. Follow the instructions provided to manually backport your changes in a new PR. Here are some steps for reference: |
We also probably need some links to our maintainer/backporting processes more generally. And to note that some CVEs will require manual resolution on each branch - maybe a major version bump on main
, a smaller version bump on 2.x
, and a resolution on 1.x
, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix
SECURITY.md
Outdated
- To backport a pull request for a CVE fix to previous versions of OpenSearch Dashboards, add the desired backport labels (ex: `backport 1.x`) to the PR. When the PR is merged, a workflow will attempt to backport the PR. If this backporting attempt fail, the PR will be updated with comments about the failure; follow the instructions in those comments to manually backport your changes which you can then use in a new PR. Here are some steps for reference. | ||
|
||
- Identify the pull request you want to backport and the target backport version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - for steps in a sequence, ordered/numbered list is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will change
SECURITY.md
Outdated
- Test the changes thoroughly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing an explicit step to run yarn osd bootstrap
. Because yarn.lock
is intended to always be a generated file, this helps catch bad merges or conflict resolutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
Issue Resolved: opensearch-project#3494 Signed-off-by: Anan Zhuang <ananzh@amazon.com>
e2912a4
to
791c856
Compare
add header line break
…ss (#3497) * [Doc] Modify SECURITY.md nested dependency fix and add backport process Issue Resolved: #3494 Signed-off-by: Anan Zhuang <ananzh@amazon.com> * Update SECURITY.md add header line break --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 457b5df) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
…ss (#3497) (#3672) * [Doc] Modify SECURITY.md nested dependency fix and add backport process Issue Resolved: #3494 Signed-off-by: Anan Zhuang <ananzh@amazon.com> * Update SECURITY.md add header line break --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit 457b5df) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ss (opensearch-project#3497) * [Doc] Modify SECURITY.md nested dependency fix and add backport process Issue Resolved: opensearch-project#3494 Signed-off-by: Anan Zhuang <ananzh@amazon.com> * Update SECURITY.md add header line break --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com>
…ss (opensearch-project#3497) * [Doc] Modify SECURITY.md nested dependency fix and add backport process Issue Resolved: opensearch-project#3494 Signed-off-by: Anan Zhuang <ananzh@amazon.com> * Update SECURITY.md add header line break --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: Josh Romero <rmerqg@amazon.com> Signed-off-by: David Sinclair <david@sinclair.tech>
Issues Resolved
#3494
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr