Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improves namespace validation for Docker Notary integration #1019

Merged
merged 5 commits into from
Nov 10, 2016
Merged

Improves namespace validation for Docker Notary integration #1019

merged 5 commits into from
Nov 10, 2016

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Nov 5, 2016

This commit solves the issue related on #1012

Basically, instead of verifying if there is a '/' character on the a.Name variable, it's applied a split function on this string with the '/' character, then verifying the size of array.

Being bigger than 1, it verifies if the array is bigger than 3 (like privaterepo.com/rpkatz/debian) and getting only the 2nd item from array (rpkatz), otherwise (as being smaller than 3, and bigger than 1, like rpkatz/debian) it gets the first item of the array.

@vmwclabot
Copy link

@rikatz, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 50.652% when pulling 802fea6 on rikatz:master into 4dd5593 on vmware:master.

@reasonerjt
Copy link
Contributor

@rikatz thanks for the PR, this code change can be used to workaround the notary issue you are seeing but I don't think it universally correct as it disregard the hostname.

@rikatz
Copy link
Contributor Author

rikatz commented Nov 7, 2016

@reasonerjt I've opened an issue on Docker Notary Server (notaryproject/notary#1023) to clarify this.

I think this might be some kind of inconsistence, but anyway the usage of hostname on the scope is specified here: https://docs.docker.com/registry/spec/auth/scope/

I can't figure why this happens, and if Harbor should handle this on a more gentle way (like parsing its own 'hostname' configuration used on harbor bootstrap - harbor.cfg).

Anyway, let's wait for the answer on the related issue.

Thanks.

@rikatz
Copy link
Contributor Author

rikatz commented Nov 8, 2016

@reasonerjt So guys from notary said that this behaviour (sending the whole context) is right, as notary can sign anything (including tagged repositories), and this is a part of the image.

So I think it would be better to read the entire 'GUN' sended by notary, and check if the first field matches the configured hostname of Harbor, and if (and only if) this matches, it removes the first field and proceeds to the validation of the repo permission.

I'm busy now, but will try to fix this with the described approach, and update the PR. Do you think this is a fine approach?

Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 50.638% when pulling 72b44a1 on rikatz:master into 4dd5593 on vmware:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+21.0%) to 71.661% when pulling 804759c on rikatz:master into 4dd5593 on vmware:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 50.638% when pulling 7a1212d on rikatz:master into 4dd5593 on vmware:master.

@reasonerjt
Copy link
Contributor

@rikatz Thanks for the update! This PR looks good to me. Could you

  1. Let us know the issue you opened to notary
  2. submit this PR to dev branch so we can do more test on that before merging to master.

@rikatz
Copy link
Contributor Author

rikatz commented Nov 9, 2016

@reasonerjt The issue is #1012

I'm going to change the PR here to DEV branch.

@rikatz rikatz changed the base branch from master to dev November 9, 2016 11:01
@vmwclabot
Copy link

@rikatz, VMware has approved your signed contributor license agreement.

@reasonerjt
Copy link
Contributor

Thanks!

@reasonerjt reasonerjt merged commit 32f8c92 into goharbor:dev Nov 10, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants