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

cmd/go: allow package authors to mark older package versions as insecure, incompatible, or broken #24031

Closed
michael-schaller opened this issue Feb 22, 2018 · 94 comments
Assignees
Labels
Milestone

Comments

@michael-schaller
Copy link
Contributor

@michael-schaller michael-schaller commented Feb 22, 2018

In order to achieve reproducible builds vgo keeps using specific package versions until an explicit upgrade is done. IMHO this is an excellent default but I'm worried about insecure package versions as currently vgo can't detect if the build contains an insecure package version.

Can vgo be changed so that a package author is able to specify that every version below X is deemed insecure and if an insecure package version is used during a build that the build will fail (with a flag to override)?

@gopherbot gopherbot added this to the vgo milestone Feb 22, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 22, 2018

@michael-schaller I'm not sure what new functionality you are asking for.

Right now vgo will not choose a version "below" a version you specify. So if there is an insecure package version, put the minimum version selector in your package or another package and it will not choose it. Maybe For modules that build main packages, you can also specify version ranges to exclude. Maybe I'm missing something?

@ALTree
Copy link
Member

@ALTree ALTree commented Feb 22, 2018

So if there is an insecure package version, put the minimum version selector in your package or another package and it will not choose it.

This only works if you know that a certain version is insecure. I think what he's asking for is a mechanism for package authors to broadcast to the world that a certain version is insecure; so that every time a user pulls it, they'll be warned that that version is deprecated and they'll know to update their mod file.

@kardianos kardianos changed the title x/vgo: Reproducible build vs. insecure package version x/vgo: allow package authors to mark packages as insecure Feb 22, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 22, 2018

@ALTree That makes sense. Thanks for clarifying for me. I think that is a fine question to ask and may go hand-in-hand with the question of how to distribute the content / function of go.modverify.

@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Feb 22, 2018

@ALTree correct. :-)

One naive idea would be that 'vgo build' could check the 'go.mod' (or another machine readable file) of the latest package versions for security information. This would also be great for Continuous Integration as then a package author could notify of security issues via CI build failures that are (hopefully) monitored.

@michael-schaller michael-schaller changed the title x/vgo: allow package authors to mark packages as insecure x/vgo: allow package authors to mark older package versions as insecure Feb 22, 2018
@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Feb 24, 2018

@rsc mentioned Deprecated Versions (as part of the Defining Go Modules article) which is similar to this issue. He proposed to append +deprecated to a version tag which would also be a viable solution for this issue if +insecure would be honored by vgo.

IMHO that would be a pretty bare bones solution though as I presume that people would soon want to extend that further. For an instance I could see that someone would also want +buggy for a version with a serious bug (for an example a serious memory leak) or +broken for a version that is broken under certain circumstances (for an example the Windows build is broken). Furthermore this solution lacks a way to add more context as for deprecated versions one might be interested in the deprecation announcement or timeline and for insecure versions one might be interested in CVE, severity, ... and so on.

That said I think signaling via tags if a version is deprecated, insecure, ... is not adequate. Maybe even the proposal from my previous comment isn't. Maybe the discussion should rather go into the direction of a machine readable changelog which could be managed via vgo release.

@uluyol
Copy link
Contributor

@uluyol uluyol commented Feb 24, 2018

I'm not sure about the utility of this feature. If I release version 1.2.3, surely it must be fixing some bugs over 1.2.2 and I would probably mark 1.2.2. In other words, on almost every (point) release, I would mark all older versions as insecure. You might say this is only for bugs with a CVE, but I think the point still stands.

I don't see this providing much over just reporting that newer versions have been published.

@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Feb 25, 2018

