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

enable token based auth #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kaplan2539
Copy link

Hi yan-foto,

I need to be able to do token based authentication. A while ago x-lhan
made a PR that contained (among other things) necessary changes for
this work. This is the x-lhan's work distilled and applied to current master.
Do you think it would be possible to merge this into your main branch?

C.f. bc790f8

Cheers
Alex

This is the x-lhan's work distilled and applied to current master.
C.f. fraunhoferfokus@bc790f8
@yan-foto
Copy link
Member

yan-foto commented Apr 3, 2020

Hi Alex! Thanks for the PR. The old PR had too many issues to be merged and is more or less deserted by now. However, @jgimenez opened a PR last week (see #33) which addresses exactly this issue. He wanted to open a new, fine grained PR, but I am not aware of its current state. Let me ping him and if he's out, we'll merge this one.

@jgimenez
Copy link

jgimenez commented Apr 3, 2020

If this works, I'm happy to use this :)

Quickly reviewing this code, it looks cleaner than mine

Copy link
Member

@yan-foto yan-foto left a comment

Choose a reason for hiding this comment

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

There are some changes that need to be done before this PR can be merged. I am not sure if you want to make all these changes or I should simply cherry pick the changes from #33 and have this PR closed.

"golang.org/x/crypto/ssh/terminal"

log "github.com/sirupsen/logrus"
"github.com/docker/distribution/context"
schema2 "github.com/docker/distribution/manifest/schema2"
"github.com/docker/distribution/reference"
"github.com/docker/distribution/registry/client"
"github.com/fraunhoferfokus/deckschrubber/util"
Copy link
Member

Choose a reason for hiding this comment

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

If you remove this dependency, you also need to remove the file, as it is not used anywhere else.

"golang.org/x/crypto/ssh/terminal"

log "github.com/sirupsen/logrus"
"github.com/docker/distribution/context"
schema2 "github.com/docker/distribution/manifest/schema2"
"github.com/docker/distribution/reference"
"github.com/docker/distribution/registry/client"
"github.com/fraunhoferfokus/deckschrubber/util"
"github.com/heroku/docker-registry-client/registry"
Copy link
Member

Choose a reason for hiding this comment

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

This causes me a bit of unease as I always prefer to keep the dependencies as small as possible (see for example how @jgimenez solved this problem here), but I suppose heroku is trustworthy enough.

if *insecure {
transport = &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
Copy link
Member

Choose a reason for hiding this comment

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

This value should not be hard coded. Again, see how it is solved in #33

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.

3 participants