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

[michael-schaller]

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

[carnott-snap]

I'd be extremely cautious about adding a mechanism like this for the whole ecosystem though.

I completely agree, but wanted to call out the limitation, since it was not discussed in the proposal text.

[jayconrod]

I completely agree, but wanted to call out the limitation, since it was not discussed in the proposal text.

Thanks, and good point. I've edited in a note to the proposal for visibility.

[Zemnmez]

@michael-schaller

Can we have something slightly better than comments to denote the retraction reason? It would be particularly good for vulnerability scanners to be able to determine how urgent a vulnerability is. Furthermore a grave bug, like a rarely occurring data corruption issue, could be urgent and should be reflected. So I'd like to see at least some machine readable reason enum and urgency/severity enum as part of the retraction definition.

Maybe. It's not clear at all to me what the format should be. We'd need something extensible that works for all use cases. Before committing to anything, I think we should have a firmer plan for vulernability tracking in general.

I took a shot at this in my previous proposal. It's very minimal but might serve as some inspiration? https://github.com/golang/go/issues/24031#issuecomment-453119084

@Zemnmez I think checking for retracted versions during a build would be too costly. We have to load the version list for each module that provides packages imported by the thing being built. If there are uncached versions, we have to fetch the go.mod file for those. That's why I'm limiting the check to go list -m -u and go get: those commands almost always have to do that work anyway.

I agree that it would still be easy to miss a notification about this though. I just opened #37781, suggesting that gorelease report an error if the module being tested requires a retracted module version.

I think there's an opportunity here for another tool as well: given a list of module versions (perhaps produced by running go version -m on a binary running in production), report whether any version is retracted.

I feel quite strongly that a proposal for marking versions as 'no longer good' would benefit greatly from some basic story about how either it should be extended to support security notifications, or how it intends to support them. Marking packages in the Go ecosystem as vulnerable or not (especially in a way that is widely recognized) is currently a big issue when it comes to Go security overall in my experience.

I think that's the biggest factor into where and how 'retracted' revisions should be surfaced. If all retractions are considered equally critical because there is no story around expressing metadata, then all retractions will have to be treated like a critical security issue for many of the security-critical systems at scale that Go supports -- a noteworthy selection of which, as mentioned before by @michael-schaller should justifiably break the build for some people (perhaps behind a flag?).

This might be a misunderstanding, but this is, I feel a possible advantage of a tag based approach. I assume the logic there can go faster than with files in the revision tree.

edit: I will add that I think, as with the proposal so far that approaching this not from a security issue perspective 'security issue in revision x' and instead a 'critical bug' perspective is probably the better way to go here. I think there are benefits to equating the severities of 'this version will break your build' vs 'this version will render this system useless as per its security requirements'

[jimmyfrasche]

If you parse the comments out godoc-style and could include them in the go list -json as something like .RetractReason so that given

retract v1.2.3 // security vulnerability: see issue #1009

instead of tools just saying v1.2.3 is retracted they could say v1.2.3 is retracted: security vulnerability: see issue #1009

What happens if I release a go.mod with a retraction then delete that line and do another release? Does that retract the retraction?

[michael-schaller]

@jayconrod

@michael-schaller

I miss a section on how to ensure extensibility. How could this be extended without breaking old Go versions?

I got into this a bit in the section Compatibility with old versions. In short, if you use retract in your own module (the main module), you must use Go 1.15 or later. retract may be used in modules you depend on with any Go version that supports modules. Old Go versions will ignore retract outside the main module.

Having a section about backward compatibility is great. What is missing is a section about forward compatibility though. ;-)

For instance if in X years we would want to add an URL field (e.g. bug, ticket, announcement, ...) to the retractions how would that be added? If that would need to be added by extending the comment then IMHO that would reflect poorly on the current design proposal.

Furthermore it might be worth considering to have a standard set of fields (versions and commentfor the beginning) and to allow users to add custom fields on demand (similarly to HTTP X- headers). Then the Go team can standardize the widely used custom fields over time (and also hard deprecate these custom fields to force adoption of the new standard fields)...

Also the current design proposal makes it hard to parse the comment from go.mod as the comment could be a trailing comment but it could also be a leading comment (possibly multi-line). Having a dedicated comment field would IMHO improve this a lot for tools that will process this information (vulnerability scanners, build checkers, ...).

