algorithm: ErrDigestInvalidFormat on Validate() even for unknown algorithms #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In most cases, the results match the analogous test in digest_test.
However, Algorithm("foo").Validate("!") returns ErrDigestUnsupported
while Parse("foo:!") returns ErrDigestInvalidFormat. Because "!" is
an invalid encoded part regardless of algorithm, I think both should
be returning ErrDigestInvalidFormat. I'll fix that in the next
commit; this one just shows the current situation.
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
invalid-format-even-for-some-unrecognized
branch
from
74ad6a6
to
c69c5e5
Compare
|
In case it's hard to pick out in the diff, the user-visible change I'm interested in is here (with some leading tabs removed for this comment): And when @dmcgowan questioned the usefulness of that distinction, my motivating example was that it allows middleware (e.g. registries) to reject invalid encoded portions like |
…rithms
In some cases (e.g. Algorithm("foo").Validate("!")) we know the
encoded portion is invalid even if we do not have an entry for foo in
anchoredEncodedRegexps. Whatever algorithm-specific additional
restrictions are applied via anchoredEncodedRegexps, the encoded
portion must satisfy the generic encoded-portion restrictions from the
DigestRegexp. This commit adjusts the Algorithm("foo").Validate("!")
result to match the current Digest("foo:!").Validate() result, and
factors the encoded portion of DigestRegexp out into encodedRegexp and
encodedRegexpAnchored.
It also guards the Size()-based length check with Available(), because
Size() returns zero for unavailable algorithms. Now that we have a
check for algorithms that aren't in anchoredEncodedRegexps, we can't
rely on lookup-misses there to protect us from running the Size()
check on unavailable algorithms. And equating anchoredEncodedRegexps
hits with Available was dicey anyway, since it depended on what
packages get loaded, etc.
Adding Algorithm.ValidateIdentifier() allows us to avoid doubling up
on the encoded check in Digest.Validate(), since both
DigestRegexpAnchored and algorithm.Validate() were covering the
encoded portion. The new tests are based on digest_test.go's
TestParseDigest.
I've also dropped the Available check from Digest.Valid, because we
can tell:
sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
is a valid digest even if Algorithm("sha256") happens to be
unavailable (e.g. because the consumer didn't import crypto/sha256).
Available-ness just affects whether you're capable of verifying
content against the digest, not digest-validity.
Running Algorithm.Validate before Algorithm.ValidateIdentifier uses
the presence of an identifier in anchoredEncodedRegexps as a hint to
skip the identifier validation. This saves some time (ad Derek's
suggestion [1]) and is safe as long as we don't add any invalid
identifiers to that map.
[1]: opencontainers#34 (comment)
Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
invalid-format-even-for-some-unrecognized
branch
from
c69c5e5
to
9a46ed6
Compare
|
Shuffled around a bit more with c69c5e5 → 9a46ed6 to use presence in |
|
House keeping. With no comments and no current demand for this I'm closing this. If it's still needed we can reopen |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some cases (e.g.
Algorithm("foo").Validate("!")) we know the encoded portion is invalid even if we do not have an entry forfooinanchoredEncodedRegexps. Whatever algorithm-specific additional restrictions are applied viaanchoredEncodedRegexps, the encoded portion must satisfy the generic encoded-portion restrictions from theDigestRegexp. This commit adjusts theAlgorithm("foo").Validate("!")result to match the currentDigest("foo:!").Validate()result, and factors the encoded portion ofDigestRegexpout intoencodedRegexpandencodedRegexpAnchoredas I'd recommended here.It also guards the
Size()-based length check withAvailable(), becauseSize()returns zero for unavailable algorithms. Now that we have a check for algorithms that aren't inanchoredEncodedRegexps, we can't rely on lookup-misses there to protect us from running theSize()check on unavailable algorithms. And equatinganchoredEncodedRegexpshits withAvailablewas dicey anyway, since it depended on what packages get loaded, etc. In #35 I'm dropping thisSizecheck entirely, but I'm happy to rebase whichever PR lands second.