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.

Sign up for GitHub

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

Jump to bottom

fix: limit the plugin output size #484

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: limit the plugin output size #484

wants to merge 2 commits into from
+129 −3

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Nov 29, 2024 •
edited
Loading

Fix:

  • set the plugin output limit for STDOUT and STDERR to be 10MiB

Test:

  • when the plugin output size exceeds 10MiB, the output pipe breaks, and the plugin process outputs an error to STDERR

Spec changes: notaryproject/specifications#320
Resolves #187

@codecov Codecov
Copy link

codecov bot commented Nov 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.47%. Comparing base (57c5e0d) to head (8d6e3cc).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
+ Coverage   80.39%   80.47%   +0.08%     
==========================================
  Files          34       35       +1     
  Lines        3330     3344      +14     
==========================================
+ Hits         2677     2691      +14     
  Misses        508      508              
  Partials      145      145

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shizhMSFT
shizhMSFT requested changes Nov 29, 2024
View reviewed changes
internal/io/limitwriter.go Outdated Show resolved Hide resolved
internal/io/limitwriter.go Outdated Show resolved Hide resolved
Two-Hearts
Two-Hearts requested changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I actually have a couple of questions regarding this PR and the issue #187 itself:

  1. We are essentially limiting the behavior of a plugin due to a certain extent of untrust. Do we need to reflect this in the Notary project spec? (such as, where does the 10MiB comes from?)
  2. What security concern do we have if the plugin output size is larger than 10 MiB? According to Notation threat model: https://github.com/notaryproject/specifications/blob/main/threatmodels/notation-threatmodel.md, plugins are inside the trust boundary. If we do not trust the plugin's output, how do we interpret the trust boundary then?

@JeyJeyGao
Copy link
Contributor Author

I actually have a couple of questions regarding this PR and the issue #187 itself:

  1. We are essentially limiting the behavior of a plugin due to a certain extent of untrust. Do we need to reflect this in the Notary project spec? (such as, where does the 10MiB comes from?)
  2. What security concern do we have if the plugin output size is larger than 10 MiB? According to Notation threat model: https://github.com/notaryproject/specifications/blob/main/threatmodels/notation-threatmodel.md, plugins are inside the trust boundary. If we do not trust the plugin's output, how do we interpret the trust boundary then?
  1. The limit has been updated to 64 MiB, which is almost impossible to be reached by a normal plugin. The limitation depends on the implementation; different implementations may have different concerns that lead to different limitation decisions. The limitation is more like a notation-specific setting rather than a specification requirement. If we need to update the specification, we can mention that the plugin output may have size limitations depending on the implementation.
  2. We don’t have security concerns regarding this issue. The goal is to improve system reliability when a plugin has a bug or generates unexpected output. Notation-go can generate proper logs without killing the process directly, ensuring proper handling.

@Two-Hearts
Copy link
Contributor

Two-Hearts commented Dec 2, 2024 •
edited
Loading

  1. The limit has been updated to 64 MiB, which is almost impossible to be reached by a normal plugin. The limitation depends on the implementation; different implementations may have different concerns that lead to different limitation decisions. The limitation is more like a notation-specific setting rather than a specification requirement. If we need to update the specification, we can mention that the plugin output may have size limitations depending on the implementation.

Note: any extra restriction on the plugin without reflecting in the spec may lead to potential breaking changes for Notation. It's not about the number, whether it's 10 or 64. It's about best practice. The reason is simple, plugin builders follow the spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md to build their plugins. Any extra restriction that's not in that spec can be a hidden rule and unexpected.

  1. We don’t have security concerns regarding this issue. The goal is to improve system reliability when a plugin has a bug or generates unexpected output. Notation-go can generate proper logs without killing the process directly, ensuring proper handling.

If we indeed do not have any security concern regarding this issue, then we should leave the control to plugin authors and users. If a plugin has a bug, its usage should be dropped until the bug is fixed right? I don't think this is in Notation's scope, just like we do not censor the output content of a plugin.

@JeyJeyGao JeyJeyGao mentioned this pull request Dec 2, 2024
Open
@JeyJeyGao
Copy link
Contributor Author

  1. The limit has been updated to 64 MiB, which is almost impossible to be reached by a normal plugin. The limitation depends on the implementation; different implementations may have different concerns that lead to different limitation decisions. The limitation is more like a notation-specific setting rather than a specification requirement. If we need to update the specification, we can mention that the plugin output may have size limitations depending on the implementation.

Note: any extra restriction on the plugin without reflecting in the spec may lead to potential breaking changes for Notation. It's not about the number, whether it's 10 or 64. It's about best practice. The reason is simple, plugin builders follow the spec: https://github.com/notaryproject/specifications/blob/main/specs/plugin-extensibility.md to build their plugins. Any extra restriction that's not in that spec can be a hidden rule and unexpected.

  1. We don’t have security concerns regarding this issue. The goal is to improve system reliability when a plugin has a bug or generates unexpected output. Notation-go can generate proper logs without killing the process directly, ensuring proper handling.