So some kind of machine readable key/value setup for the retractions would IMHO be great.

Can we add 'dead/abandoned/end-of-life' as possible reason as well?

I'd generally advise against using retract for that reason: if you want to encourage users to upgrade to a new version, it's better to do it with a carrot (new features, better performance) instead of a stick. Upgrades (especially to incompatible versions) are costly, and not always worth it from the user's perspective.

My intention wasn't for versions but rather for the whole project/package. You see many abandoned packages where the author/maintainer mentioned this in the README. So it would be great if all versions of a package could be marked as abandoned in the retractions.

Can we have something slightly better than comments to denote the retraction reason? It would be particularly good for vulnerability scanners to be able to determine how urgent a vulnerability is. Furthermore a grave bug, like a rarely occurring data corruption issue, could be urgent and should be reflected. So I'd like to see at least some machine readable reason enum and urgency/severity enum as part of the retraction definition.

Maybe. It's not clear at all to me what the format should be. We'd need something extensible that works for all use cases. Before committing to anything, I think we should have a firmer plan for vulernability tracking in general.

See my other comment about extensibility.

I would like to see that go vet checks for retracted versions. Many people run go vet before commit and so this would IMHO be a good place to check for retracted versions.

This feels a bit out of go vet's wheelhouse to me. You could probably build an analyzer (using golang.org/x/tools/go/analysis, the framework go vet is built on) that does this. But analyzers typically just do static analysis on packages, and they don't usually go out to the network.

I've just opened #37781 to do something like this in gorelease though. That seems like a better fit to me.

Sounds good to me. Thanks.

[michael-schaller]

@Zemnmez Maybe it's best to delay the discussion about vulnerability details. IMHO having a solid, simple and extensible foundation as a first step would simplify all further extensions, like adding vulnerability information. I would also like to see vulnerability details asap but IMHO they aren't required for the very first iteration. Just being able to retract versions will be IMHO already a huge win. All further extensions are icing on the cake to give more and better context.

Also I think @jayconrod made some really good points why versions shouldn't be retracted via tags and so maybe, just maybe, it is time to consider giving up on the tags approach.

[michael-schaller]

@jayconrod Could you add that with the go.mod approach it is also possible for anyone to suggest a retraction via a pull request? This might seem obvious but with the tag approach it would only be possible for repo owners to add a retraction and contributors could only suggest retractions via an issue report.

[jayconrod]

@Zemnmez

I feel quite strongly that a proposal for marking versions as 'no longer good' would benefit greatly from some basic story about how either it should be extended to support security notifications, or how it intends to support them. Marking packages in the Go ecosystem as vulnerable or not (especially in a way that is widely recognized) is currently a big issue when it comes to Go security overall in my experience.

I think that's the biggest factor into where and how 'retracted' revisions should be surfaced. If all retractions are considered equally critical because there is no story around expressing metadata, then all retractions will have to be treated like a critical security issue for many of the security-critical systems at scale that Go supports -- a noteworthy selection of which, as mentioned before by @michael-schaller should justifiably break the build for some people (perhaps behind a flag?).

I agree there should be a better story here, but I'll defer to the Go security team (@katiehockman, @FiloSottile) as to exactly what's needed. So far, it sounds like it would be useful to have a severity and some identifier (URL? CVE number?) in addition to a description.

At the moment, I don't think the go command should behave differently depending on the annotation: that should be up to other tools. I don't think regular builds should fair for example.

Consequently, a comment seems like the right way to express this. For example:

// Vulnerability: CVE-2020-1234
// Severity: HIGH
// Description: Uses ROT13 encryption
retract v1.5.0

Just to throw out a strawman alternative:

retract v1.5.2 (
    vulnerability CVE-2020-1234
    severity HIGH
    description "Uses ROT13 encryption"
)

This would just be a list of key-value pairs, all strings. They would be expressed as a JSON object in go list -m -u -json output.

This might be a misunderstanding, but this is, I feel a possible advantage of a tag based approach. I assume the logic there can go faster than with files in the revision tree.

Not sure I understand. Could you elaborate?

I sketched out a design for this a while back (never published), and I thought the logical place for metadata would be an annotated tag commit message (git only) or in the commit message for the revision the tag points to. Both are free-form text, so they have the same advantages and disadvantages as doc comments for metadata representation.

