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

descriptor: sha{256,512}: disallow /[A-F]/ characters #667

Merged
merged 1 commit into from
May 12, 2017

Conversation

AkihiroSuda
Copy link
Member

Disallowing upper characters will be useful for avoiding implementation bugs

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@@ -137,11 +140,17 @@ If a useful algorithm is not included in the above table, it SHOULD be submitted
[SHA-256][rfc4634-s4.1] is a collision-resistant hash function, chosen for ubiquity, reasonable size and secure characteristics.
Implementations MUST implement SHA-256 digest verification for use in descriptors.

When the _algorithm identifier_ is `sha256`, the _encoded_ portion MUST match `/[a-f0-9]{64}/`.
Note that `[A-F]` MUST NOT be used here.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather stay DRY by defining a hex encoding with these restrictions, and then saying that sha256 and sha512 MUST be hex-encoded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think hex-encoding is specific to algorithm.
Depending on the algorithm, hex-encoded digest can be somealgorithm+uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think hex-encoding is specific to algorithm.

This is true, but both of the current algorithms use the same hex encoding. Defining that in one place doesn't block other algorithms from defining or using alternative hex encodings, but it does make it easier for other algorithms that want the current hex encoding.

@erikh
Copy link
Contributor

erikh commented May 11, 2017 via email

@AkihiroSuda
Copy link
Member Author

@erikh

base64 should use different algorithm identifier, so I think that is the out scope of this PR, which only focuses on sha256 and sha512.

@stevvooe
Copy link
Contributor

@AkihiroSuda This looks good. Could you also make a PR to go-digest?

@AkihiroSuda
Copy link
Member Author

sure opencontainers/go-digest#34

@stevvooe
Copy link
Contributor

stevvooe commented May 12, 2017

LGTM

Approved with PullApprove

1 similar comment
@vbatts
Copy link
Member

vbatts commented May 12, 2017

LGTM

Approved with PullApprove

@vbatts vbatts merged commit 3437101 into opencontainers:master May 12, 2017
@vbatts vbatts mentioned this pull request May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants