Implement encoding.TextUnmarshaler in Digest

[mtrmac]

... to ensure invalid values are rejected.

Otherwise Go would allow setting a Digest value to arbitrary strings, causing a later panic or other misuse if users forget to call Validate(). (e.g. containers/image#2403 , CVE-2024-3727 .)

I don’t think adding this validation should break correct programs, although it can’t quite be ruled out. I did observe this breaking unit tests which were using unrealistic invalid Digest values.

[stevvooe]

LGTM

I'd like to get another opinion on this, as it can create bugs in other systems that delay validation after serialization.

[tianon]

This is definitely a breaking change, right? It's going to be hard to quantify exactly how widespread the breakage is, but it doesn't seem terribly unusual IMO for a project to have used this Digest type in a JSON context and round tripping string values that are not technically valid (since to actually use the value, you have to either explicitly validate it or suffer a panic).

[thaJeztah]

Yes, this is a tricky one; I wonder if this code could be used in situations where the consumer doesn't actually need it to be valid (I guess that's roughly what @tianon describes above, but in better words).

Also not gonna lie that this module has quite some potential footguns, and I cursed a few times when we had code that didn't validate, and panicked after it turned out that code depended on <some other random package> that happened to be importing crypto/sha256.

I wonder if we can make overall use less error-prone, but this very likely would require a "v2".

[mtrmac]

An alternative implementation could use just a DigestRegexpAnchored match, to also allow unknown digest algorithms. In theory, that would be more compatible if the deployed algorithms changed over time.

As for users who store completely invalid strings while marshaling/unmarshaling digest.Digest values, I don’t see how that makes sense, and I don’t think it should be encouraged, but I can’t quite rule out that somebody somewhere is doing that in production.

[dmcgowan]

We could ignore ErrDigestUnsupported here

[tianon]

An example invalid string I've personally stored and seen in these many times is the empty string, and I think that one is probably pretty common (although I don't know if it's also common to marshal that to/from JSON), but that case is already covered here. 😅