[jayconrod]

@jimmyfrasche

If you parse the comments out godoc-style and could include them in the go list -json as something like .RetractReason ...

I'm suggesting we just have the field Retracted in the go list -m -u -json output. It would contain the comment, and tools could do what they need with that.

Just added example output to the proposal.

[jayconrod]

@michael-schaller

Having a section about backward compatibility is great. What is missing is a section about forward compatibility though. ;-)

For instance if in X years we would want to add an URL field (e.g. bug, ticket, announcement, ...) to the retractions how would that be added? If that would need to be added by extending the comment then IMHO that would reflect poorly on the current design proposal.

This hinges on whether we can express everything in a comment or if we really need syntax. We should decide that based on whether the go command needs to actively do anything with the comment, besides presenting it in go list -m -u -json output.

If we need syntax, we should allow unrecognized fields for future compatibility.

[jimmyfrasche]

@jayconrod

If the Retracted field is a string how do you disambiguate between no retraction and an uncommented retraction? (Keeping in mind that build infrastructure is often written in looser languages).

Should gorelease warn about an uncommented retraction?

I also asked:

What happens if I release a go.mod with a retraction then delete that line and do another release? Does that retract the retraction?

If it's expected to be append-only, gorelease should probably warn if a version is accidentally removed.

[michael-schaller]

@jayconrod

This hinges on whether we can express everything in a comment or if we really need syntax. We should decide that based on whether the go command needs to actively do anything with the comment, besides presenting it in go list -m -u -json output.

If we need syntax, we should allow unrecognized fields for future compatibility.

The comment part is actually my only major concern about the current design proposal as it can be easily misused if there aren't any other options available. There are already people who voice their preference for machine readable retraction data (including me) but that becomes very hard as soon as people start to pack all kinds of information in a comment. That is really a future I'd like to avoid and hence I urge you to consider a more elaborate syntax. Just a basic key/value store would IMHO improve this design proposal a lot.

Furthermore I disagree that this is only about the go command as the retraction data should really be accessible to everyone, particularly scanners. For instance pkg.go.dev could show retracted versions and additional data (if available). There could also be a Security tab next to Versions tab with just the security retractions...

[tv42]

There are some drawbacks:

• There's no history about when versions were retracted (or un-retracted) and who made the change. [...]
• The set of version control tags, the version list (the $module/@v/list endpoint in the GOPROXY protocol), and the version info ($module/@v/$version.info) are not authenticated by go.sum files or the checksum database. Their contents may change over time, and they can be manipulated by a malicious proxy.

Aren't these drawbacks true for release tags in the first place, in exactly the same way, and implicitly considered acceptable? (More likely, out of scope; it's up the version control to retain history of who does what with it.)

[bcmills]

@tv42, the contents of tagged releases are covered by the checksum database. Their metadata is not, but that metadata is not used for any kind of security auditing today.

[jayconrod]

@jimmyfrasche

If the Retracted field is a string how do you disambiguate between no retraction and an uncommented retraction? (Keeping in mind that build infrastructure is often written in looser languages).

Should gorelease warn about an uncommented retraction?

An uncommented retraction would have a Retracted field with the empty string as the value. The Retracted field would not be present if there was no retraction.

What happens if I release a go.mod with a retraction then delete that line and do another release? Does that retract the retraction?

Sorry for missing this question earlier

That would un-retract the version. That's okay. Just as there are cases where a version is published accidentally, there will be cases where a version is retracted accidentally, so the ability to undo a retraction is important.

[jayconrod]

@michael-schaller

The comment part is actually my only major concern about the current design proposal as it can be easily misused if there aren't any other options available. There are already people who voice their preference for machine readable retraction data (including me) but that becomes very hard as soon as people start to pack all kinds of information in a comment. That is really a future I'd like to avoid and hence I urge you to consider a more elaborate syntax. Just a basic key/value store would IMHO improve this design proposal a lot.

Furthermore I disagree that this is only about the go command as the retraction data should really be accessible to everyone, particularly scanners. For instance pkg.go.dev could show retracted versions and additional data (if available). There could also be a Security tab next to Versions tab with just the security retractions...

Point taken. I'll experiment with the syntax and chat with @rsc about this.

I definitely like the idea of pkg.go.dev surfacing this, too.

