fix: limit the plugin output size #484

JeyJeyGao · 2024-11-29T07:42:03Z

Fix:

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

Test:

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

Spec changes: notaryproject/specifications#320
Resolves #187

codecov · 2024-11-29T07:44:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.47%. Comparing base (d1d64e7) to head (cf1ef41).
Report is 1 commits behind head on main.

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.

internal/io/limitwriter.go

Two-Hearts

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

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?)
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 · 2024-12-02T03:35:47Z

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

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?)

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?

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.
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 · 2024-12-02T03:52:30Z

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.

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 · 2024-12-02T05:41:00Z

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.

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

internal/io/limitedwriter.go

plugin/plugin.go

shizhMSFT

LGTM with suggestions

internal/io/limitedwriter.go

Two-Hearts

LGTM

shizhMSFT

LGTM

Two-Hearts

LGTM

Two-Hearts · 2024-12-09T01:57:37Z

@JeyJeyGao Please sign all the commits.

commit ecd22a2 Author: Junjie Gao <junjiegao@microsoft.com> Date: Tue Dec 3 06:19:18 2024 +0000 fix: update comment Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 32ae375 Author: Junjie Gao <junjiegao@microsoft.com> Date: Mon Dec 2 09:10:37 2024 +0000 fix: update Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 0076d0f Author: Junjie Gao <junjiegao@microsoft.com> Date: Mon Dec 2 03:13:08 2024 +0000 fix: update LimitedWriter Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 067d4f6 Author: Junjie Gao <junjiegao@microsoft.com> Date: Mon Dec 2 02:32:17 2024 +0000 fix(test): update Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 3099d35 Author: Junjie Gao <junjiegao@microsoft.com> 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 <junjiegao@microsoft.com> commit 8bc331b Author: Patrick Zheng <patrickzheng@microsoft.com> 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 <patrickzheng@microsoft.com> Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 161a736 Author: Junjie Gao <junjiegao@microsoft.com> Date: Mon Dec 2 02:13:35 2024 +0000 fix: resolve comments for Shiwei Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 665e111 Author: Junjie Gao <junjiegao@microsoft.com> Date: Fri Nov 29 08:07:16 2024 +0000 fix: update code style Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 5d6c89e Author: Junjie Gao <junjiegao@microsoft.com> Date: Fri Nov 29 08:06:15 2024 +0000 fix: update comments Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 3bc343d Author: Junjie Gao <junjiegao@microsoft.com> Date: Fri Nov 29 07:42:38 2024 +0000 fix: update comment Signed-off-by: Junjie Gao <junjiegao@microsoft.com> commit 69303b5 Author: Junjie Gao <junjiegao@microsoft.com> Date: Fri Nov 29 07:40:15 2024 +0000 fix: limit the plugin output size Signed-off-by: Junjie Gao <junjiegao@microsoft.com> Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

priteshbandi

LGTM

Fix: - set the plugin output limit for STDOUT and STDERR to be 10MiB Test: - when the plugin output size exceeds 64MiB, the output pipe breaks, and the plugin process outputs an error to STDERR Spec changes: notaryproject/specifications#320 Resolves notaryproject#187 Signed-off-by: Junjie Gao <junjiegao@microsoft.com> Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>

JeyJeyGao requested review from gokarnm, NiazFK, priteshbandi, rgnote, shizhMSFT, toddysm, Two-Hearts, vaninrao10 and yizha1 as code owners November 29, 2024 07:42

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 requested changes Dec 2, 2024

View reviewed changes

JeyJeyGao mentioned this pull request Dec 2, 2024

fix(doc): add plugin output size limitation notaryproject/specifications#320

Merged

JeyJeyGao requested review from shizhMSFT and Two-Hearts December 2, 2024 05:43

Two-Hearts reviewed Dec 2, 2024

View reviewed changes

internal/io/limitedwriter.go Show resolved Hide resolved

JeyJeyGao requested a review from Two-Hearts December 2, 2024 07:27

Two-Hearts reviewed Dec 2, 2024

View reviewed changes

internal/io/limitedwriter.go Outdated Show resolved Hide resolved

Two-Hearts reviewed Dec 2, 2024

View reviewed changes

plugin/plugin.go Show resolved Hide resolved

JeyJeyGao requested a review from Two-Hearts December 3, 2024 01:18

shizhMSFT previously approved these changes Dec 3, 2024

View reviewed changes

internal/io/limitedwriter.go Show resolved Hide resolved

Two-Hearts previously approved these changes Dec 3, 2024

View reviewed changes

JeyJeyGao dismissed stale reviews from Two-Hearts and shizhMSFT via fffc7f0 December 3, 2024 06:19

JeyJeyGao requested review from shizhMSFT and Two-Hearts December 3, 2024 06:20

shizhMSFT approved these changes Dec 3, 2024

View reviewed changes

Two-Hearts approved these changes Dec 9, 2024

View reviewed changes

JeyJeyGao force-pushed the fix/plugin_output_size branch 2 times, most recently from 8c94583 to ecd22a2 Compare December 9, 2024 02:08

JeyJeyGao force-pushed the fix/plugin_output_size branch from ecd22a2 to 7a78f52 Compare December 9, 2024 02:10

Merge branch 'main' into fix/plugin_output_size

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

8d6e3cc

priteshbandi approved these changes Dec 12, 2024

View reviewed changes

Merge branch 'main' into fix/plugin_output_size

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

cf1ef41

JeyJeyGao merged commit cefd007 into notaryproject:main Dec 13, 2024
9 checks passed

Two-Hearts mentioned this pull request Dec 13, 2024

bump: release v1.3.0-rc.2 #495

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: limit the plugin output size #484

fix: limit the plugin output size #484

JeyJeyGao commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

Two-Hearts left a comment •

edited

Loading

JeyJeyGao commented Dec 2, 2024

Two-Hearts commented Dec 2, 2024 •

edited

Loading

JeyJeyGao commented Dec 2, 2024

shizhMSFT left a comment

Two-Hearts left a comment

shizhMSFT left a comment

Two-Hearts left a comment

Two-Hearts commented Dec 9, 2024

priteshbandi left a comment

fix: limit the plugin output size #484

fix: limit the plugin output size #484

Conversation

JeyJeyGao commented Nov 29, 2024 • edited Loading

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Two-Hearts left a comment • edited Loading

Choose a reason for hiding this comment

JeyJeyGao commented Dec 2, 2024

Two-Hearts commented Dec 2, 2024 • edited Loading

JeyJeyGao commented Dec 2, 2024

shizhMSFT left a comment

Choose a reason for hiding this comment

Two-Hearts left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

Two-Hearts left a comment

Choose a reason for hiding this comment

Two-Hearts commented Dec 9, 2024

priteshbandi left a comment

Choose a reason for hiding this comment

JeyJeyGao commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

Two-Hearts left a comment •

edited

Loading

Two-Hearts commented Dec 2, 2024 •

edited

Loading