If we indeed do not have any security concern regarding this issue, then we should leave the control to plugin authors and users. If a plugin has a bug, its usage should be dropped until the bug is fixed right? I don't think this is in Notation's scope, just like we do not censor the output content of a plugin.

As per our online discussion, I created a corresponding Spec PR: notaryproject/specifications#320

@JeyJeyGao JeyJeyGao requested a review from Two-Hearts December 2, 2024 07:27
@JeyJeyGao JeyJeyGao requested a review from Two-Hearts December 3, 2024 01:18
shizhMSFT
shizhMSFT previously approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM with suggestions

internal/io/limitedwriter.go Show resolved Hide resolved
Two-Hearts
Two-Hearts previously approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao JeyJeyGao dismissed stale reviews from Two-Hearts and shizhMSFT via fffc7f0 December 3, 2024 06:19
shizhMSFT
shizhMSFT approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Two-Hearts
Two-Hearts approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

@Two-Hearts
Copy link
Contributor

@JeyJeyGao Please sign all the commits.

@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_output_size branch 2 times, most recently from 8c94583 to ecd22a2 Compare December 9, 2024 02:08
@JeyJeyGao
Squashed commit of the following:
7a78f52 
commit ecd22a2
Author: Junjie Gao <[email protected]>
Date:   Tue Dec 3 06:19:18 2024 +0000

    fix: update comment

    Signed-off-by: Junjie Gao <[email protected]>

commit 32ae375
Author: Junjie Gao <[email protected]>
Date:   Mon Dec 2 09:10:37 2024 +0000

    fix: update

    Signed-off-by: Junjie Gao <[email protected]>

commit 0076d0f
Author: Junjie Gao <[email protected]>
Date:   Mon Dec 2 03:13:08 2024 +0000

    fix: update LimitedWriter

    Signed-off-by: Junjie Gao <[email protected]>

commit 067d4f6
Author: Junjie Gao <[email protected]>
Date:   Mon Dec 2 02:32:17 2024 +0000

    fix(test): update

    Signed-off-by: Junjie Gao <[email protected]>

commit 3099d35
Author: Junjie Gao <[email protected]>
Date:   Mon Dec 2 10:03:54 2024 +0800

    perf(log): encode objects only when logged (notaryproject#481)

    Fix:
    - replaced `.String()` with the `%v` format to avoid rendering the
    string before actually logging it.

    Resolves notaryproject#480

    Signed-off-by: Junjie Gao <[email protected]>

commit 8bc331b
Author: Patrick Zheng <[email protected]>
Date:   Mon Dec 2 08:30:56 2024 +0800

    fix: enable timestamping cert chain revocation check during signing (notaryproject#482)

    Signed-off-by: Patrick Zheng <[email protected]>
    Signed-off-by: Junjie Gao <[email protected]>

commit 161a736
Author: Junjie Gao <[email protected]>
Date:   Mon Dec 2 02:13:35 2024 +0000

    fix: resolve comments for Shiwei

    Signed-off-by: Junjie Gao <[email protected]>

commit 665e111
Author: Junjie Gao <[email protected]>
Date:   Fri Nov 29 08:07:16 2024 +0000

    fix: update code style

    Signed-off-by: Junjie Gao <[email protected]>

commit 5d6c89e
Author: Junjie Gao <[email protected]>
Date:   Fri Nov 29 08:06:15 2024 +0000

    fix: update comments

    Signed-off-by: Junjie Gao <[email protected]>

commit 3bc343d
Author: Junjie Gao <[email protected]>
Date:   Fri Nov 29 07:42:38 2024 +0000

    fix: update comment

    Signed-off-by: Junjie Gao <[email protected]>

commit 69303b5
Author: Junjie Gao <[email protected]>
Date:   Fri Nov 29 07:40:15 2024 +0000

    fix: limit the plugin output size

    Signed-off-by: Junjie Gao <[email protected]>

Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao force-pushed the fix/plugin_output_size branch from ecd22a2 to 7a78f52 Compare December 9, 2024 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Two-Hearts Two-Hearts Two-Hearts approved these changes

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@gokarnm gokarnm Awaiting requested review from gokarnm gokarnm is a code owner

@NiazFK NiazFK Awaiting requested review from NiazFK NiazFK is a code owner

@priteshbandi priteshbandi Awaiting requested review from priteshbandi priteshbandi is a code owner

@rgnote rgnote Awaiting requested review from rgnote rgnote is a code owner

@toddysm toddysm Awaiting requested review from toddysm toddysm is a code owner

@vaninrao10 vaninrao10 Awaiting requested review from vaninrao10 vaninrao10 is a code owner

@yizha1 yizha1 Awaiting requested review from yizha1 yizha1 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

plugin output size limitation
3 participants
@JeyJeyGao @Two-Hearts @shizhMSFT