[michael-schaller]

@jayconrod

Point taken. I'll experiment with the syntax and chat with @rsc about this.

Sorry if this endangers your goal to implement this for Go 1.15.

[jimmyfrasche]

My concern is code that treats both the absence of the Retract field and a Retract field with a falsey value as "not retracted".

[rsc]

I'm skeptical about using retract statements as a database of security vulnerabilities. There are many reasons a particular version might be retracted, not all of them security. Note that this very issue title says "insecure, incompatible, or broken". We shouldn't overfit to security. That's biting off too much.

Remember, go.mod is for directives to the go command related to version selection. Security information should be elsewhere. @FiloSottile is working on that problem; it's out of scope for go.mod.

What we want to avoid is go.mod becoming a complex, general-purpose dumping ground for all possible information about the code. That's the easy answer, but not the right one.

Also, note that this syntax:

retract v1.5.2 (
    vulnerability CVE-2020-1234
    severity HIGH
    description "Uses ROT13 encryption"
)

does not make sense since the go command defines ( ) as factoring, like Go itself. This would need to be semantically equivalent to:

retract v1.5.2 vulnerability CVE-2020-1234
retract v1.5.2 severity HIGH
retract v.1.5.2 description "Uses ROT13 encryption"

That would in turn disallow us from flagging the same version appearing in multiple retractions.

[rsc]

My suggestion is to scale back, not attempt to be a security database, and have just a plain-text comment, like we do for doc comments in Go code (like in struct fields). Tooling could easily recognize a CVE number or a URL by syntax and pull them out.

The syntax I propose is

retract version... // optional comment

as in

retract v1.5.0 v1.5.1 v1.5.2 // buggy marshaling (https://golang.org/issue/12345)
retract v1.6.0 // accidental incompatible API change

We should also allow closed intervals like:

retract [v1.5.0, v1.5.2] // buggy marshaling (https://golang.org/issue/12345)

And we should prep the go command now so that we can allow open or half-open intervals. (Today the ( ) parsing gets in the way, but we can limit that to ( at end of line and ) at beginning of line.)

[michael-schaller]

@jayconrod Could you add a documentation section to your design that outlines how this feature will be documented? What I'm particularly looking for is how the retraction feature is intended to be used and how it shouldn't be used. For instance that a retraction is merely a hint by the package maintainer(s) that a specific version (range) should be avoided because of the commented reason but that package users are free to ignore this hint. Also that retractions should not be used to deprecate old (but perfectly working) versions to force users to update to newer versions as that is against the spirit of the whole Go modules minimal version selection principle.

@jayconrod @rsc I'm a bit unhappy with having the comment optional. The package maintainer(s) are sending a message with a retraction notice and without a comment/reason the message is just "avoid" which isn't all that helpful to package users, particularly as then they can't decide for themselves if they should follow the retraction recommendation or not. Also having a mandatory reason sends a bit of a stronger message to the package maintainer(s) that a retraction shouldn't be made lightly. So please consider having a mandatory reason instead of an optional comment.

@jayconrod @rsc @FiloSottile Could you open a follow up bug for the security vulnerability tracking? It would also be great if the new bug wouldn't just reference this bug but would contain a summary of what has been discussed in this bug.

[gopherbot]

Change https://golang.org/cl/226639 mentions this issue: modfile: scan ASCII brackets and commas as separate tokens

[rsc]

Does anyone object to the design in https://github.com/golang/go/issues/24031#issuecomment-603922991, with the understanding that security details will be elsewhere?

[alex]

Can you expand on "security details will be elsewhere"? Will indicating a release contains a vuln still be done with the retract syntax?

[rsc]

Tooling could easily recognize a CVE number or a URL by syntax and pull them out; that's where the security details would probably be maintained. We don't know for sure; what we do know is that go.mod is for deciding which versions of modules to use, not a security database.

[jayconrod]

I think it makes sense to have a separate mechanism for tracking security vulnerabilities.

Retractions solve a related problem but not the same problem. 1) Authors shouldn't be the only people that can report vulnerabilities on their modules, and 2) it should be possible to report a vulnerability without retracting an entire version, for example, on specific package used in a specific way. Not all users may be affected.

That said, I think much of the user experience in the go command should be the same. go list -m should show information about vulnerabilities (separate from retractions, perhaps with another flag). go get should avoid upgrading to a vulnerable version unless it's requested specifically.

