Create and populate htpasswd file if missing by liron-l · Pull Request #2362 · distribution/distribution

liron-l · 2017-08-13T06:08:48Z

If htpasswd authentication option is configured but the htpasswd file is
missing, populate it with a default user and automatically generated
password.
The password will be printed to stdout.

Signed-off-by: Liron Levin liron@twistlock.com

Replaced the default registry auth configuration from 'none' to 'htpasswd'. Following the change in distribution/distribution#2362. Signed-off-by: Liron Levin <liron@twistlock.com>

codecov · 2017-08-13T06:30:23Z

Codecov Report

Merging #2362 into master will decrease coverage by 9.63%.
The diff coverage is 45%.

@@ Coverage Diff @@## master #2362 +/- ##
==========================================
- Coverage 60.34% 50.71% -9.64% 
==========================================
Files 126 126 Lines 14436 14472 +36 ==========================================
- Hits 8712 7340 -1372 - Misses 4841 6381 +1540 + Partials 883 751 -132

Impacted Files	Coverage Δ
registry/handlers/app.go	`48.3% <100%> (ø)`	⬆️
registry/auth/htpasswd/access.go	`55.67% <43.58%> (-8.27%)`	⬇️
registry/storage/driver/gcs/gcs.go	`0.32% <0%> (-66.13%)`	⬇️
registry/storage/driver/oss/oss.go	`0.45% <0%> (-56.5%)`	⬇️
registry/storage/driver/s3-aws/s3.go	`4.07% <0%> (-55.4%)`	⬇️
registry/storage/driver/s3-goamz/s3.go	`0.4% <0%> (-52.4%)`	⬇️
registry/client/transport/transport.go	`68.75% <0%> (-13.75%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3800056...c785740. Read the comment docs.

liron-l · 2017-08-15T17:19:44Z

CC @dmcgowan

dmcgowan · 2017-08-15T17:57:40Z

+auth:
+ htpasswd:
+ realm: basic-realm
+ path: /auth/htpasswd


would prefer to use /var/lib/registry or something under /etc for the examples

Makes sense, I've put it under /etc/registry

dmcgowan · 2017-08-15T18:16:33Z

+ if _, err := f.Write([]byte(fmt.Sprintf("docker:%s", string(encryptedPass[:])))); err != nil {
+ return err
+ }
+ logrus.Warnf("htpasswd is missing. provisioned with default user:docker password: %s", pass)


This output looks kind of off
WARN[0000] htpasswd is missing. provisioned with default user:docker password: qakOn184C_WYgnUt7RDyE3xO7YrDkYhpqA9A2JbwIz0=
Maybe just getting rid of the space after password: would make it look more consistent. Also please use RawURLEncoding to get rid of the = at the end, some might find it confusing as to whether it is part of the password or part of an encoding.

Makes sense. I've removed the space and replaced the encoding method.

liron-l · 2017-08-15T18:34:42Z

Thanks @dmcgowan, I've updated the review.

Replaced the default registry auth configuration from 'none' to 'htpasswd'. Following the change in distribution/distribution#2362. Signed-off-by: Liron Levin <liron@twistlock.com>

liron-l · 2017-08-27T19:49:37Z

@dmcgowan PTAL

liron-l · 2017-09-07T08:06:54Z

@dmcgowan anything else I need to change?

liron-l · 2017-10-02T06:42:49Z

@tiborvass @dmcgowan PTAL.

endophage · 2017-10-09T16:36:04Z

@tiborvass @stevvooe @dmcgowan what's required to get this merged?

stevvooe · 2017-10-09T17:44:38Z


+// createHtpasswdFile creates and populates htpasswd file with a new user in case the file is missing
+func createHtpasswdFile(path string) error {
+	if _, err := os.Open(path); os.IsNotExist(err) {


Put the bulk of the work in the main path, not the indented path.

Any other non-empty error should also be returned

Thanks @stevvooe @dmcgowan fixed.

stevvooe · 2017-10-09T17:50:09Z

+ if _, err := f.Write([]byte(fmt.Sprintf("docker:%s", string(encryptedPass[:])))); err != nil {
+ return err
+ }
+ logrus.Warnf("htpasswd is missing. provisioned with default user:docker password:%s", pass)


Please use the logging system already used throughout the registry. We don't access logrus directly.

Also, use the field-based logging for these values, rather than just dumping them in the message.

Thanks @stevvooe can I use GetLoggerWithFields? if so, fixed.

liron-l · 2017-10-09T18:22:01Z

Thanks @stevvooe @dmcgowan @endophage I've updated the PR according to your comments.

stevvooe · 2017-10-09T18:28:26Z

+	if _, err := f.Write([]byte(fmt.Sprintf("docker:%s", string(encryptedPass[:])))); err != nil {
+ return err
+	}
+	dcontext.GetLoggerWithFields(context.Background(), map[interface{}]interface{}{


Also, this message should explain the "escape hatch", as this will confusing to those who have already ran the registry. I'll discuss the "escape hatch" more on distribution/distribution-library-image#58.

@stevvooe but this config only apply if you start the registry with htpasswd enabled.
If you use REGISTRY_AUTH="" (or silly with realm) it is not enabled.
Is it OK to write
Disable default basic authentication by overriding the 'REGISTRY_AUTH' environment variable?

@liron-l If you are using the registry behind a proxy, which is an extremely common deployment, this will break them and those users will panic. What do they do when this happens?

@stevvooe I think the scenario you describe is relevant to the hub image change (which is indeed a breaking change).
However, here we just automatically populate the username/password when they are missing in basic auth scenario (like Jenkins does on default setup).

dmcgowan · 2017-10-11T18:55:42Z

Can you rebase this to get CI passing, circleci does not seem to be building from a merged branch even when I kicked it to run without cache

liron-l · 2017-10-11T19:31:18Z

Thanks @dmcgowan, The tests passed now.
I'm not sure the code coverage check is related to my change (they appear to be failing in other PRs)

dmcgowan

LGTM

If htpasswd authentication option is configured but the htpasswd file is missing, populate it with a default user and automatically generated password. The password will be printed to stdout. Signed-off-by: Liron Levin <liron@twistlock.com>

jstoja · 2018-08-06T07:20:12Z

Is this still okay for you @dmcgowan?

dmp42

LGTM

dmp42 · 2018-08-31T22:38:29Z

@dmcgowan we broke something here. New PRs are not passing failing on gometalinter checks.

gometalinter --config .gometalinter.json ./...
registry/auth/htpasswd/access.go:1::warning: file is not goimported (goimports)
make: *** [check] Error 1

dmcgowan · 2018-09-04T21:30:18Z

We cannot merge old PRs that haven't run Travis, we can kick it off by opening a dummy PR with the change commit or having them rebased

dmp42 · 2018-09-05T00:32:06Z

@dmcgowan agreed.
My bad on this one.

liron-l mentioned this pull request Aug 13, 2017
Replaced default auth configuration from 'none' to 'htpasswd'. distribution/distribution-library-image#58
Open

dmcgowan reviewed Aug 15, 2017
View reviewed changes

liron-l force-pushed the populate_htpasswd branch from adae5b3 to 2cd4ed6Compare August 15, 2017 18:33

stevvooe reviewed Oct 9, 2017
View reviewed changes

dmcgowan added this to the Registry/2.7 milestone Oct 9, 2017

liron-l force-pushed the populate_htpasswd branch 3 times, most recently from fdcbea8 to da7d5e3Compare October 9, 2017 18:20

stevvooe reviewed Oct 9, 2017
View reviewed changes

liron-l force-pushed the populate_htpasswd branch from da7d5e3 to 4546330Compare October 11, 2017 19:06

dmcgowan approved these changes Oct 11, 2017
View reviewed changes

Create and populate htpasswd file if missing
c785740
If htpasswd authentication option is configured but the htpasswd file is missing, populate it with a default user and automatically generated password. The password will be printed to stdout. Signed-off-by: Liron Levin <liron@twistlock.com>

liron-l force-pushed the populate_htpasswd branch from 4546330 to c785740Compare November 1, 2017 10:30

dmp42 approved these changes Aug 31, 2018
View reviewed changes

dmp42 merged commit 90705d2 into distribution:masterAug 31, 2018

dmp42 mentioned this pull request Aug 31, 2018
Update ncw/swift Vendor Package #2698
Merged

davidswu mentioned this pull request Sep 5, 2018
fix gometalinter #2701
Merged

Uh oh!

Conversation

liron-l commented Aug 13, 2017

Uh oh!

codecovBot commented Aug 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liron-l commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liron-l commented Aug 15, 2017

Uh oh!

liron-l commented Aug 27, 2017

Uh oh!

liron-l commented Sep 7, 2017

Uh oh!

liron-l commented Oct 2, 2017

Uh oh!

endophage commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liron-l commented Oct 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Oct 11, 2017

Uh oh!

liron-l commented Oct 11, 2017

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

jstoja commented Aug 6, 2018

Uh oh!

dmp42 left a comment

Choose a reason for hiding this comment

Uh oh!

dmp42 commented Aug 31, 2018

Uh oh!

dmcgowan commented Sep 4, 2018

Uh oh!

dmp42 commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecovBot commented Aug 13, 2017•
edited
Loading