search

openshift / oauth-proxy

A reverse proxy that provides authentication with OpenShift via OAuth and Kubernetes service accounts
MIT License
261 stars 137 forks source link

Bug 1874322: add bcrypt as a supported hashing method for htpasswd passwords #186

Closed stlaz closed 3 years ago

stlaz commented 3 years ago

The PR is based on https://github.com/bitly/oauth2_proxy/commit/008ffae3bb5f1d068f2ea4dddb88ea80a6697297 with the expection of not including the $2b$ prefix based on https://svn.apache.org/viewvc/apr/apr/trunk/crypto/crypt_blowfish.c?view=markup#l580

openshift-ci-robot commented 3 years ago

@stlaz: This pull request references Bugzilla bug 1874322, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/oauth-proxy/pull/186): >Bug 1874322: add bcrypt as a supported hashing method for htpasswd passwords Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-ci-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/oauth-proxy/blob/master/OWNERS)~~ [stlaz] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
stlaz commented 3 years ago

/refresh

stlaz commented 3 years ago

/retest impossibru, the unit test shouldn't fail

stlaz commented 3 years ago

/bugzilla refresh

openshift-ci-robot commented 3 years ago

@stlaz: This pull request references Bugzilla bug 1874322, which is invalid:

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to [this](https://github.com/openshift/oauth-proxy/pull/186#issuecomment-708248057): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
stlaz commented 3 years ago

/bugzilla refresh

openshift-ci-robot commented 3 years ago

@stlaz: This pull request references Bugzilla bug 1874322, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target release (4.7.0) matches configured target release for branch (4.7.0) * bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
In response to [this](https://github.com/openshift/oauth-proxy/pull/186#issuecomment-708313138): >/bugzilla refresh Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
stlaz commented 3 years ago

/retest

deads2k commented 3 years ago

I don't think this will actually resolve the bug since the bug is about the possibility to store passwords in sha1, not the requirement to do so.

Also, why do we attempt to verify passwords in this proxy at all? That doesn't make much sense to me.

stlaz commented 3 years ago

@deads2k we can't remove the sha1-logins as that would break everyone who is using those, unfortunately.

This is not exactly password verification for the oauth-users, it is a htpasswd config that allows to bypass the oauth-login. I think it might be thought about as kind of a service-account idp for the proxy.

openshift-bot commented 3 years ago

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

stlaz commented 3 years ago

/remove-lifecycle stale for now

s-urbaniak commented 3 years ago

agreed with @stlaz, this just adds the bcrypt capability, while not removing sha1

s-urbaniak commented 3 years ago

/lgtm

s-urbaniak commented 3 years ago

/retest

openshift-ci[bot] commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: s-urbaniak, stlaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/oauth-proxy/blob/master/OWNERS)~~ [stlaz] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
s-urbaniak commented 3 years ago

/retest

openshift-bot commented 3 years ago

/retest

Please review the full test history for this PR and help us cut down flakes.

stlaz commented 3 years ago

/test e2e-component

openshift-ci[bot] commented 3 years ago

@stlaz: All pull requests linked via external trackers have merged:

Bugzilla bug 1874322 has been moved to the MODIFIED state.

In response to [this](https://github.com/openshift/oauth-proxy/pull/186): >Bug 1874322: add bcrypt as a supported hashing method for htpasswd passwords Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
openshift-bot commented 9 months ago

[ART PR BUILD NOTIFIER]

This PR has been included in build golang-github-openshift-oauth-proxy-container-v4.8.0-202311261141.p0.g3fc0d89.assembly.stream for distgit golang-github-openshift-oauth-proxy. All builds following this will include this PR.