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: Require lowercase hex #292

Merged
merged 1 commit into from
Sep 12, 2016

Conversation

wking
Copy link
Contributor

@wking wking commented Sep 9, 2016

Now that 75a51ed (fix regular expression of algorithim, 2016-08-30, #221) has made the Markdown and JSON Schema consistent (and required lowercase algorithms), make digest comparisons easier by also requiring lowercase hex.

This also:

  • Makes it easier to serve blobs out of a case-insensitive filesystem
    store.
  • Avoids having two otherwise-identical descriptor (or
    descriptor-containing) blobs with different hashes because they
    picked differend hex-casing.

@wking wking changed the title descriptor.md: Require lowercase hex descriptor: Require lowercase hex Sep 9, 2016
@wking wking force-pushed the lowecase-hex branch 2 times, most recently from c4ed400 to efb13e3 Compare September 9, 2016 22:07
@stevvooe
Copy link
Contributor

stevvooe commented Sep 9, 2016

LGTM

Approved with PullApprove

@runcom
Copy link
Member

runcom commented Sep 9, 2016

@stevvooe @wking is this a must for the spec? Should we add validation to oci-image-tool? (I'll fix it if so in a follow up not to block this)

Now that 75a51ed (fix regular expression of algorithim, 2016-08-30,
opencontainers#221) has made the Markdown and JSON Schema consistent (and required
lowercase algorithms), make digest comparisons easier by also
requiring lowercase hex.

This also:

* Makes it easier to serve blobs out of a case-insensitive filesystem
  store.
* Avoids having two otherwise-identical descriptor (or
  descriptor-containing) blobs with different hashes because they
  picked differend hex-casing.

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Sep 9, 2016

On Fri, Sep 09, 2016 at 03:40:09PM -0700, Antonio Murdaca wrote:

is this a must for the spec?

Yes. I've pushed efb13e3660d130 adding a MUST to make that
clearer.

Should we add validation to oci-image-tool?

This should be handled automatically via the JSON Schema regex (bumped
in this PR). We're using that as part of validate already, aren't we?

@runcom
Copy link
Member

runcom commented Sep 9, 2016

@stevvooe does the MUST look good to you?

@wking I can't check right now, I hope it's validating using the json schema.

@runcom
Copy link
Member

runcom commented Sep 10, 2016

@wking @stevvooe unfortunately with this PR, oci-image-tool isn't reporting any invalid digest format.

if you change a digest under refs/ to have some uppercase char you get:

oci-image-tool validate --ref latest . 
.: validation failed: sha256:2A2b6168c040d25148a4972cb20108b737adc51685d4dc68a8e01d55416d27a1: not found

it isn't saying anything about lower/upper cases..I guess it's the same for blobs/ (notice that I would have had the above to fail with "invalid digest, no uppercase!" or something like this, I know I renamed to digest and now it can't find the correct blob)

@stevvooe
Copy link
Contributor

@runcom I don't think a fix for that should be a pre-cursor to a merge here. Sounds like our validation is slightly broken.

Could we still merge this and file a bug?

@runcom
Copy link
Member

runcom commented Sep 12, 2016

@stevvooe absolutely, this is by any means a blocker, sorry if I intended that, please go ahead and merge this, I'll file a separate issue and probably wait to do this because I would love to have the separate repo for the tool in order not to make noise in the spec.

The fix in oci-image-tool will be done separately and it's part of a big refactor I have in mind about validation steps.

@jonboulle
Copy link
Contributor

jonboulle commented Sep 12, 2016

LGTM

Approved with PullApprove

@stevvooe
Copy link
Contributor

stevvooe commented Sep 12, 2016

LGTM

Approved with PullApprove

@stevvooe stevvooe merged commit 4aa74ac into opencontainers:master Sep 12, 2016
@wking wking deleted the lowecase-hex branch September 21, 2016 20:13
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.

4 participants