@uluyol there are actually multiple points for this feature (which I sadly failed to point out so far):

  1. It enables machines to automatically react:
    It's a chore to check for new versions and humans are sadly error prone, especially in burdensome cases of chore like this one. Having a machine readable form to notify of insecure, deprecated, buggy, ... versions enables machines to react so that humans can spend their time on better things. In this case vgo would be able to detect an unhealthy build and could let the build fail.

  2. Cut down reaction time:
    Once machines are able to react on such issues you can cut down reaction time drastically as for an instance Continuous Integration would fail and ideally you would automatically get a bug report for the build failure with the details why the build failed. This is especially valuable for projects that aren't any longer in the active development phase but rather receive infrequent updates on demand.

  3. Indicator for healthiness of new dependencies:
    If you add a new dependency to your project you get immediately a sense of the healthiness of that new dependency. If the new dependency is unhealthy vgo should fail to build and with that you know that you either need to change your project, need to file a bug report against the new dependency or you need to fork and fix the new dependency (if you still want to use it). For an instance this would catch the error that a human tries to add the new dependency with the major version v1 albeit that version is deprecated and a newer major version should be used.

  4. Indicator for healthiness of indirect dependencies:
    In the end you should have a cascading effect in case a package version is marked insecure as every other package that directly and most importantly indirectly depends on it should fail to build.

@uluyol
Copy link
Contributor

@uluyol uluyol commented Feb 25, 2018

I see. I agree that tags lack content, and that it would be useful to also have a reason (or changeling) listed for why a version is insecure. Then on build you could get messages like

build failed: insecure packages
oauth2: CVE-X-Y vulnerable to DoS
zstd: crash on invalid input

I can see why this would be useful. I do think that we'd want to let people override this behavior though.

@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Feb 25, 2018

A definite +1 on the letting people override this behavior part. :-)

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Feb 25, 2018

I agree that additional in-band signalling would be useful way to let the maintainer know that action may need to be taken—but I disagree that every situation requires automatic, mechanical action.

Security fixes are important but automatically applying them can be as fraught as always ignoring them.

If the module in question is for a non-opensourced essential service using foo v1.0.5 and there's a foo v1.1.4+security that needs to be immediately investigated by a person. However, if its fixing a vulnerability introduced in foo v1.1.0 it may not necessarily be worth the effort and risk to drop everything and upgrade right now.

@4ad
Copy link
Member

@4ad 4ad commented Feb 27, 2018

I would prefer if vgo would continue to work the way it does today, with another tool, perhaps vgo audit that could check online if the versions used have problems.

The idea is that this tool is run on-demand by the programmer, rather than automatically. If this tool is easy to use, vgo audit could become as natural as running go fmt.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 2, 2018

The purpose of vgo is to add versions to the vocabulary of the toolchain, so that users and tools can talk to each other sensibly about versions. As I mentioned in the https://research.swtch.com/vgo-module article, I think it would make sense to have a v1.2.3+deprecated tag, using an annotated tag so that there's a commit message. The commit message can say anything it wants about why the release is deprecated, and we can show that to users. We could easily add a notation in the text for identifying security problems. What happens next is up to tools. Probably vgo list -m -u (tell me about pending module updates) would do well to show information about currently-used modules that have deprecation notices.

@rsc rsc modified the milestones: vgo, vgo2 Jun 6, 2018
@rsc rsc modified the milestones: vgo2, Go1.12 Jul 12, 2018
@rsc rsc changed the title x/vgo: allow package authors to mark older package versions as insecure cmd/go: allow package authors to mark older package versions as insecure Jul 12, 2018
@rsc rsc added the modules label Jul 12, 2018
@rsc
Copy link
Contributor

@rsc rsc commented Jul 25, 2018

I've been thinking a bit about where to write down this information. The magic extra tag is clearly too limited in what it can record. I looked briefly into finding a way to write more information, such as using an annotated tag's commit message in Git, using svn propset to record a special per-revision property in Subversion, and so on. But something that must be reinvented for every different version control system is a bad idea.

Of course, we can't write the information in the original module version's go.mod, since we didn't know it was insecure when we tagged it, and the file tree is by convention (and enforcement via go.sum) immutable after tagging.

But maybe we can record it in a go.mod in a later release of the same module. Specifically, we could say that to look for updated post-release metadata about a particular module we grab the latest version's go.mod and look there. So for example, suppose v1.1.2 has a security problem, it was fixed in a rewrite for v1.2.0, and we're up to v1.2.4 when we discover the problem. Then we'd issue a v1.2.5 that is just v1.2.4 with an updated go.mod that adds something like:

bug example.com/mymodule/rpc v1.1.2-v1.2.0 https://example.com/issue1234 "RPC client bug - can get stuck if too many servers restart"