I don't expect a proposal for vulnerability tracking will be ready for 1.15 though. I think it should be possible to add it later as another channel of information without changing retraction.

[alex]

Ok, so an author wanting a retract a vulnerable version would use this retraction mechanism, and include a comment, which maybe of some form that a third party tool can recognize (e.g. a CVE, a link to some other vuln aggregator, etc.) seems reasonable to me.

[gopherbot]

Change https://golang.org/cl/228039 mentions this issue: modfile: support retract directive and version intervals

[gopherbot]

Change https://golang.org/cl/228379 mentions this issue: cmd/go/internal/modload: refactor version filtering for exclude

[gopherbot]

Change https://golang.org/cl/228383 mentions this issue: cmd/go: improve 'go get' handling of retracted versions

[gopherbot]

Change https://golang.org/cl/228377 mentions this issue: cmd/go: update vendored golang.org/x/mod

[gopherbot]

Change https://golang.org/cl/228380 mentions this issue: cmd/go/internal/modload: support go.mod retract directive

[gopherbot]

Change https://golang.org/cl/228382 mentions this issue: cmd/go: add -retracted flag to 'go list'

[gopherbot]

Change https://golang.org/cl/228381 mentions this issue: cmd/go: add -retract and -dropretract flags to 'go mod edit'

[gopherbot]

Change https://golang.org/cl/228378 mentions this issue: cmd/go/internal/modload: extract readGoMod from mvsReqs.Required

[gopherbot]

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

[gopherbot]

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

[jayconrod]

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]

@jayconrod Could you please give a progress update?

[jayconrod]

@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]

Makes sense. Thanks for the update, @jayconrod.

[jayconrod]

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

[michael-schaller]

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]

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.

[gopherbot]

Change https://golang.org/cl/270557 mentions this issue: cmd/go: fix retract interval syntax in 'go help mod edit'

[gopherbot]

Change https://golang.org/cl/272066 mentions this issue: cmd/go: recommend 'go get' command to switch from retracted versions

[icholy]

warning: bikeshedding

I was a little surprised with the choice of square brackets.

retract [ v1.0.0 ]

I would have expected parenthesis.

retract v1.0.0

retract (
  v1.0.0
  v2.0.0
)

edit

There also seem to be some issues with the parser:

retract ( v1.0.0 )

# go: errors parsing go.mod:
# go.mod:5: expected '[' or version

retract (
  v1.0.0
)

# no error

retract (
  v1.0.0
  v2.0.0
)

# go: errors parsing go.mod:
# go.mod:7:3: malformed module path "": empty string

retract [
  v1.0.0
  v2.0.0
]

# go: errors parsing go.mod:
# go.mod:3: expected version after '['
# go.mod:4:3: unknown directive: v1.0.0
# go.mod:5:3: unknown directive: v2.0.0
# go.mod:6: unknown directive: ]

[jayconrod]

@icholy Square brackets denote closed intervals. For now, open and half-open intervals aren't supported because it would break the parser used in Go 1.14 and earlier. We may support those later, maybe for replace and exclude, too.

Blocks of multiple directives are still delimited by parentheses.

If you find parser bugs, please open a new issue. Those examples look like they're working as intended though.

[icholy]

@icholy Square brackets denote closed intervals. For now, open and half-open intervals aren't supported because it would break the parser used in Go 1.14 and earlier. We may support those later, maybe for replace and exclude, too.

That makes sense, but it's definitely not obvious from looking at it. I was expecting every retracted version to be explicitly listed.

Those examples look like they're working as intended though.

This doesn't seem like valid syntax

retract (
  v1.0.0
)

# no error

Why is it looking for a module path?

retract (
  v1.0.0
  v2.0.0
)

# go: errors parsing go.mod:
# go.mod:7:3: malformed module path "": empty string

[liggitt]

Why is it looking for a module path?

retract (
  v1.0.0
  v2.0.0
)

# go: errors parsing go.mod:
# go.mod:7:3: malformed module path "": empty string

v2.0.0 is not a valid module version for a module whose name doesn't end in /v2. If you had a v2.0.0 tag on a non-v2 module you wanted to retract, you'd need to retract the module-ized version, e.g. v2.0.0+incompatible