-
Notifications
You must be signed in to change notification settings - Fork 60
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
disallow upper characters (/A-F/) in hex-encoded portion #34
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import ( | |
"fmt" | ||
"hash" | ||
"io" | ||
"regexp" | ||
) | ||
|
||
// Algorithm identifies and implementation of a digester by an identifier. | ||
|
@@ -28,9 +29,9 @@ type Algorithm string | |
|
||
// supported digest types | ||
const ( | ||
SHA256 Algorithm = "sha256" // sha256 with hex encoding | ||
SHA384 Algorithm = "sha384" // sha384 with hex encoding | ||
SHA512 Algorithm = "sha512" // sha512 with hex encoding | ||
SHA256 Algorithm = "sha256" // sha256 with hex encoding (lower case only) | ||
SHA384 Algorithm = "sha384" // sha384 with hex encoding (lower case only) | ||
SHA512 Algorithm = "sha512" // sha512 with hex encoding (lower case only) | ||
|
||
// Canonical is the primary digest algorithm used with the distribution | ||
// project. Other digests may be used but this one is the primary storage | ||
|
@@ -50,6 +51,14 @@ var ( | |
SHA384: crypto.SHA384, | ||
SHA512: crypto.SHA512, | ||
} | ||
|
||
// anchoredEncodedRegexps contains anchored regular expressions for hex-encoded digests. | ||
// Note that /A-F/ disallowed. | ||
anchoredEncodedRegexps = map[Algorithm]*regexp.Regexp{ | ||
SHA256: regexp.MustCompile(`^[a-f0-9]{64}$`), | ||
SHA384: regexp.MustCompile(`^[a-f0-9]{96}$`), | ||
SHA512: regexp.MustCompile(`^[a-f0-9]{128}$`), | ||
} | ||
) | ||
|
||
// Available returns true if the digest type is available for use. If this | ||
|
@@ -164,3 +173,20 @@ func (a Algorithm) FromBytes(p []byte) Digest { | |
func (a Algorithm) FromString(s string) Digest { | ||
return a.FromBytes([]byte(s)) | ||
} | ||
|
||
// Validate validates the encoded portion string | ||
func (a Algorithm) Validate(encoded string) error { | ||
r, ok := anchoredEncodedRegexps[a] | ||
if !ok { | ||
return ErrDigestUnsupported | ||
} | ||
// Digests much always be hex-encoded, ensuring that their hex portion will | ||
// always be size*2 | ||
if a.Size()*2 != len(encoded) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can drop this block. For algorithms in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done, also removed ErrDigestInvalidLength There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
While I think we may want to keep the |
||
return ErrDigestInvalidLength | ||
} | ||
if r.MatchString(encoded) { | ||
return nil | ||
} | ||
return ErrDigestInvalidFormat | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,26 +101,18 @@ func FromString(s string) Digest { | |
// error if not. | ||
func (d Digest) Validate() error { | ||
s := string(d) | ||
|
||
i := strings.Index(s, ":") | ||
|
||
// validate i then run through regexp | ||
if i < 0 || i+1 == len(s) || !DigestRegexpAnchored.MatchString(s) { | ||
if i <= 0 || i+1 == len(s) { | ||
return ErrDigestInvalidFormat | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regexp check is no longer needed, the second regexp will do all the needed validation and the algorithm lookup validates the algorithm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think it is. What if someone tries to make a digest with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am only talking about the regexp check, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Dropping the digest regexp is going to make this package much more permissive than image spec (which has a number of charset restrictions). That's fine, go-digest is free to be more permissive, but it would be nice if there was a way to ask “is this a valid image-spec digest?”. “Does go-digest have any personal beef with this?” is a useful question, but potentially less useful for folks using go-digest to handle image-spec digests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not suggesting we drop the drop the regexp from the package. I am pointing out that running this regexp is redundant when the input is valid. If returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The distinction is that
Yeah, I'm fine with that. |
||
} | ||
|
||
algorithm := Algorithm(s[:i]) | ||
algorithm, encoded := Algorithm(s[:i]), s[i+1:] | ||
if !algorithm.Available() { | ||
if !DigestRegexpAnchored.MatchString(s) { | ||
return ErrDigestInvalidFormat | ||
} | ||
return ErrDigestUnsupported | ||
} | ||
|
||
// Digests much always be hex-encoded, ensuring that their hex portion will | ||
// always be size*2 | ||
if algorithm.Size()*2 != len(s[i+1:]) { | ||
return ErrDigestInvalidLength | ||
} | ||
|
||
return nil | ||
return algorithm.Validate(encoded) | ||
} | ||
|
||
// Algorithm returns the algorithm portion of the digest. This will panic if | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is safer than before, but we can still do better (assuming go-digest is ok with the spec restrictions on the encoded charset). @stevvooe felt that had already been handled in “the top-level validate”, but
Algorithm.Validate
is a public method, so you can hit it without going throughDigest.Validate
. I expect we want to handle the following cases:anchoredEncodedRegexps
, which you already handle correctly.anchoredEncodedRegexps
and:ErrDigestUnsupported
like you're doing here.ErrDigestInvalidFormat
even without recognizing the algorithm.I'm not clear which case zero-length encoded parts fall into; they should be either
ErrDigestInvalidFormat
(which you've chosen below) orErrDigestInvalidLength
. Either way, the comment for this function should document which applies to that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking This validates the character set for the encoded portion for the algorithm. Nothing less, nothing more. If you use a function incorrectly, you pay the price. Please stop trying to expand the scope of things.
This is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if your
Algorithm
is not listed inanchoredEncodedRegexps
, we can still validate the encoded portion of the algorithm using the generic spec charset and distinguish between “I don't know about your algorithm; maybe the encoded part here is ok” and “I don't know about your algorithm but I know your encoded part is wrong”.