The fields are "bug", the affected package (if you don't use this package you don't have the bug), the half-open version range when the bug existed, a URL with more information, and a short description. Maybe a security bug would conventionally begin with a "security: " prefix in the description.

Then any future "go get", even one not asked about that module, would look up the latest version, find v1.2.5, learn about the bug in v1.1.2, and print a warning. Also, we could make this information available to running programs, which could inspect their own binaries for the package and version and then self-diagnose on a server status page, automatically report to local monitoring systems, and so on. (We've done something like this inside Google since early 2013 and it works really well.)

If we later decided to issue a v1.1.3 with that fix, we could issue a v1.2.6 that only updates go.mod:

bug example.com/mymodule/rpc v1.1.2-v1.1.3 https://example.com/issue1234 "RPC client bug - can get stuck if too many servers restart"

If we wanted to warn people about the bug but didn't have time to fix it yet, or the bug has been there from the beginning, the half-open interval can drop either side:

bug example.com/mymodule/rpc v1.1.2- https://example.com/issue1234 "RPC client bug - can get stuck if too many servers restart"
bug example.com/mymodule/rpc -v1.1.3 https://example.com/issue1234 "RPC client bug - can get stuck if too many servers restart"
bug example.com/mymodule/rpc - https://example.com/issue1234 "RPC client bug - can get stuck if too many servers restart"

The same general idea could apply to marking earlier versions deprecated or to reporting known conflicts with other dependencies.

It's slightly awkward to be issuing go.mod-update-only patch releases, but doing so creates a history of the annotations and makes them available via module proxies without special arrangement.

All of this is still sketchy but the above seems like it's on a better path than just the +deprecated tags.

@mattfarina
Copy link

@mattfarina mattfarina commented Aug 6, 2018

@rsc for some clarification, when should the bug directive be used? Only when there are security issues? Security issues plus deprecation? If deprecation, is there a common test people should us for marking something as such (e.g., every patch release)? Every bug?

Note, for every bug I poked at a couple repos I've had to deal with:

  • the go-grpc repo has 76 closed bugs
  • kubernetes/kubernetes has 4,109 closed bugs
  • gin-gonic/gin as 28 closed bugs

I tried to look at a sampling of typical and worse case scenarios.

@thepudds
Copy link

@thepudds thepudds commented Aug 6, 2018

@mattfarina If I have followed, I believe @rsc has referred (e.g., here and elsewhere) to this particular issue #24031 as potentially also being part of the solution for recording pair-wise incompatibility post publishing:

as I noted in #24301 on 5/25, I do want to find a place to record incompatibilities, but one that allows recording them after the fact instead of requiring accurate prediction of the future.

and:

More generally we need a place to record other known problems with already-released versions, like security bugs. That's #24031. Maybe the answer there applies to incompatibilities too.

And in #24031 (comment), towards the end of that more recent comment (which was mostly using security or a general bug as an example), Russ also added:

The same general idea could apply to marking earlier versions deprecated or to reporting known conflicts with other dependencies.

That said, the one-liner here is currently:

"cmd/go: allow package authors to mark older package versions as insecure"

If the intent of this particular issue #24031 is broader than security, it might make sense to update the one-liner to help people know where to discuss which topic (vs. maybe #26829 is the better place to discuss recording incompatibilities, or ___).

@thepudds
Copy link

@thepudds thepudds commented Aug 6, 2018

@mattfarina Said another way, using the start of your example from #26829:

Consider the following case where you have an app with a dependency on a module (modA) that has a dependency on another module (modB).

App --> modA --> modB

modB is released with a bug.

My understanding of the discussion of #24031 (both in the issue here and outside of github) is that #24031 might be the answer for:

  • the author of modA declares certain versions of modA do not want to use the buggy v.1.2.3 of modB
  • the author of "App" becoming aware of the actions of the author of modA
  • all of that happening without requiring any action on the part of the author of modB

Mechanism still TBD, but perhaps the mechanism (if I've followed the discussion) might be something like:

  • the author of modA produces a new go.mod in a later release of modA that declares that certain versions of modA are incompatible with the buggy v1.2.3 of modB
  • the author of 'App' issues a 'go get' (where perhaps that 'go get' is not directly related to modA or modB, or perhaps a different go command), and the author of 'App' is warned about the pair-wise incompatibility between the versions of modA and modB now in use in 'App' (or perhaps it is something other than a warning).
  • and all of that is done without needing to predict the future (by declaring future incompatibilities that don't exist yet at the moment of publishing a release) and also without needing to update an immutable published release.

But sorry if this is off base or just noise... and I agree some clarity on what this particular issue is intended to cover would be helpful, because it is an important set of topics...

@Zemnmez
Copy link

@Zemnmez Zemnmez commented Jan 3, 2019

@myitcv mentioned this issue. I put forward #29537 which i think would go some way to solving this issue

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 15, 2020

Change https://golang.org/cl/228384 mentions this issue: doc: add module retraction to release notes

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2020

Change https://golang.org/cl/230697 mentions this issue: modfile: add retract directives to TestParseLax

gopherbot pushed a commit to golang/mod that referenced this issue Apr 29, 2020
Old versions of modfile before CL 228039 should be able to parse and
ignore retract directives with version intervals.

Updates golang/go#24031

Change-Id: I0f35261997c0704a785ea8467dc5ed0738549966
Reviewed-on: https://go-review.googlesource.com/c/mod/+/230697
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit to golang/mod that referenced this issue Apr 29, 2020
The characters '( ) [ ] { } ,' are now scanned as separate tokens,
even when they are preceded by non-whitespace "identifier"
characters. Previously, '( )' were scanned like this when preceded by
whitespace, but they could be in the middle of an identifier
token. None of these characters are allowed in module paths or
versions. They are allowed within file paths, so file paths containing
them will need to be quoted in the future. Using these characters
should not break ParseLax, since replace directives (the only directive
that allows files paths) are ignored by ParseLax.

Additionally, '(' is only treated as the beginning of a block if it
appears at the end of the line or is immediately followed by ')' at the
end of the line. ')' is treated as the end of a block if it
appears within a block at the beginning of a line.

Fixes golang/go#38167
Updates golang/go#38144
Updates golang/go#24031

Change-Id: I5a7fb522163802c3723d289cf0fbc5856ca075ec
Reviewed-on: https://go-review.googlesource.com/c/mod/+/226639
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented May 1, 2020

Just wanted to give a quick update.

First, a couple changes to the big design doc comment above:

  • go list -m will have a -retracted flag that will include retracted versions in version lists and consider them when resolving version queries. They'll be hidden otherwise.
  • Version intervals look like this: [v1.1.0, v1.2.0]
  • One version or version interval per line.

Draft CLs of the implementation are linked above in comments. This is a complicated change, and as it's going through the review process, we're still making decisions on how to deal with all the corner cases. For example, we're deciding how to deal with versions retracted multiple times with different rationales (we'll likely show a list of rationales, instead of one string). This interacts with exclude in some interesting ways (excluded versions should be ignored in the same situations that retracted versions are; they currently aren't).

And now some bad news:

Today is the first day of the Go 1.15 freeze. While CLs mailed before the freeze may still be merged for another week, given the number of open questions we still have to resolve in review, this change will probably not make it into 1.15. That said, I'm hoping we can resolve everything in the next few weeks so this can be merged at the beginning of the 1.16 development cycle. I'll update the milestone at the end of next week to confirm this.

Sorry for the delay, and thanks for your patience. We want to get this right.

@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Jul 17, 2020

@jayconrod Could you please give a progress update?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jul 17, 2020

@michael-schaller The development work for this is mostly done, though I'm still waiting for a +2 on several CLs. CL 228384 is the last CL in the stack.

These can't land until after the 1.15 code freeze is over and 1.16 development starts though.

@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Jul 17, 2020

Makes sense. Thanks for the update, @jayconrod.

gopherbot pushed a commit to golang/mod that referenced this issue Aug 24, 2020
This CL adds support for parsing and programmatically adding and
removing a new directive, "retract", as described in golang/go#24031.

The "retract" directive comes in two forms:

    retract v1.0.0           // single version
    retract [v1.1.0, v1.2.0] // closed interval

Updates golang/go#24031

Change-Id: I1236c7d89e7674abf694e49e9b4869b14a59fac0
Reviewed-on: https://go-review.googlesource.com/c/mod/+/228039
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
Query and other functions now accept an "allowed" function that
returns an error (previously, the function returned a bool). If the
error is equivalent to ErrDisallowed, it indicates the version is
excluded (or, in a future CL, retracted). This provides predicates a
chance to explain why a version is not allowed.

When a query refers to a specific revision (by version, branch, tag,
or commit name), most callers will not use the Allowed predicate. This
allows commands like 'go list -m' and 'go mod download' to handle
disallowed versions when explicitly requested. 'go get' will reject
excluded versions though.

When a query does not refer to a specific revision (for example,
"latest"), disallowed versions will not be considered.

When an "allowed" predicate returns an error not equivalent to
ErrDisallowed, it may be ignored or returned, depending on the
case. This never happens for excluded versions, but it may happen for
retractions (in a future CL). This indicates a list of retractions
could not be loaded. This frequently happens when offline, and it
shouldn't cause a fatal or warning in most cases.

For #24031

Change-Id: I4df6fb6bd60e3e0259e5b3b4bf71a307b4b32298
Reviewed-on: https://go-review.googlesource.com/c/go/+/228379
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
This CL vendors go.mod parser changes for the retract directive.

For #24031

Change-Id: Ief19b0eca4c7956eceadc893bb209da7e9ecf22c
Reviewed-on: https://go-review.googlesource.com/c/go/+/228377
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
The go command now recognizes 'retract' directives in go.mod. A
retract directive may be used by a module author to indicate a
version should not be used. The go command will not automatically
upgrade to a retracted version. Retracted versions will not be
considered when resolving version queries like "latest" that don't
refer to a specific version.

Internally, when the go command resolves a version query, it will find
the highest release version (or pre-release if no release is
available), then it will load retractions from the go.mod file for
that version. Comments on retractions are treated as a rationale and
may appear in error messages. Retractions are only loaded when a query
is resolved, so this should have no impact on performance for most
builds, except when go.mod is incomplete.

For #24031

Change-Id: I17d643b9e03a3445676dbf1a5a351090c6ff6914
Reviewed-on: https://go-review.googlesource.com/c/go/+/228380
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
'go mod edit' can now add and remove 'retract' directives from go.mod
files.

Also, retractions are now included in the 'go mod edit -json' output.

For #24031

Change-Id: Ife7915e259fa508626d6ec5f786b5c860b489599
Reviewed-on: https://go-review.googlesource.com/c/go/+/228381
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
The -retracted flag causes 'go list' to load information about
retracted module module versions.

When -retracted is used with -f or -json, the Retracted field is set
to a string containing the reason for the retraction on retracted
module versions. The string is based on comments on the retract
directive. This field is also populated when the -u flag is used.

When -retracted is used with -versions, retracted versions are shown.
Normally, they are omitted.

For #24031

Change-Id: Ic13d516eddffb1b8404e21034f78cecc9896d1b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/228382
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
'go get' will now warn about retracted versions in the build list,
after updating go.mod. The warning instructs users to run
'go get module@latest' to upgrade or downgrade away from the retracted
version.

'go get' now allows users to explicitly request a specific retracted
version.

For #24031

Change-Id: I15fda918dc84258fb35b615dcd33b0f499481bd7
Reviewed-on: https://go-review.googlesource.com/c/go/+/228383
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Aug 26, 2020
For #24031

Change-Id: I9bd0905e9aacee4bec3463b7d91f6f0929744752
Reviewed-on: https://go-review.googlesource.com/c/go/+/228384
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 27, 2020

This is now implemented at tip. Feel free to try it out with golang.org/dl/gotip!

@michael-schaller
Copy link
Contributor Author

@michael-schaller michael-schaller commented Aug 28, 2020

Thank you, Jay! :-D

BTW: Where is the work tracked for the retract tutorial or blog post mentioned in the draft release notes ( https://tip.golang.org/doc/go1.16#go-command)?

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 28, 2020

BTW: Where is the work tracked for the retract tutorial or blog post mentioned in the draft release notes ( https://tip.golang.org/doc/go1.16#go-command)?

There's not a separate issue for that on the issue tracker right now; they'll be written closer to the 1.16 release. We don't usually file issues for upcoming blog posts and articles. We're still working out how to divide things up and where they should go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.